I found a small bug coming from a couple of recent patches of mine for
cooked_index_entry::full_name.
First, commit aab26529b30 (Add "Ada linkage" mode to
cooked_index_entry::full_name) added a small hack to optionally
compute the Ada linkage name.
Then, commit aab2ac34d7f (Avoid excessive CU expansion on failed
matches) changed the relevant expand_symtabs_matching implementation
to use this feature.
However, the feature was used unconditionally, causing a bad side
effect: the non-canonical name is now used for all languages, not just
Ada. But, for C++ this is wrong.
Furthermore, consider the declaration of full_name:
const char *full_name (struct obstack *storage,
bool for_main = false,
bool for_ada_linkage = false,
const char *default_sep = nullptr) const;
... and then consider this call in cooked_index::dump:
gdb_printf (" qualified: %s\n",
entry->full_name (&temp_storage, false, "::"));
Oops! The "::" is silently converted to 'true' here.
To fix both of these problems, this patch changes full_name to accept
a flags enum rather than booleans. This avoids the type-safety
problem.
Then, full_name is changed to remove the "Ada" flag when the entry is
not in fact an Ada symbol.
Regression tested on x86-64 Fedora 40.
---
gdb/dwarf2/cooked-index.c | 32 +++++++++++++++++------------
gdb/dwarf2/cooked-index.h | 42 ++++++++++++++++++++++++++-------------
gdb/dwarf2/index-write.c | 2 +-
gdb/dwarf2/parent-map.c | 2 +-
gdb/dwarf2/read.c | 2 +-
5 files changed, 50 insertions(+), 30 deletions(-)
On 3/10/25 10:58 AM, Tom Tromey wrote:
> I found a small bug coming from a couple of recent patches of mine for
> cooked_index_entry::full_name.
>
> First, commit aab26529b30 (Add "Ada linkage" mode to
> cooked_index_entry::full_name) added a small hack to optionally
> compute the Ada linkage name.
>
> Then, commit aab2ac34d7f (Avoid excessive CU expansion on failed
> matches) changed the relevant expand_symtabs_matching implementation
> to use this feature.
>
> However, the feature was used unconditionally, causing a bad side
> effect: the non-canonical name is now used for all languages, not just
> Ada. But, for C++ this is wrong.
>
> Furthermore, consider the declaration of full_name:
>
> const char *full_name (struct obstack *storage,
> bool for_main = false,
> bool for_ada_linkage = false,
> const char *default_sep = nullptr) const;
>
> ... and then consider this call in cooked_index::dump:
>
> gdb_printf (" qualified: %s\n",
> entry->full_name (&temp_storage, false, "::"));
>
> Oops! The "::" is silently converted to 'true' here.
>
> To fix both of these problems, this patch changes full_name to accept
> a flags enum rather than booleans. This avoids the type-safety
> problem.
>
> Then, full_name is changed to remove the "Ada" flag when the entry is
> not in fact an Ada symbol.
>
> Regression tested on x86-64 Fedora 40.
I think it's a good idea.
> @@ -139,18 +150,22 @@ struct cooked_index_entry : public allocate_on_obstack<cooked_index_entry>
>
> /* Construct the fully-qualified name of this entry and return a
> pointer to it. If allocation is needed, it will be done on
> - STORAGE. FOR_MAIN is true if we are computing the name of the
> - "main" entry -- one marked DW_AT_main_subprogram. This matters
> - for avoiding name canonicalization and also a related race (if
> - "main" computation is done during finalization). If
> - FOR_ADA_LINKAGE is true, then Ada-language symbols will have
> - their "linkage-style" name computed. The default is
> - source-style. If the language
> - doesn't prescribe a separator, one can be specified using
> - DEFAULT_SEP. */
> + STORAGE.
> +
> + FLAGS affects the result. If the FOR_MAIN flag is set, we are
> + computing the name of the "main" entry -- one marked
> + DW_AT_main_subprogram. This matters for avoiding name
> + canonicalization and also a related race (if "main" computation
> + is done during finalization).
> +
> + If the FOR_ADA_LINKAGE flag is set, then Ada-language symbols
The flag is just "FOR_ADA".
However, would you be able to find a more descriptive name for this
flag, without it being too long? Something like
"ADA_USE_WHATEVER_NAME", where you replace WHATEVER with an appropriate
word? If not, it's not the end of the world, but I just find that
"FOR_ADA" doesn't really give a clue about how the flag changes the
behavior of the function.
Simon
@@ -221,11 +221,11 @@ cooked_index_entry::matches (domain_search_flags kind) const
/* See cooked-index.h. */
const char *
-cooked_index_entry::full_name (struct obstack *storage, bool for_main,
- bool for_ada_linkage,
+cooked_index_entry::full_name (struct obstack *storage,
+ cooked_index_full_name_flag name_flags,
const char *default_sep) const
{
- const char *local_name = for_main ? name : canonical;
+ const char *local_name = ((name_flags & FOR_MAIN) != 0) ? name : canonical;
if ((flags & IS_LINKAGE) != 0 || get_parent () == nullptr)
return local_name;
@@ -240,7 +240,7 @@ cooked_index_entry::full_name (struct obstack *storage, bool for_main,
break;
case language_ada:
- if (for_ada_linkage)
+ if ((name_flags & FOR_ADA) != 0)
{
sep = "__";
break;
@@ -257,7 +257,12 @@ cooked_index_entry::full_name (struct obstack *storage, bool for_main,
break;
}
- get_parent ()->write_scope (storage, sep, for_main, for_ada_linkage);
+ /* The FOR_ADA flag should only affect Ada entries, so disable it
+ here if we don't need it. */
+ if (lang != language_ada)
+ name_flags &= ~FOR_ADA;
+
+ get_parent ()->write_scope (storage, sep, name_flags);
obstack_grow0 (storage, local_name, strlen (local_name));
return (const char *) obstack_finish (storage);
}
@@ -267,14 +272,15 @@ cooked_index_entry::full_name (struct obstack *storage, bool for_main,
void
cooked_index_entry::write_scope (struct obstack *storage,
const char *sep,
- bool for_main,
- bool for_ada_linkage) const
+ cooked_index_full_name_flag flags) const
{
if (get_parent () != nullptr)
- get_parent ()->write_scope (storage, sep, for_main, for_ada_linkage);
+ get_parent ()->write_scope (storage, sep, flags);
/* When computing the Ada linkage name, the entry might not have
been canonicalized yet, so use the 'name'. */
- const char *local_name = (for_main || for_ada_linkage) ? name : canonical;
+ const char *local_name = ((flags & (FOR_MAIN | FOR_ADA)) != 0
+ ? name
+ : canonical);
obstack_grow (storage, local_name, strlen (local_name));
obstack_grow (storage, sep, strlen (sep));
}
@@ -471,8 +477,8 @@ cooked_index_shard::finalize (const parent_map_map *parent_maps)
code in cooked_index_entry::full_name). */
if (entry->get_parent () != nullptr)
{
- const char *fullname = entry->full_name (&m_storage, false,
- true);
+ const char *fullname = entry->full_name (&m_storage,
+ FOR_ADA);
cooked_index_entry *linkage = create (entry->die_offset,
entry->tag,
(entry->flags
@@ -834,7 +840,7 @@ cooked_index::get_main_name (struct obstack *obstack, enum language *lang)
return nullptr;
*lang = entry->lang;
- return entry->full_name (obstack, true);
+ return entry->full_name (obstack, FOR_MAIN);
}
/* See cooked_index.h. */
@@ -902,7 +908,7 @@ cooked_index::dump (gdbarch *arch)
gdb_printf (" name: %s\n", entry->name);
gdb_printf (" canonical: %s\n", entry->canonical);
gdb_printf (" qualified: %s\n",
- entry->full_name (&temp_storage, false, "::"));
+ entry->full_name (&temp_storage, 0, "::"));
gdb_printf (" DWARF tag: %s\n", dwarf_tag_name (entry->tag));
gdb_printf (" flags: %s\n", to_string (entry->flags).c_str ());
gdb_printf (" DIE offset: %s\n", sect_offset_str (entry->die_offset));
@@ -65,6 +65,17 @@ enum cooked_index_flag_enum : unsigned char
};
DEF_ENUM_FLAGS_TYPE (enum cooked_index_flag_enum, cooked_index_flag);
+/* Flags used when requesting the full name of an entry. */
+enum cooked_index_full_name_enum : unsigned char
+{
+ /* Set when requesting the name of "main". See the method for the
+ full description. */
+ FOR_MAIN = 1,
+ /* Set when requesting the linkage name for an Ada entry. */
+ FOR_ADA = 2,
+};
+DEF_ENUM_FLAGS_TYPE (enum cooked_index_full_name_enum, cooked_index_full_name_flag);
+
/* Type representing either a resolved or deferred cooked_index_entry. */
union cooked_index_entry_ref
@@ -139,18 +150,22 @@ struct cooked_index_entry : public allocate_on_obstack<cooked_index_entry>
/* Construct the fully-qualified name of this entry and return a
pointer to it. If allocation is needed, it will be done on
- STORAGE. FOR_MAIN is true if we are computing the name of the
- "main" entry -- one marked DW_AT_main_subprogram. This matters
- for avoiding name canonicalization and also a related race (if
- "main" computation is done during finalization). If
- FOR_ADA_LINKAGE is true, then Ada-language symbols will have
- their "linkage-style" name computed. The default is
- source-style. If the language
- doesn't prescribe a separator, one can be specified using
- DEFAULT_SEP. */
+ STORAGE.
+
+ FLAGS affects the result. If the FOR_MAIN flag is set, we are
+ computing the name of the "main" entry -- one marked
+ DW_AT_main_subprogram. This matters for avoiding name
+ canonicalization and also a related race (if "main" computation
+ is done during finalization).
+
+ If the FOR_ADA_LINKAGE flag is set, then Ada-language symbols
+ will have their "linkage-style" name computed. The default is
+ source-style.
+
+ If the language doesn't prescribe a separator, one can be
+ specified using DEFAULT_SEP. */
const char *full_name (struct obstack *storage,
- bool for_main = false,
- bool for_ada_linkage = false,
+ cooked_index_full_name_flag name_flags = 0,
const char *default_sep = nullptr) const;
/* Comparison modes for the 'compare' function. See the function
@@ -255,10 +270,9 @@ struct cooked_index_entry : public allocate_on_obstack<cooked_index_entry>
/* A helper method for full_name. Emits the full scope of this
object, followed by the separator, to STORAGE. If this entry has
a parent, its write_scope method is called first. See full_name
- for a description of the FOR_MAIN and FOR_ADA_LINKAGE
- parameters. */
+ for a description of the FLAGS parameter. */
void write_scope (struct obstack *storage, const char *sep,
- bool for_main, bool for_ada_linkage) const;
+ cooked_index_full_name_flag flags) const;
/* The parent entry. This is NULL for top-level entries.
Otherwise, it points to the parent entry, such as a namespace or
@@ -1280,7 +1280,7 @@ write_shortcuts_table (cooked_index *table, data_buf &shortcuts,
if (dw_lang != 0)
{
auto_obstack obstack;
- const auto main_name = main_info->full_name (&obstack, true);
+ const auto main_name = main_info->full_name (&obstack, FOR_MAIN);
main_name_offset = cpool.size ();
cpool.append_cstr0 (main_name);
@@ -60,7 +60,7 @@ dump_parent_map (dwarf2_per_bfd *per_bfd, const struct addrmap *map)
gdb_printf (outfile, " -> (0x%" PRIx64 ": %s)",
to_underlying (parent_entry->die_offset),
- parent_entry->full_name (&temp_storage, false));
+ parent_entry->full_name (&temp_storage));
};
addrmap_dump (const_cast<addrmap *> (map), gdb_stdlog, nullptr,
@@ -14942,7 +14942,7 @@ cooked_index_functions::expand_symtabs_matching
{
auto_obstack temp_storage;
const char *full_name = entry->full_name (&temp_storage,
- false, true);
+ FOR_ADA);
if (symbol_matcher == nullptr)
{
symbol_name_matcher_ftype *name_matcher