| Message ID | 20240716114229.329355-1-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] gdbstub: Re-factor gdb command extensions | expand |
Hi Alex, On 7/16/24 8:42 AM, Alex Bennée wrote: > Coverity reported a memory leak (CID 1549757) in this code and its > admittedly rather clumsy handling of extending the command table. > Instead of handing over a full array of the commands lets use the > lighter weight GPtrArray and simply test for the presence of each > entry as we go. This avoids complications of transferring ownership of > arrays and keeps the final command entries as static entries in the > target code. How did you run Coverity to find the leak? I'm wondering what's the quickest way to check it next time. > Cc: Akihiko Odaki <akihiko.odaki@daynix.com> > Cc: Gustavo Bueno Romero <gustavo.romero@linaro.org> > Cc: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org> Cheers, Gustavo
Gustavo Romero <gustavo.romero@linaro.org> writes: > Hi Alex, > > On 7/16/24 8:42 AM, Alex Bennée wrote: >> Coverity reported a memory leak (CID 1549757) in this code and its >> admittedly rather clumsy handling of extending the command table. >> Instead of handing over a full array of the commands lets use the >> lighter weight GPtrArray and simply test for the presence of each >> entry as we go. This avoids complications of transferring ownership of >> arrays and keeps the final command entries as static entries in the >> target code. > How did you run Coverity to find the leak? I'm wondering what's the > quickest way to check it next time. Coverity is only run in the cloud on the released build. There is a container somewhere but I don't know how its used. I did test on a build with --enable-sanitizers though. > > >> Cc: Akihiko Odaki <akihiko.odaki@daynix.com> >> Cc: Gustavo Bueno Romero <gustavo.romero@linaro.org> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org> > > > Cheers, > Gustavo
On Tue, 16 Jul 2024 at 14:48, Alex Bennée <alex.bennee@linaro.org> wrote: > > Gustavo Romero <gustavo.romero@linaro.org> writes: > > > Hi Alex, > > > > On 7/16/24 8:42 AM, Alex Bennée wrote: > >> Coverity reported a memory leak (CID 1549757) in this code and its > >> admittedly rather clumsy handling of extending the command table. > >> Instead of handing over a full array of the commands lets use the > >> lighter weight GPtrArray and simply test for the presence of each > >> entry as we go. This avoids complications of transferring ownership of > >> arrays and keeps the final command entries as static entries in the > >> target code. > > How did you run Coverity to find the leak? I'm wondering what's the > > quickest way to check it next time. > > Coverity is only run in the cloud on the released build. There is a > container somewhere but I don't know how its used. The Coverity cloud stuff comes in two parts: (1) you build locally with the Coverity tools, which creates a big opaque build-artefact (2) you upload that artefact to the cloud server, and the actual analysis happens on the cloud The container stuff and the integration with the Gitlab CI is there for the sole purpose of automating the "local build and upload" steps. You can't do an analysis-run locally. (Well, you probably can if you have the paid-for commercial version of the tooling, but we haven't got any kind of setup for doing that.) We only do analysis runs on head-of-git because the Coverity Scan resource limits for open source projects give us about one complete scan a day. So this is all "after the fact" stuff. Developers who want to look at the scan results can create an account via https://scan.coverity.com/projects/qemu . Triaging new coverity reports is a bit tedious because there are a ton of false positives... thanks -- PMM
Hi Peter, On 7/16/24 11:09 AM, Peter Maydell wrote: > On Tue, 16 Jul 2024 at 14:48, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Gustavo Romero <gustavo.romero@linaro.org> writes: >> >>> Hi Alex, >>> >>> On 7/16/24 8:42 AM, Alex Bennée wrote: >>>> Coverity reported a memory leak (CID 1549757) in this code and its >>>> admittedly rather clumsy handling of extending the command table. >>>> Instead of handing over a full array of the commands lets use the >>>> lighter weight GPtrArray and simply test for the presence of each >>>> entry as we go. This avoids complications of transferring ownership of >>>> arrays and keeps the final command entries as static entries in the >>>> target code. >>> How did you run Coverity to find the leak? I'm wondering what's the >>> quickest way to check it next time. >> >> Coverity is only run in the cloud on the released build. There is a >> container somewhere but I don't know how its used. > > The Coverity cloud stuff comes in two parts: > (1) you build locally with the Coverity tools, which creates > a big opaque build-artefact > (2) you upload that artefact to the cloud server, and the > actual analysis happens on the cloud > > The container stuff and the integration with the Gitlab CI > is there for the sole purpose of automating the "local build > and upload" steps. You can't do an analysis-run locally. > (Well, you probably can if you have the paid-for commercial > version of the tooling, but we haven't got any kind of > setup for doing that.) > > We only do analysis runs on head-of-git because the Coverity Scan > resource limits for open source projects give us about one > complete scan a day. So this is all "after the fact" stuff. > Developers who want to look at the scan results can create > an account via https://scan.coverity.com/projects/qemu . > Triaging new coverity reports is a bit tedious because there > are a ton of false positives... Thanks for the explanation! Cheers, Gustavo
On 7/16/24 21:42, Alex Bennée wrote: > void gdb_extend_qsupported_features(char *qsupported_features) > { > - /* > - * We don't support different sets of CPU gdb features on different CPUs yet > - * so assert the feature strings are the same on all CPUs, or is set only > - * once (1 CPU). > - */ > - g_assert(extended_qsupported_features == NULL || > - g_strcmp0(extended_qsupported_features, qsupported_features) == 0); > - > - extended_qsupported_features = qsupported_features; > + if (!extended_qsupported_features) { > + extended_qsupported_features = g_strdup(qsupported_features); > + } else if (!g_strrstr(extended_qsupported_features, qsupported_features)) { Did you really need the last instance of the substring? I'll note that g_strrstr is quite simplistic, whereas strstr has a much more scalable algorithm. > + char *old = extended_qsupported_features; > + extended_qsupported_features = g_strdup_printf("%s%s", old, qsupported_features); Right tool for the right job, please: g_strconcat(). That said, did you *really* want to concatenate now, and have to search through the middle, as opposed to storing N strings separately? You could defer the concat until the actual negotiation with gdb. That would reduce strstr above to a loop over strcmp. > + for (int i = 0; i < extensions->len; i++) { > + gpointer entry = g_ptr_array_index(extensions, i); > + if (!g_ptr_array_find(table, entry, NULL)) { > + g_ptr_array_add(table, entry); Are you expecting the same GdbCmdParseEntry object to be registered multiple times? Can we fix that at a higher level? r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 7/16/24 21:42, Alex Bennée wrote: >> void gdb_extend_qsupported_features(char *qsupported_features) >> { >> - /* >> - * We don't support different sets of CPU gdb features on different CPUs yet >> - * so assert the feature strings are the same on all CPUs, or is set only >> - * once (1 CPU). >> - */ >> - g_assert(extended_qsupported_features == NULL || >> - g_strcmp0(extended_qsupported_features, qsupported_features) == 0); >> - >> - extended_qsupported_features = qsupported_features; >> + if (!extended_qsupported_features) { >> + extended_qsupported_features = g_strdup(qsupported_features); >> + } else if (!g_strrstr(extended_qsupported_features, qsupported_features)) { > > Did you really need the last instance of the substring? Not really - I just want to check the string hasn't been added before. > > I'll note that g_strrstr is quite simplistic, whereas strstr has a > much more scalable algorithm. > > >> + char *old = extended_qsupported_features; >> + extended_qsupported_features = g_strdup_printf("%s%s", old, qsupported_features); > > Right tool for the right job, please: g_strconcat(). > > That said, did you *really* want to concatenate now, and have to > search through the middle, as opposed to storing N strings separately? > You could defer the concat until the actual negotiation with gdb. > That would reduce strstr above to a loop over strcmp. > >> + for (int i = 0; i < extensions->len; i++) { >> + gpointer entry = g_ptr_array_index(extensions, i); >> + if (!g_ptr_array_find(table, entry, NULL)) { >> + g_ptr_array_add(table, entry); > > Are you expecting the same GdbCmdParseEntry object to be registered > multiple times? Can we fix that at a higher level? Its basically a hack to deal with the fact everything is tied to the CPUObject so we register everything multiple times. We could do a if (!registerd) register() dance but I guess I'm thinking forward to a hydrogenous future but I guess we'd need to do more work then anyway. > > > r~
On 7/17/24 02:55, Alex Bennée wrote: >> Are you expecting the same GdbCmdParseEntry object to be registered >> multiple times? Can we fix that at a higher level? > > Its basically a hack to deal with the fact everything is tied to the > CPUObject so we register everything multiple times. We could do a if > (!registerd) register() dance but I guess I'm thinking forward to a > hydrogenous future but I guess we'd need to do more work then anyway. Any chance we could move it all to the CPUClass? r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 7/17/24 02:55, Alex Bennée wrote: >>> Are you expecting the same GdbCmdParseEntry object to be registered >>> multiple times? Can we fix that at a higher level? >> Its basically a hack to deal with the fact everything is tied to the >> CPUObject so we register everything multiple times. We could do a if >> (!registerd) register() dance but I guess I'm thinking forward to a >> hydrogenous future but I guess we'd need to do more work then anyway. > > Any chance we could move it all to the CPUClass? We would have to move quite a bunch of stuff, including the system register creation code. I don't think that's a re-factor I want to attempt quite so close to soft-freeze. > > > r~
On Tue, 16 Jul 2024 at 22:53, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 7/17/24 02:55, Alex Bennée wrote: > >> Are you expecting the same GdbCmdParseEntry object to be registered > >> multiple times? Can we fix that at a higher level? > > > > Its basically a hack to deal with the fact everything is tied to the > > CPUObject so we register everything multiple times. We could do a if > > (!registerd) register() dance but I guess I'm thinking forward to a > > hydrogenous future but I guess we'd need to do more work then anyway. > > Any chance we could move it all to the CPUClass? No, because different instances of the same CPUClass might have different feature sets. In this case, one CPU might have MTE and another not, or one be AArch64 and another not. The underlying problem here is that there's quite a lot here that potentially varies across different CPUs in the system, but the gdbstub layer has an assumption of heterogeneity. (cf also the stuff about system registers.) thanks -- PMM
diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h index f3058f9dda..40f0514fe9 100644 --- a/include/gdbstub/commands.h +++ b/include/gdbstub/commands.h @@ -74,23 +74,28 @@ int gdb_put_packet(const char *buf); /** * gdb_extend_query_table() - Extend query table. - * @table: The table with the additional query packet handlers. - * @size: The number of handlers to be added. + * @table: GPtrArray of GdbCmdParseEntry entries. + * + * The caller should free @table afterwards */ -void gdb_extend_query_table(GdbCmdParseEntry *table, int size); +void gdb_extend_query_table(GPtrArray *table); /** * gdb_extend_set_table() - Extend set table. - * @table: The table with the additional set packet handlers. - * @size: The number of handlers to be added. + * @table: GPtrArray of GdbCmdParseEntry entries. + * + * The caller should free @table afterwards */ -void gdb_extend_set_table(GdbCmdParseEntry *table, int size); +void gdb_extend_set_table(GPtrArray *table); /** * gdb_extend_qsupported_features() - Extend the qSupported features string. * @qsupported_features: The additional qSupported feature(s) string. The string * should start with a semicolon and, if there are more than one feature, the - * features should be separate by a semiocolon. + * features should be separate by a semicolon. + * + * The caller should free @qsupported_features afterwards if + * dynamically allocated. */ void gdb_extend_qsupported_features(char *qsupported_features); diff --git a/target/arm/internals.h b/target/arm/internals.h index da22d04121..757b1fae92 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -359,8 +359,8 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu); void arm_translate_init(void); void arm_cpu_register_gdb_commands(ARMCPU *cpu); -void aarch64_cpu_register_gdb_commands(ARMCPU *cpu, GString *, GArray *, - GArray *); +void aarch64_cpu_register_gdb_commands(ARMCPU *cpu, GString *, + GPtrArray *, GPtrArray *); void arm_restore_state_to_opc(CPUState *cs, const TranslationBlock *tb, diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index b9ad0a063e..fd7b4ecddb 100644 --- a/gdbstub/gdbstub.c +++ b/gdbstub/gdbstub.c @@ -1615,17 +1615,16 @@ static void handle_query_thread_extra(GArray *params, void *user_ctx) } static char *extended_qsupported_features; + void gdb_extend_qsupported_features(char *qsupported_features) { - /* - * We don't support different sets of CPU gdb features on different CPUs yet - * so assert the feature strings are the same on all CPUs, or is set only - * once (1 CPU). - */ - g_assert(extended_qsupported_features == NULL || - g_strcmp0(extended_qsupported_features, qsupported_features) == 0); - - extended_qsupported_features = qsupported_features; + if (!extended_qsupported_features) { + extended_qsupported_features = g_strdup(qsupported_features); + } else if (!g_strrstr(extended_qsupported_features, qsupported_features)) { + char *old = extended_qsupported_features; + extended_qsupported_features = g_strdup_printf("%s%s", old, qsupported_features); + g_free(old); + } } static void handle_query_supported(GArray *params, void *user_ctx) @@ -1753,39 +1752,59 @@ static const GdbCmdParseEntry gdb_gen_query_set_common_table[] = { }, }; -/* Compares if a set of command parsers is equal to another set of parsers. */ -static bool cmp_cmds(GdbCmdParseEntry *c, GdbCmdParseEntry *d, int size) +/** + * extend_table() - extend one of the command tables + * @table: the command table to extend (or NULL) + * @extensions: a list of GdbCmdParseEntry pointers + * + * The entries themselves should be pointers to static const + * GdbCmdParseEntry entries. If the entry is already in the table we + * skip adding it again. + * + * Returns (a potentially freshly allocated) GPtrArray of GdbCmdParseEntry + */ +static GPtrArray * extend_table(GPtrArray *table, GPtrArray *extensions) { - for (int i = 0; i < size; i++) { - if (!(c[i].handler == d[i].handler && - g_strcmp0(c[i].cmd, d[i].cmd) == 0 && - c[i].cmd_startswith == d[i].cmd_startswith && - g_strcmp0(c[i].schema, d[i].schema) == 0)) { + if (!table) { + table = g_ptr_array_new(); + } - /* Sets are different. */ - return false; + + for (int i = 0; i < extensions->len; i++) { + gpointer entry = g_ptr_array_index(extensions, i); + if (!g_ptr_array_find(table, entry, NULL)) { + g_ptr_array_add(table, entry); } } - /* Sets are equal, i.e. contain the same command parsers. */ - return true; + return table; } -static GdbCmdParseEntry *extended_query_table; -static int extended_query_table_size; -void gdb_extend_query_table(GdbCmdParseEntry *table, int size) +/** + * process_extended_table() - run through an extended command table + * @table: the command table to check + * @data: parameters + * + * returns true if the command was found and executed + */ +static bool process_extended_table(GPtrArray *table, const char *data) { - /* - * We don't support different sets of CPU gdb features on different CPUs yet - * so assert query table is the same on all CPUs, or is set only once - * (1 CPU). - */ - g_assert(extended_query_table == NULL || - (extended_query_table_size == size && - cmp_cmds(extended_query_table, table, size))); + for (int i = 0; i < table->len; i++) { + const GdbCmdParseEntry *entry = g_ptr_array_index(table, i); + if (process_string_cmd(data, entry, 1)) { + return true; + } + } + return false; +} - extended_query_table = table; - extended_query_table_size = size; + +/* Ptr to GdbCmdParseEntry */ +static GPtrArray *extended_query_table; + +void gdb_extend_query_table(GPtrArray *new_queries) +{ + extended_query_table = extend_table(extended_query_table, new_queries); } static const GdbCmdParseEntry gdb_gen_query_table[] = { @@ -1880,20 +1899,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = { #endif }; -static GdbCmdParseEntry *extended_set_table; -static int extended_set_table_size; -void gdb_extend_set_table(GdbCmdParseEntry *table, int size) -{ - /* - * We don't support different sets of CPU gdb features on different CPUs yet - * so assert set table is the same on all CPUs, or is set only once (1 CPU). - */ - g_assert(extended_set_table == NULL || - (extended_set_table_size == size && - cmp_cmds(extended_set_table, table, size))); +/* Ptr to GdbCmdParseEntry */ +static GPtrArray *extended_set_table; - extended_set_table = table; - extended_set_table_size = size; +void gdb_extend_set_table(GPtrArray *new_set) +{ + extended_set_table = extend_table(extended_set_table, new_set); } static const GdbCmdParseEntry gdb_gen_set_table[] = { @@ -1924,26 +1935,28 @@ static const GdbCmdParseEntry gdb_gen_set_table[] = { static void handle_gen_query(GArray *params, void *user_ctx) { + const char *data; + if (!params->len) { return; } - if (process_string_cmd(gdb_get_cmd_param(params, 0)->data, + data = gdb_get_cmd_param(params, 0)->data; + + if (process_string_cmd(data, gdb_gen_query_set_common_table, ARRAY_SIZE(gdb_gen_query_set_common_table))) { return; } - if (process_string_cmd(gdb_get_cmd_param(params, 0)->data, + if (process_string_cmd(data, gdb_gen_query_table, ARRAY_SIZE(gdb_gen_query_table))) { return; } if (extended_query_table && - process_string_cmd(gdb_get_cmd_param(params, 0)->data, - extended_query_table, - extended_query_table_size)) { + process_extended_table(extended_query_table, data)) { return; } @@ -1953,26 +1966,28 @@ static void handle_gen_query(GArray *params, void *user_ctx) static void handle_gen_set(GArray *params, void *user_ctx) { + const char *data; + if (!params->len) { return; } - if (process_string_cmd(gdb_get_cmd_param(params, 0)->data, + data = gdb_get_cmd_param(params, 0)->data; + + if (process_string_cmd(data, gdb_gen_query_set_common_table, ARRAY_SIZE(gdb_gen_query_set_common_table))) { return; } - if (process_string_cmd(gdb_get_cmd_param(params, 0)->data, + if (process_string_cmd(data, gdb_gen_set_table, ARRAY_SIZE(gdb_gen_set_table))) { return; } if (extended_set_table && - process_string_cmd(gdb_get_cmd_param(params, 0)->data, - extended_set_table, - extended_set_table_size)) { + process_extended_table(extended_set_table, data)) { return; } diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c index c3a9b5eb1e..554b8736bb 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -477,11 +477,9 @@ static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs, void arm_cpu_register_gdb_commands(ARMCPU *cpu) { - GArray *query_table = - g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry)); - GArray *set_table = - g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry)); - GString *qsupported_features = g_string_new(NULL); + g_autoptr(GPtrArray) query_table = g_ptr_array_new(); + g_autoptr(GPtrArray) set_table = g_ptr_array_new(); + g_autoptr(GString) qsupported_features = g_string_new(NULL); if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { #ifdef TARGET_AARCH64 @@ -492,16 +490,12 @@ void arm_cpu_register_gdb_commands(ARMCPU *cpu) /* Set arch-specific handlers for 'q' commands. */ if (query_table->len) { - gdb_extend_query_table(&g_array_index(query_table, - GdbCmdParseEntry, 0), - query_table->len); + gdb_extend_query_table(query_table); } /* Set arch-specific handlers for 'Q' commands. */ if (set_table->len) { - gdb_extend_set_table(&g_array_index(set_table, - GdbCmdParseEntry, 0), - set_table->len); + gdb_extend_set_table(set_table); } /* Set arch-specific qSupported feature. */ diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c index 2e2bc2700b..c8cef8cbc0 100644 --- a/target/arm/gdbstub64.c +++ b/target/arm/gdbstub64.c @@ -564,7 +564,7 @@ enum Command { NUM_CMDS }; -static GdbCmdParseEntry cmd_handler_table[NUM_CMDS] = { +static const GdbCmdParseEntry cmd_handler_table[NUM_CMDS] = { [qMemTags] = { .handler = handle_q_memtag, .cmd_startswith = true, @@ -590,17 +590,16 @@ static GdbCmdParseEntry cmd_handler_table[NUM_CMDS] = { #endif /* CONFIG_USER_ONLY */ void aarch64_cpu_register_gdb_commands(ARMCPU *cpu, GString *qsupported, - GArray *qtable, GArray *stable) + GPtrArray *qtable, GPtrArray *stable) { #ifdef CONFIG_USER_ONLY /* MTE */ if (cpu_isar_feature(aa64_mte, cpu)) { g_string_append(qsupported, ";memory-tagging+"); - g_array_append_val(qtable, cmd_handler_table[qMemTags]); - g_array_append_val(qtable, cmd_handler_table[qIsAddressTagged]); - - g_array_append_val(stable, cmd_handler_table[QMemTags]); + g_ptr_array_add(qtable, (gpointer) &cmd_handler_table[qMemTags]); + g_ptr_array_add(qtable, (gpointer) &cmd_handler_table[qIsAddressTagged]); + g_ptr_array_add(stable, (gpointer) &cmd_handler_table[QMemTags]); } #endif }
Coverity reported a memory leak (CID 1549757) in this code and its admittedly rather clumsy handling of extending the command table. Instead of handing over a full array of the commands lets use the lighter weight GPtrArray and simply test for the presence of each entry as we go. This avoids complications of transferring ownership of arrays and keeps the final command entries as static entries in the target code. Cc: Akihiko Odaki <akihiko.odaki@daynix.com> Cc: Gustavo Bueno Romero <gustavo.romero@linaro.org> Cc: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/gdbstub/commands.h | 19 ++++-- target/arm/internals.h | 4 +- gdbstub/gdbstub.c | 127 +++++++++++++++++++++---------------- target/arm/gdbstub.c | 16 ++--- target/arm/gdbstub64.c | 11 ++-- 5 files changed, 95 insertions(+), 82 deletions(-)