On Thu, Mar 19, 2020 at 8:46 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Thu, Mar 19, 2020 at 4:00 PM Martin Liška <mli...@suse.cz> wrote:
> >
> > On 3/19/20 10:12 AM, Richard Biener wrote:
> > > On Wed, Mar 18, 2020 at 9:52 AM Martin Liška <mli...@suse.cz> wrote:
> > >>
> > >> On 3/18/20 12:27 AM, Jan Hubicka wrote:
> > >>>> Hi.
> > >>>>
> > >>>> There's updated version of the patch.
> > >>>> Changes from the previous version:
> > >>>> - comment added to ld_plugin_symbol
> > >>>> - new section renamed to ext_symtab
> > >>>> - assert added for loop iterations in produce_symtab and
> > >>>> produce_symtab_extension
> > >>> Hi,
> > >>> I hope this is last version of the patch.
> > >>
> > >> Hello.
> > >>
> > >> Yes.
> > >>
> > >>>>
> > >>>> 2020-03-12 Martin Liska <mli...@suse.cz>
> > >>>>
> > >>>> * lto-section-in.c: Add extension_symtab.
> > >>> ext_symtab :)
> > >>
> > >> Fixed.
> > >>
> > >>>> diff --git a/gcc/lto-section-in.c b/gcc/lto-section-in.c
> > >>>> index c17dd69dbdd..78b015be696 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",
> > >>>> + "ext_symtab"
> > >>> I would move ext_symtab next to symtab so the sections remains at least
> > >>> bit reasonably ordered.
> > >>
> > >> Ok, I'll adjust it and I will send a separate patch where we bump
> > >> LTO_major_version.
> > >>
> > >>>>
> > >>>> +/* Write extension information for symbols (symbol type, section
> > >>>> flags). */
> > >>>> +
> > >>>> +static void
> > >>>> +write_symbol_extension_info (tree t)
> > >>>> +{
> > >>>> + unsigned char c;
> > >>> Do we still use vertical whitespace after decls per GNU coding style?
> > >>
> > >> Dunno. This seems to me like a nit.
> > >>
> > >>>> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
> > >>>> index 25bf6c468f7..4f82b439360 100644
> > >>>> --- a/gcc/lto-streamer.h
> > >>>> +++ b/gcc/lto-streamer.h
> > >>>> @@ -236,6 +236,7 @@ enum lto_section_type
> > >>>> LTO_section_ipa_hsa,
> > >>>> LTO_section_lto,
> > >>>> LTO_section_ipa_sra,
> > >>>> + LTO_section_symtab_extension,
> > >>> I guess symtab_ext to match the actual section name?
> > >>
> > >> No. See e.g. LTO_section_jump_functions - "jmpfuncs". We want to have
> > >> more descriptive
> > >> enum names.
> > >>
> > >>>> LTO_N_SECTION_TYPES /* Must be last. */
> > >>>> };
> > >>>>
> > >>>> diff --git a/include/lto-symtab.h b/include/lto-symtab.h
> > >>>> index 0ce0de10121..47f0ff27df8 100644
> > >>>> --- a/include/lto-symtab.h
> > >>>> +++ b/include/lto-symtab.h
> > >>>> @@ -38,4 +38,16 @@ enum gcc_plugin_symbol_visibility
> > >>>> GCCPV_HIDDEN
> > >>>> };
> > >>>>
> > >>>> +enum gcc_plugin_symbol_type
> > >>>> +{
> > >>>> + GCCST_UNKNOWN,
> > >>>> + GCCST_FUNCTION,
> > >>>> + GCCST_VARIABLE,
> > >>>> +};
> > >>>> +
> > >>>> +enum gcc_plugin_symbol_section_flags
> > >>>> +{
> > >>>> + GCCSSS_BSS = 1
> > >>>> +};
> > >>>
> > >>> Probably comments here?
> > >>
> > >> No. There are just shadow copy of enum types from plugin-api.h which
> > >> are documented.
> > >>
> > >>>> +
> > >>>> #endif /* GCC_LTO_SYMTAB_H */
> > >>>> +/* Parse an entry of the IL symbol table. The data to be parsed is
> > >>>> pointed
> > >>>> + by P and the result is written in ENTRY. The slot number is stored
> > >>>> in SLOT.
> > >>>> + Returns the address of the next entry. */
> > >>>> +
> > >>>> +static char *
> > >>>> +parse_table_entry_extension (char *p, struct ld_plugin_symbol *entry)
> > >>>> +{
> > >>>> + unsigned char t;
> > >>>> + enum ld_plugin_symbol_type symbol_types[] =
> > >>>> + {
> > >>>> + LDST_UNKNOWN,
> > >>>> + LDST_FUNCTION,
> > >>>> + LDST_VARIABLE,
> > >>>> + };
> > >>>> +
> > >>>> + t = *p;
> > >>>> + check (t <= 3, LDPL_FATAL, "invalid symbol type found");
> > >>>> + entry->symbol_type = symbol_types[t];
> > >>>> + p++;
> > >>>> + entry->section_flags = *p;
> > >>>> + p++;
> > >>>> +
> > >>>> + return p;
> > >>>> +}
> > >>>
> > >>> I think we have chance to make some plan for future extensions without
> > >>> introducing too many additional sections.
> > >>>
> > >>> Currently there are 2 bytes per entry, while only 3 bits are actively
> > >>> used of them. If we invent next flag to pass we can use unused bits
> > >>> however we need a way to indicate to plugin that the bit is defined.
> > >>> This could be done by a simple version byte at the beggining of
> > >>> ext_symtab section which will be 0 now and once we define extra bits we
> > >>> bump it up to 1.
> > >>
> > >> I like the suggested change, it can help us in the future.
> > >>
> > >>>
> > >>> It is not that important given that even empty file results in 2k LTO
> > >>> object file, but I think it would be nicer in longer run.
> > >>>> + /* This is for compatibility with older ABIs. */
> > >>> Perhaps say here that this ABI defined only "int def;"
> > >>
> > >> Good point.
> > >>
> > >>>
> > >>> The patch look good to me. Thanks for the work!
> > >>
> > >> Thanks. I'm sending updated patch that I've just tested on lto.exp and
> > >> both binutils master and HJ's branch that utilizes the new API.
> > >
> > > @@ -495,10 +560,16 @@ write_resolution (void)
> > >
> > > /* Version 2 of API supports IRONLY_EXP resolution that is
> > > accepted by GCC-4.7 and newer. */
> > > - if (get_symbols_v2)
> > > - get_symbols_v2 (info->handle, symtab->nsyms, syms);
> > > + if (get_symbols_v4)
> > > + get_symbols_v4 (info->handle, symtab->nsyms, syms);
> > > else
> > > - get_symbols (info->handle, symtab->nsyms, syms);
> > > + {
> > > + clear_new_symtab_flags (symtab);
> > >
> > > can you instead just avoid parsing the ext symtab?
> >
> > Yes, I simplified the changes and I bet we'll only need one new hook
> > get_symbols_v2.
> > Then we can base parsing of the external symtab on that.
> >
> > >
> > > + if (get_symbols_v2)
> > > + get_symbols_v2 (info->handle, symtab->nsyms, syms);
> > > + else
> > > + get_symbols (info->handle, symtab->nsyms, syms);
> > > + }
> > >
> > > I guess this also points to the fact that LDs symbol resolution
> > > can't tell GCC it chose "BSS" (from a non-IL object) or that
> > > it chose a variable or function.
> > >
> > > @@ -296,6 +300,8 @@ parse_table_entry (char *p, struct ld_plugin_symbol
> > > *entry,
> > > entry->visibility = translate_visibility[t];
> > > p++;
> > >
> > > + entry->unused = 0;
> > > +
> > > memcpy (&entry->size, p, sizeof (uint64_t));
> > > p += 8;
> > >
> > > isn't that either not enough or too much clearing?
> > > I'd have expected
> > >
> > > entry->unused = entry->section_flags = entry->symbol_type = 0;
> > >
> > > _before_
> > >
> > > t = *p;
> > > check (t <= 4, LDPL_FATAL, "invalid symbol kind found");
> > > entry->def = translate_kind[t];
> > > p++;
> > >
> > > ?
> >
> > Yes, that's better.
> >
> > >
> > > +enum ld_plugin_symbol_section_flags
> > > +{
> > > + LDSSS_BSS = 1
> > > +};
> > > +
> > >
> > > please add a symbolic name for the value zero,
> > > may I suggest LDSSS_DEFAULT? I see you've
> > > settled on symbol_section_flags rather than
> > > section_kind (BSS is not really a flag...).
> >
> > I renamed that to ld_plugin_symbol_section_kind.
> >
> > May I install the 2 patches now? It's again tested on lto.exp
> > and both old binutils and new HJ's binutils patch and it works as expected.
>
> OK if HJ is happy, lto-symtab.h is owned by binutils I think and existing
> users might need to be adjusted to clear the unused fields now that the
> width of 'def' changed.
>
> Richard.
I like it and I will take case of binutils side.
Thanks.
--
H.J.
