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.
> Martin
>
> >
> > Otherwise looks OK to me.
> >
> > Thanks,
> > Richard.
> >
> >> Martin
> >>
> >>> Honza
> >>>> +#ifdef __BIG_ENDIAN__
> >>>> + char unused;
> >>>> + char section_flags;
> >>>> + char symbol_type;
> >>>> + char def;
> >>>> +#else
> >>>> + char def;
> >>>> + char symbol_type;
> >>>> + char section_flags;
> >>>> + char unused;
> >>>> +#endif
> >>>> int visibility;
> >>>> uint64_t size;
> >>>> char *comdat_key;
> >>>> @@ -123,6 +134,20 @@ enum ld_plugin_symbol_visibility
> >>>> LDPV_HIDDEN
> >>>> };
> >>>>
> >>>> +/* The type of the symbol. */
> >>>> +
> >>>> +enum ld_plugin_symbol_type
> >>>> +{
> >>>> + LDST_UNKNOWN,
> >>>> + LDST_FUNCTION,
> >>>> + LDST_VARIABLE,
> >>>> +};
> >>>> +
> >>>> +enum ld_plugin_symbol_section_flags
> >>>> +{
> >>>> + LDSSS_BSS = 1
> >>>> +};
> >>>> +
> >>>> /* How a symbol is resolved. */
> >>>>
> >>>> enum ld_plugin_symbol_resolution
> >>>> @@ -431,7 +456,9 @@ enum ld_plugin_tag
> >>>> LDPT_GET_INPUT_SECTION_ALIGNMENT = 29,
> >>>> LDPT_GET_INPUT_SECTION_SIZE = 30,
> >>>> LDPT_REGISTER_NEW_INPUT_HOOK = 31,
> >>>> - LDPT_GET_WRAP_SYMBOLS = 32
> >>>> + LDPT_GET_WRAP_SYMBOLS = 32,
> >>>> + LDPT_ADD_SYMBOLS_V2 = 33,
> >>>> + LDPT_GET_SYMBOLS_V4 = 34,
> >>>> };
> >>>>
> >>>> /* The plugin transfer vector. */
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>
> >>
>
