Re: [PATCH] Add name_of_main and language_of_main to the DWARF index

[Tom Tromey <tom@tromey.com>]

>>>>> Matheus Branco Borella via Gdb-patches <gdb-patches@sourceware.org> writes:

Hi.  Thank you for the patch.  I think it's a good addition and is
fundamentally fine.  There are a few nits below.

For a patch of this size, I think we will need a copyright assignment.
The form is here:

http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future

Normally this is a pretty quick process.

> This patch adds a new section to the DWARF index containing the name
> and the language of the main function symbol, gathered from
> `cooked_index::get_main`, if available. Currently, for lack of a better name,
> this section is called the "shortcut table" (suggestions for a better name are
> appreciated).

Seems fine to me :)

> In my testing (a binary with about 1.8GB worth of DWARF data) this change brings
> startup time down from about 34 seconds to about 1.5 seconds.

Very nice.

> (I feel like I might've changed too much about the index format by adding a 
> breaking change. If there's a better way to do this it'd be glad to hear about 
> it.)

The index format is documented in gdb/doc/gdb.texinfo.  So, your patch
will at least need an update to that file.  I suspect a brief entry in
gdb/NEWS describing the change would also be good.

There's a note on the compatibility issue inline, below.

>    /* The version number.  */
> -  contents.append_offset (8);
> +  contents.append_offset (9);

Someday we should probably use a #define for this.
Not your problem though.

> +/* Write shortcut information. */
> +
> +static void
> +write_shortcuts_table (cooked_index *table, data_buf& shortcuts,

gdb style puts the "&" next to "shortcuts".
There's a few of these I think.

> +      lang = main_info->per_cu->lang ();
...
> +  shortcuts.append_uint (4, BFD_ENDIAN_LITTLE, lang);

I think it would be better to use a DW_LANG_ constant here -- it's
better not to expose gdb's enum language values to the world, whereas
the DWARF values are already fixed.

Those aren't really preserved exactly in the reader, but mapping back
from the gdb language to DWARF would be fine (probably).

> +  ++i;
> +
> +  const gdb_byte *shortcut_table = addr + metadata[i];
> +  const gdb_byte *shortcut_table_end = addr + metadata[i + 1];
> +  map->shortcut_table
> +    = gdb::array_view<const gdb_byte> (shortcut_table, shortcut_table_end);
 
This section probably has to be conditional on 'version >= 8.
Otherwise a new gdb will fail with an older version of the index.

> +/* Sets the name and language of the main function from the shortcut table. */
> +
> +static void
> +set_main_name_from_gdb_index (dwarf2_per_objfile *per_objfile, 
> +                              mapped_gdb_index *index)
> +{
> +  auto ptr = index->shortcut_table.data ();
> +  const auto lang = extract_unsigned_integer (ptr, 4, BFD_ENDIAN_LITTLE);

This code should probably check the size of the data, both for safety
and also because that's a decent way to handle the index version issue.

Tom