Re: [PATCH 1/2] [gdb/symtab] Add name_of_main and language_of_main to the DWARF index

[Tom de Vries <tdevries@suse.de>]

On 10/10/23 21:19, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> From: Matheus Branco Borella <dark.ryu.550@gmail.com>
> Tom> This patch adds a new section to the DWARF index containing the name
> Tom> and the language of the main function symbol, gathered from
> Tom> `cooked_index::get_main`, if available.
> 
> This patch has a bunch of formatting nits.  I think it's also a little
> incorrect in its handling of unknown languages / its understanding of
> its own idea of how the "0" case is handled.
> 
> Tom> +@item The shortcut table
> Tom> +This is a data structure with the following fields:
> Tom> +
> Tom> +@table @asis
> Tom> +@item Language of main
> Tom> +An @code{offset_type} value indicating the language of the main function as a
> Tom> +@code{DW_LANG_} constant.  This value will be zero if main function information
> Tom> +is not present.
> Tom> +
> Tom> +@item Name of main
> Tom> +An @code{offset_type} value indicating the offset of the main function's name
> Tom> +in the constant pool.  This value must be ignored if the value for the language
> Tom> +of main is zero.
> 
> This phrasing is a little strange.  The index-writing code seems to omit
> the name field if the language is unknown.  However, the text here makes
> it sound like the field is present but must be ignored.
> 
> If the writing code is correct, I would suggest changing this to say
> "This field is not present if..."
> 
> Tom> +  dwarf_source_language dw_lang = (dwarf_source_language)0;
> 
> gdb style puts a space after the ")" of a cast.
> There's a lot of cases like this.
> 

Done.

> Tom> +  shortcuts.append_uint (4, BFD_ENDIAN_LITTLE, dw_lang);
> Tom> +  shortcuts.append_offset (main_name_offset);
> 
> The first ljne using append_uint seems confusing... it made me wonder
> why it isn't using offset_type.  But in the end I don't think there's
> any reason.  "offset_type" is used for all kinds of fields in the index,
> it's better to just use that here.  It is 4 bytes anyway.
> 

Done.

> Tom> +/* Sets the name and language of the main function from the shortcut table. */
> Tom> +
> Tom> +static void
> Tom> +set_main_name_from_gdb_index (dwarf2_per_objfile *per_objfile,
> Tom> +			      mapped_gdb_index *index)
> Tom> +{
> Tom> +  const auto expected_size = 4 + sizeof (offset_type);
> 
> Better to use 2 * sizeof (offset_type) IMO.
> 

Done.

> Tom> +  if (index->shortcut_table.size () < expected_size)
> Tom> +    /* The data in the section is not present, is corrupted or is in a version
> Tom> +     * we don't know about. Regardless, we can't make use of it. */
> Tom> +    return;
> 
> The leading "*" on comments is not gdb style.  Several cases of this.
> 

Done.

> This is where the reader seems to expect that both members are always
> written.
> 
> Tom> +
> Tom> +  auto ptr = index->shortcut_table.data ();
> Tom> +  const auto dw_lang = extract_unsigned_integer (ptr, 4, BFD_ENDIAN_LITTLE);
> Tom> +  if (dw_lang >= DW_LANG_hi_user)
> Tom> +    {
> Tom> +      complaint (_(".gdb_index shortcut table has invalid main language %u"),
> Tom> +		   (unsigned) dw_lang);
> Tom> +      return;
> Tom> +    }
> 
> IMO it would be better for this check to happen in
> dwarf_lang_to_enum_language.  Suppose gdb adds support for a new language.
> That version of gdb may still emit index version 9, but the index
> won't be directly usable by an older gdb.
> 

AFAIU the check is the usual: is the value we've just read in the valid 
range.  It makes no assumption about which language gdb supports.  So 
I'm not sure this needs to be moved.

> Tom> +  const auto name = (const char*) (index->constant_pool.data () + name_offset);
> 
> Space before "*".  There were some with "&" as well but I didn't
> remember to point them out before deleting the patch text.
> 

Done.  Submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/203161.html ).

Thanks,
- Tom