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

[Tom Tromey <tom@tromey.com>]

>>>>> "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.

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.

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.

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.

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.

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.

Tom