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 > >> > > >