* [PATCH v2 1/3] gdb: use std::string for internalvar::name @ 2023-02-14 19:23 Simon Marchi 2023-02-14 19:23 ` [PATCH v2 2/3] gdb: add constructor to internalvar Simon Marchi ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Simon Marchi @ 2023-02-14 19:23 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Change internalvar::name to std::string, automating memory management. It becomes necessary to allocate internalvar with new instead of XNEW. I didn't find how to trigger the code in complete_internalvar. It is called from condition_completer, so it should be by using the "condition" command, but I never managed to get in the right code path. Change-Id: I814d61361663e7becb8f3fb5f58c0180cdc414bc --- gdb/value.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/gdb/value.c b/gdb/value.c index 7873aeb9558e..936949f4a1f1 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -1830,7 +1830,7 @@ union internalvar_data struct internalvar { struct internalvar *next; - char *name; + std::string name; /* We support various different kinds of content of an internal variable. enum internalvar_kind specifies the kind, and union internalvar_data @@ -1894,7 +1894,7 @@ lookup_only_internalvar (const char *name) struct internalvar *var; for (var = internalvars; var; var = var->next) - if (strcmp (var->name, name) == 0) + if (var->name == name) return var; return NULL; @@ -1912,8 +1912,8 @@ complete_internalvar (completion_tracker &tracker, const char *name) len = strlen (name); for (var = internalvars; var; var = var->next) - if (strncmp (var->name, name, len) == 0) - tracker.add_completion (make_unique_xstrdup (var->name)); + if (var->name.compare (0, len, name) == 0) + tracker.add_completion (make_unique_xstrdup (var->name.c_str ())); } /* Create an internal variable with name NAME and with a void value. @@ -1922,9 +1922,9 @@ complete_internalvar (completion_tracker &tracker, const char *name) struct internalvar * create_internalvar (const char *name) { - struct internalvar *var = XNEW (struct internalvar); + internalvar *var = new internalvar; - var->name = xstrdup (name); + var->name = name; var->kind = INTERNALVAR_VOID; var->next = internalvars; internalvars = var; @@ -1996,7 +1996,7 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var) /* If there is a trace state variable of the same name, assume that is what we really want to see. */ - tsv = find_trace_state_variable (var->name); + tsv = find_trace_state_variable (var->name.c_str ()); if (tsv) { tsv->value_known = target_get_trace_state_variable_value (tsv->number, @@ -2149,7 +2149,7 @@ set_internalvar (struct internalvar *var, struct value *val) union internalvar_data new_data = { 0 }; if (var->kind == INTERNALVAR_FUNCTION && var->u.fn.canonical) - error (_("Cannot overwrite convenience function %s"), var->name); + error (_("Cannot overwrite convenience function %s"), var->name.c_str ()); /* Prepare new contents. */ switch (check_typedef (val->type ())->code ()) @@ -2261,7 +2261,7 @@ clear_internalvar (struct internalvar *var) const char * internalvar_name (const struct internalvar *var) { - return var->name; + return var->name.c_str (); } static struct internal_function * @@ -2450,7 +2450,7 @@ show_convenience (const char *ignore, int from_tty) { varseen = 1; } - gdb_printf (("$%s = "), var->name); + gdb_printf (("$%s = "), var->name.c_str ()); try { -- 2.39.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] gdb: add constructor to internalvar 2023-02-14 19:23 [PATCH v2 1/3] gdb: use std::string for internalvar::name Simon Marchi @ 2023-02-14 19:23 ` Simon Marchi 2023-02-14 20:42 ` Tom Tromey 2023-02-14 19:23 ` [PATCH v2 3/3] gdb: store internalvars in an std::map Simon Marchi 2023-02-14 20:37 ` [PATCH v2 1/3] gdb: use std::string for internalvar::name Tom Tromey 2 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2023-02-14 19:23 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Add a constructor that takes the name as a parameter. Initialize the Change-Id: Ic4db0aba85f1da9f12f3eee0ac62c0e5ef0cfe88 --- gdb/value.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/gdb/value.c b/gdb/value.c index 936949f4a1f1..7fd11ba70db6 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -1829,14 +1829,18 @@ union internalvar_data struct internalvar { - struct internalvar *next; + internalvar (std::string name) + : name (std::move (name)) + {} + + struct internalvar *next = nullptr; std::string name; /* We support various different kinds of content of an internal variable. enum internalvar_kind specifies the kind, and union internalvar_data provides the data associated with this particular kind. */ - enum internalvar_kind kind; + enum internalvar_kind kind = INTERNALVAR_VOID; union internalvar_data u; }; @@ -1922,10 +1926,8 @@ complete_internalvar (completion_tracker &tracker, const char *name) struct internalvar * create_internalvar (const char *name) { - internalvar *var = new internalvar; + internalvar *var = new internalvar (name); - var->name = name; - var->kind = INTERNALVAR_VOID; var->next = internalvars; internalvars = var; return var; -- 2.39.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] gdb: add constructor to internalvar 2023-02-14 19:23 ` [PATCH v2 2/3] gdb: add constructor to internalvar Simon Marchi @ 2023-02-14 20:42 ` Tom Tromey 2023-02-15 16:38 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Tom Tromey @ 2023-02-14 20:42 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> Add a constructor that takes the name as a parameter. Initialize the This is truncated. Tom ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] gdb: add constructor to internalvar 2023-02-14 20:42 ` Tom Tromey @ 2023-02-15 16:38 ` Simon Marchi 2023-02-15 16:56 ` Tom Tromey 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2023-02-15 16:38 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Simon Marchi On 2/14/23 15:42, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> Add a constructor that takes the name as a parameter. Initialize the > > This is truncated. Woops, it should have read: gdb: add constructor to internalvar Add a constructor that takes the name as a parameter. Initialize the next and kind fields inline. Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] gdb: add constructor to internalvar 2023-02-15 16:38 ` Simon Marchi @ 2023-02-15 16:56 ` Tom Tromey 2023-02-15 16:58 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Tom Tromey @ 2023-02-15 16:56 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Tom Tromey, Simon Marchi, Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> On 2/14/23 15:42, Tom Tromey wrote: >>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: >> Simon> Add a constructor that takes the name as a parameter. Initialize the >> >> This is truncated. Simon> Woops, it should have read: Simon> gdb: add constructor to internalvar Simon> Add a constructor that takes the name as a parameter. Initialize the Simon> next and kind fields inline. Thanks, this is fine by me. Tom ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] gdb: add constructor to internalvar 2023-02-15 16:56 ` Tom Tromey @ 2023-02-15 16:58 ` Simon Marchi 0 siblings, 0 replies; 9+ messages in thread From: Simon Marchi @ 2023-02-15 16:58 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Simon Marchi On 2/15/23 11:56, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> On 2/14/23 15:42, Tom Tromey wrote: >>>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: >>> > Simon> Add a constructor that takes the name as a parameter. Initialize the >>> >>> This is truncated. > > Simon> Woops, it should have read: > > Simon> gdb: add constructor to internalvar > > Simon> Add a constructor that takes the name as a parameter. Initialize the > Simon> next and kind fields inline. > > Thanks, this is fine by me. > > Tom Thanks, I will push the series now. Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] gdb: store internalvars in an std::map 2023-02-14 19:23 [PATCH v2 1/3] gdb: use std::string for internalvar::name Simon Marchi 2023-02-14 19:23 ` [PATCH v2 2/3] gdb: add constructor to internalvar Simon Marchi @ 2023-02-14 19:23 ` Simon Marchi 2023-02-14 20:46 ` Tom Tromey 2023-02-14 20:37 ` [PATCH v2 1/3] gdb: use std::string for internalvar::name Tom Tromey 2 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2023-02-14 19:23 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi In a test downstream in ROCgdb, we had a test case failing when GDB_REVERSE_INIT_FUNCTIONS was set. The test was assuming a particular order in the output of "show convenience". And the order changes when running with GDB_REVERSE_INIT_FUNCTIONS. I think that a nice way to fix it is to make the output of "show convenience" sorted, and therefore stable. Ideally, I think that the the user-visible behavior of GDB should not change when using GDB_REVERSE_INIT_FUNCTIONS. Plus, it makes the output of "show convenience" look nice, not that it's really important. Implement this by storing the internal vars in an std::map, which is a sorted container. Change-Id: I1fca7e7877cc984a3a3432c7639d45e68d437241 --- gdb/value.c | 55 +++++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/gdb/value.c b/gdb/value.c index 7fd11ba70db6..4db6cb4ed30d 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -41,6 +41,7 @@ #include "user-regs.h" #include <algorithm> #include <iterator> +#include <map> #include <utility> #include <vector> #include "completer.h" @@ -1833,7 +1834,6 @@ struct internalvar : name (std::move (name)) {} - struct internalvar *next = nullptr; std::string name; /* We support various different kinds of content of an internal variable. @@ -1845,7 +1845,10 @@ struct internalvar union internalvar_data u; }; -static struct internalvar *internalvars; +/* Use std::map, a sorted container, to make the order of iteration (and + therefore the output of "show convenience" stable). */ + +static std::map<std::string, internalvar> internalvars; /* If the variable does not already exist create it and give it the value given. If no value is given then the default is zero. */ @@ -1895,13 +1898,11 @@ init_if_undefined_command (const char* args, int from_tty) struct internalvar * lookup_only_internalvar (const char *name) { - struct internalvar *var; - - for (var = internalvars; var; var = var->next) - if (var->name == name) - return var; + auto it = internalvars.find (name); + if (it == internalvars.end ()) + return nullptr; - return NULL; + return &it->second; } /* Complete NAME by comparing it to the names of internal @@ -1910,27 +1911,29 @@ lookup_only_internalvar (const char *name) void complete_internalvar (completion_tracker &tracker, const char *name) { - struct internalvar *var; - int len; + int len = strlen (name); - len = strlen (name); + for (auto &pair : internalvars) + { + const internalvar &var = pair.second; - for (var = internalvars; var; var = var->next) - if (var->name.compare (0, len, name) == 0) - tracker.add_completion (make_unique_xstrdup (var->name.c_str ())); + if (var.name.compare (0, len, name) == 0) + tracker.add_completion (make_unique_xstrdup (var.name.c_str ())); + } } /* Create an internal variable with name NAME and with a void value. - NAME should not normally include a dollar sign. */ + NAME should not normally include a dollar sign. + + An internal variable with that name must not exist already. */ struct internalvar * create_internalvar (const char *name) { - internalvar *var = new internalvar (name); + auto pair = internalvars.emplace (std::make_pair (name, internalvar (name))); + gdb_assert (pair.second); - var->next = internalvars; - internalvars = var; - return var; + return &pair.first->second; } /* Create an internal variable with name NAME and register FUN as the @@ -2414,8 +2417,6 @@ preserve_one_varobj (struct varobj *varobj, struct objfile *objfile, void preserve_values (struct objfile *objfile) { - struct internalvar *var; - /* Create the hash table. We allocate on the objfile's obstack, since it is soon to be deleted. */ htab_up copied_types = create_copied_types_hash (); @@ -2423,8 +2424,8 @@ preserve_values (struct objfile *objfile) for (const value_ref_ptr &item : value_history) item->preserve (objfile, copied_types.get ()); - for (var = internalvars; var; var = var->next) - preserve_one_internalvar (var, objfile, copied_types.get ()); + for (auto &pair : internalvars) + preserve_one_internalvar (&pair.second, objfile, copied_types.get ()); /* For the remaining varobj, check that none has type owned by OBJFILE. */ all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj) @@ -2440,25 +2441,25 @@ static void show_convenience (const char *ignore, int from_tty) { struct gdbarch *gdbarch = get_current_arch (); - struct internalvar *var; int varseen = 0; struct value_print_options opts; get_user_print_options (&opts); - for (var = internalvars; var; var = var->next) + for (auto &pair : internalvars) { + internalvar &var = pair.second; if (!varseen) { varseen = 1; } - gdb_printf (("$%s = "), var->name.c_str ()); + gdb_printf (("$%s = "), var.name.c_str ()); try { struct value *val; - val = value_of_internalvar (gdbarch, var); + val = value_of_internalvar (gdbarch, &var); value_print (val, gdb_stdout, &opts); } catch (const gdb_exception_error &ex) -- 2.39.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] gdb: store internalvars in an std::map 2023-02-14 19:23 ` [PATCH v2 3/3] gdb: store internalvars in an std::map Simon Marchi @ 2023-02-14 20:46 ` Tom Tromey 0 siblings, 0 replies; 9+ messages in thread From: Tom Tromey @ 2023-02-14 20:46 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> In a test downstream in ROCgdb, we had a test case failing when Simon> GDB_REVERSE_INIT_FUNCTIONS was set. The test was assuming a particular Simon> order in the output of "show convenience". And the order changes when Simon> running with GDB_REVERSE_INIT_FUNCTIONS. Simon> I think that a nice way to fix it is to make the output of "show Simon> convenience" sorted, and therefore stable. Ideally, I think that the Simon> the user-visible behavior of GDB should not change when using Simon> GDB_REVERSE_INIT_FUNCTIONS. Plus, it makes the output of "show Simon> convenience" look nice, not that it's really important. Simon> Implement this by storing the internal vars in an std::map, which is a Simon> sorted container. Makes sense to me. Approved-By: Tom Tromey <tom@tromey.com> Tom ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] gdb: use std::string for internalvar::name 2023-02-14 19:23 [PATCH v2 1/3] gdb: use std::string for internalvar::name Simon Marchi 2023-02-14 19:23 ` [PATCH v2 2/3] gdb: add constructor to internalvar Simon Marchi 2023-02-14 19:23 ` [PATCH v2 3/3] gdb: store internalvars in an std::map Simon Marchi @ 2023-02-14 20:37 ` Tom Tromey 2 siblings, 0 replies; 9+ messages in thread From: Tom Tromey @ 2023-02-14 20:37 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> Change internalvar::name to std::string, automating memory management. Simon> It becomes necessary to allocate internalvar with new instead of XNEW. Looks good. Approved-By: Tom Tromey <tom@tromey.com> Simon> I didn't find how to trigger the code in complete_internalvar. It is Simon> called from condition_completer, so it should be by using the Simon> "condition" command, but I never managed to get in the right code path. I am not sure either but I believe there's a bug open to the effect that convenience variable completion does not work. Tom ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-02-15 16:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-14 19:23 [PATCH v2 1/3] gdb: use std::string for internalvar::name Simon Marchi 2023-02-14 19:23 ` [PATCH v2 2/3] gdb: add constructor to internalvar Simon Marchi 2023-02-14 20:42 ` Tom Tromey 2023-02-15 16:38 ` Simon Marchi 2023-02-15 16:56 ` Tom Tromey 2023-02-15 16:58 ` Simon Marchi 2023-02-14 19:23 ` [PATCH v2 3/3] gdb: store internalvars in an std::map Simon Marchi 2023-02-14 20:46 ` Tom Tromey 2023-02-14 20:37 ` [PATCH v2 1/3] gdb: use std::string for internalvar::name Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).