On Thu, Mar 12, 2020 at 2:11 PM Martin Liška <mli...@suse.cz> wrote:
>
> On 3/12/20 1:55 PM, Jan Hubicka wrote:
> >> gcc/ChangeLog:
> >>
> >> 2020-03-12 Martin Liska <mli...@suse.cz>
> >>
> >> * lto-section-in.c: Add extension_symtab.
> >> * lto-streamer-out.c (write_symbol_extension_info):
> >> New.
> >> (produce_symtab_extension): New.
> >> (produce_asm_for_decls): Stream also produce_symtab_extension.
> >> * lto-streamer.h (enum lto_section_type): New section.
> >>
> >> include/ChangeLog:
> >>
> >> 2020-03-12 Martin Liska <mli...@suse.cz>
> >>
> >> * lto-symtab.h (enum gcc_plugin_symbol_type): New.
> >> (enum gcc_plugin_symbol_section_flags): Likewise.
> >>
> >> lto-plugin/ChangeLog:
> >>
> >> 2020-03-12 Martin Liska <mli...@suse.cz>
> >>
> >> * lto-plugin.c (LTO_SECTION_PREFIX): Rename to
> >> ...
> >> (LTO_SYMTAB_PREFIX): ... this.
> >> (LTO_SECTION_PREFIX_LEN): Rename to ...
> >> (LTO_SYMTAB_PREFIX_LEN): ... this.
> >> (LTO_SYMTAB_EXT_PREFIX): New.
> >> (LTO_SYMTAB_EXT_PREFIX_LEN): New.
> >> (LTO_LTO_PREFIX): New.
> >> (LTO_LTO_PREFIX_LEN): New.
> >> (parse_table_entry): Fill up unused to zero.
> >> (parse_table_entry_extension): New.
> >> (parse_symtab_extension): New.
> >> (finish_conflict_resolution): Change type
> >> for resolution.
> >> (clear_new_symtab_flags): New.
> >> (write_resolution): Support new get_symbols_v4.
> >> (process_symtab): Use new macro name.
> >> (process_symtab_extension): New.
> >> (claim_file_handler): Parse also process_symtab_extension.
> >> (onload): Call new add_symbols_v2.
> > Hi,
> > thanks for updating the patch. Two minor nits
> >> ---
> >> gcc/lto-section-in.c | 3 +-
> >> gcc/lto-streamer-out.c | 64 +++++++++++++++-
> >> gcc/lto-streamer.h | 1 +
> >> include/lto-symtab.h | 12 +++
> >> lto-plugin/lto-plugin.c | 165 +++++++++++++++++++++++++++++++++++-----
> >> 5 files changed, 226 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/gcc/lto-section-in.c b/gcc/lto-section-in.c
> >> index c17dd69dbdd..7bf59c513fc 100644
> >> --- a/gcc/lto-section-in.c
> >> +++ b/gcc/lto-section-in.c
> >> @@ -54,7 +54,8 @@ const char *lto_section_name[LTO_N_SECTION_TYPES] =
> >> "mode_table",
> >> "hsa",
> >> "lto",
> >> - "ipa_sra"
> >> + "ipa_sra",
> >> + "extension_symtab"
> > I would call it symtab_ext so alphabetically it is coupled with symtab.
Maybe ext_symtab then, proper enlish for the long form would be
extended_symtab I guess.
> It's problematic because of this collision where we search for prefix:
>
> if (strncmp (name, LTO_SYMTAB_PREFIX, LTO_SYMTAB_PREFIX_LEN) != 0)
>
> Note that the section names have suffix:
> .gnu.lto_.symtab.48f1e54b1c048b0f
>
> > I wonder if moving it up in the enum would make it physically appear
> > next to .symtab in object file so we save a bit of i/o?
>
> It's not about names order, but streaming order. Which should be fine:
>
> $ readelf -SW /tmp/bss.o
> ...
> [10] .gnu.lto_.decls.48f1e54b1c048b0f PROGBITS 0000000000000000
> 0001dd 0002de 00 E 0 0 1
> [11] .gnu.lto_.symtab.48f1e54b1c048b0f PROGBITS 0000000000000000
> 0004bb 000049 00 E 0 0 1
> [12] .gnu.lto_.extension_symtab.48f1e54b1c048b0f PROGBITS
> 0000000000000000 000504 000006 00 E 0 0 1
> [13] .gnu.lto_.opts PROGBITS 0000000000000000 00050a 000051 00
> E 0 0 1
> ...
>
>
> >> +static void
> >> +produce_symtab_extension (struct output_block *ob)
> >> +{
> >> + char *section_name = lto_get_section_name (LTO_section_symtab_extension,
> >> + NULL, 0, NULL);
> >> + lto_symtab_encoder_t encoder = ob->decl_state->symtab_node_encoder;
> >> + lto_symtab_encoder_iterator lsei;
> >> +
> >> + lto_begin_section (section_name, false);
> >> + free (section_name);
> >> +
> >> + /* Write the symbol table.
> >> + First write everything defined and then all declarations.
> >> + This is necessary to handle cases where we have duplicated symbols.
> >> */
> >> + for (lsei = lsei_start (encoder);
> >> + !lsei_end_p (lsei); lsei_next (&lsei))
> >> + {
> >> + symtab_node *node = lsei_node (lsei);
> >> +
> >> + if (DECL_EXTERNAL (node->decl) ||
> >> !node->output_to_lto_symbol_table_p ())
> >> + continue;
> >> + write_symbol_extension_info (node->decl);
> >> + }
> >> + for (lsei = lsei_start (encoder);
> >> + !lsei_end_p (lsei); lsei_next (&lsei))
> >> + {
> >> + symtab_node *node = lsei_node (lsei);
> >> +
> >> + if (!DECL_EXTERNAL (node->decl) ||
> >> !node->output_to_lto_symbol_table_p ())
> >> + continue;
> >> + write_symbol_extension_info (node->decl);
> >> + }
> >> +
> >> + lto_end_section ();
> >> +}
> >
> > It seems fragile to me to duplicate the loops that needs to be kept in
> > sync because breaking that will likely get bit suprising outcome (i.e.
> > bootstrap will work since we do not care about symbol types). Perhaps it
> > would be more robust to simply push the extension bits into a vector in
> > write_symbol for later streaming. Or at least add comment that loops
> > needs to be kept in sync.
>
> I'll add comment.
>
> >> diff --git a/include/plugin-api.h b/include/plugin-api.h
> >> index 09e1202df07..804684449cf 100644
> >> --- a/include/plugin-api.h
> >> +++ b/include/plugin-api.h
> >> @@ -87,7 +87,17 @@ struct ld_plugin_symbol
> >> {
> >> char *name;
> >> char *version;
> >> - int def;
> >> +#ifdef __BIG_ENDIAN__
> >> + char unused;
> >> + char section_flags;
> >> + char symbol_type;
> >> + char def;
> >> +#else
#elif defined __LITTLE_ENDIAN__
> >> + char def;
> >> + char symbol_type;
> >> + char section_flags;
> >> + char unused;
#else
#error ...
#endif
better be safe than sorry ;)
> >> +#endif
> > Perhaps at least drop a comment why this is done this way (i.e.
> > compatibility with V3 version of API) so in 10 years we know?
>
> Likewise here.
>
> >
> > Bit more systematic way would be to add a new hook to query extended
> > properties of a given symbol. I.e. something like
> > void *get_symbol_property (symbol, enum property)
>
> I know, but it would complicate code on both consumer and LTO plugin side.
>
> Martin
>
> >
> > Honza
> >
>
