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  <mliska@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.

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