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?
+ 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++;
?
+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...).
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
> >>
> >
>
