Re: [PATCH][RFC] API extension for binutils (type of symbols).

[Martin Liška]

On 3/12/20 1:55 PM, Jan Hubicka wrote:

gcc/ChangeLog:

2020-03-12  Martin Liska  <mli...@suse.cz>

        * lto-section-in.c: Add extension_symtab.
        * lto-streamer-out.c (write_symbol_extension_info):
        New.
        (produce_symtab_extension): New.
        (produce_asm_for_decls): Stream also produce_symtab_extension.
        * lto-streamer.h (enum lto_section_type): New section.

include/ChangeLog:

2020-03-12  Martin Liska  <mli...@suse.cz>

        * lto-symtab.h (enum gcc_plugin_symbol_type): New.
        (enum gcc_plugin_symbol_section_flags): Likewise.

lto-plugin/ChangeLog:

2020-03-12  Martin Liska  <mli...@suse.cz>

        * lto-plugin.c (LTO_SECTION_PREFIX): Rename to
        ...
        (LTO_SYMTAB_PREFIX): ... this.
        (LTO_SECTION_PREFIX_LEN): Rename to ...
        (LTO_SYMTAB_PREFIX_LEN): ... this.
        (LTO_SYMTAB_EXT_PREFIX): New.
        (LTO_SYMTAB_EXT_PREFIX_LEN): New.
        (LTO_LTO_PREFIX): New.
        (LTO_LTO_PREFIX_LEN): New.
        (parse_table_entry): Fill up unused to zero.
        (parse_table_entry_extension): New.
        (parse_symtab_extension): New.
        (finish_conflict_resolution): Change type
        for resolution.
        (clear_new_symtab_flags): New.
        (write_resolution): Support new get_symbols_v4.
        (process_symtab): Use new macro name.
        (process_symtab_extension): New.
        (claim_file_handler): Parse also process_symtab_extension.
        (onload): Call new add_symbols_v2.

Hi,
thanks for updating the patch.  Two minor nits

---
  gcc/lto-section-in.c    |   3 +-
  gcc/lto-streamer-out.c  |  64 +++++++++++++++-
  gcc/lto-streamer.h      |   1 +
  include/lto-symtab.h    |  12 +++
  lto-plugin/lto-plugin.c | 165 +++++++++++++++++++++++++++++++++++-----
  5 files changed, 226 insertions(+), 19 deletions(-)

diff --git a/gcc/lto-section-in.c b/gcc/lto-section-in.c
index c17dd69dbdd..7bf59c513fc 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",
+  "extension_symtab"

I would call it symtab_ext so alphabetically it is coupled with symtab.

It's problematic because of this collision where we search for prefix:

if (strncmp (name, LTO_SYMTAB_PREFIX, LTO_SYMTAB_PREFIX_LEN) != 0)

Note that the section names have suffix:
.gnu.lto_.symtab.48f1e54b1c048b0f

I wonder if moving it up in the enum would make it physically appear
next to .symtab in object file so we save a bit of i/o?

It's not about names order, but streaming order. Which should be fine:

$ readelf -SW /tmp/bss.o
...
  [10] .gnu.lto_.decls.48f1e54b1c048b0f PROGBITS        0000000000000000 0001dd 
0002de 00   E  0   0  1
  [11] .gnu.lto_.symtab.48f1e54b1c048b0f PROGBITS        0000000000000000 
0004bb 000049 00   E  0   0  1
  [12] .gnu.lto_.extension_symtab.48f1e54b1c048b0f PROGBITS        
0000000000000000 000504 000006 00   E  0   0  1
  [13] .gnu.lto_.opts    PROGBITS        0000000000000000 00050a 000051 00   E  
0   0  1
...

+static void
+produce_symtab_extension (struct output_block *ob)
+{
+  char *section_name = lto_get_section_name (LTO_section_symtab_extension,
+                                            NULL, 0, NULL);
+  lto_symtab_encoder_t encoder = ob->decl_state->symtab_node_encoder;
+  lto_symtab_encoder_iterator lsei;
+
+  lto_begin_section (section_name, false);
+  free (section_name);
+
+  /* Write the symbol table.
+     First write everything defined and then all declarations.
+     This is necessary to handle cases where we have duplicated symbols.  */
+  for (lsei = lsei_start (encoder);
+       !lsei_end_p (lsei); lsei_next (&lsei))
+    {
+      symtab_node *node = lsei_node (lsei);
+
+      if (DECL_EXTERNAL (node->decl) || !node->output_to_lto_symbol_table_p ())
+       continue;
+      write_symbol_extension_info (node->decl);
+    }
+  for (lsei = lsei_start (encoder);
+       !lsei_end_p (lsei); lsei_next (&lsei))
+    {
+      symtab_node *node = lsei_node (lsei);
+
+      if (!DECL_EXTERNAL (node->decl) || !node->output_to_lto_symbol_table_p 
())
+       continue;
+      write_symbol_extension_info (node->decl);
+    }
+
+  lto_end_section ();
+}

It seems fragile to me to duplicate the loops that needs to be kept in
sync because breaking that will likely get bit suprising outcome (i.e.
bootstrap will work since we do not care about symbol types). Perhaps it
would be more robust to simply push the extension bits into a vector in
write_symbol for later streaming.  Or at least add comment that loops
needs to be kept in sync.

I'll add comment.

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 09e1202df07..804684449cf 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -87,7 +87,17 @@ struct ld_plugin_symbol
  {
    char *name;
    char *version;
-  int def;
+#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

Perhaps at least drop a comment why this is done this way (i.e.
compatibility with V3 version of API) so in 10 years we know?

Likewise here.

Bit more systematic way would be to add a new hook to query extended
properties of a given symbol. I.e. something like
void *get_symbol_property (symbol, enum property)

I know, but it would complicate code on both consumer and LTO plugin side.

Martin

Honza