RFC [PATCH 0/2] Use ld_plugin_symbol_type and ld_plugin_symbol_section_flags

RFC [PATCH 0/2] Use ld_plugin_symbol_type and ld_plugin_symbol_section_flags

[H.J. Lu]

Since LTO plugin may generate more than one ltrans.o file from one input
IR object as LTO wrapper ignores -flto-partition=none:

lto-wrapper.c:608:

   604          /* Drop arguments that we want to take from the link line.  */
   605          case OPT_flto_:
   606          case OPT_flto:
   607          case OPT_flto_partition_:
   608            continue;

the LTO wrapper approach is not only slow but also unreliable.  There is
a proposal to extend The LTO plugin API to add LDPT_ADD_SYMBOLS_V2 with
symbol type and section flags:

https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542187.html

We can use the new LTO plugin API to get symbol type, instead of invoking
the LTO wrapper.

H.J. Lu (1):
  Use ld_plugin_symbol_type and ld_plugin_symbol_section_flags

Martin Liska (1):
  Update include/plugin-api.h

 bfd/plugin.c         | 66 ++++++++++++++++++++++++++++++++++----------
 include/plugin-api.h | 32 +++++++++++++++++++--
 2 files changed, 82 insertions(+), 16 deletions(-)

-- 
2.25.1

RFC [PATCH 1/2] Update include/plugin-api.h

[H.J. Lu]

From: Martin Liska <mliska@suse.cz>

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

	PR binutils/25640
	* plugin-api.h (struct ld_plugin_symbol): Split
	int def into 4 char fields.
	(enum ld_plugin_symbol_type): New.
	(enum ld_plugin_symbol_section_flags): New.
	(enum ld_plugin_tag): Add LDPT_ADD_SYMBOLS_V2 and
	LDPT_GET_SYMBOLS_V4.
---
 include/plugin-api.h | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 09e1202df0..88676a63e5 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -87,7 +87,19 @@ struct ld_plugin_symbol
 {
   char *name;
   char *version;
-  int def;
+  /* This is for compatibility with older ABIs.  The older ABI defined
+     only 'def' field.  */
+#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 +135,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 +457,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

RFC [PATCH 2/2] Use ld_plugin_symbol_type and ld_plugin_symbol_section_flags

[H.J. Lu]

Since LTO plugin may generate more than one ltrans.o file from one input
IR object as LTO wrapper ignores -flto-partition=none:

lto-wrapper.c:608:

   604          /* Drop arguments that we want to take from the link line.  */
   605          case OPT_flto_:
   606          case OPT_flto:
   607          case OPT_flto_partition_:
   608            continue;

the LTO wrapper approach is not only slow but also unreliable.  There is
a proposal to extend The LTO plugin API to add LDPT_ADD_SYMBOLS_V2 with
symbol type and section flags:

https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542187.html

We can use the new LTO plugin API to get symbol type, instead of invoking
the LTO wrapper.

	PR binutils/25640
	* plugin.c (plugin_list_entry): Add has_symbol_type.
	(add_symbols_v2): New function.
	(bfd_plugin_open_input): Don't invoke LTO wrapper if LTO plugin
	provides symbol type.
	(try_load_plugin): Add LDPT_ADD_SYMBOLS_V2.
	(bfd_plugin_canonicalize_symtab): Use LTO plugin symbol type if
	available.
---
 bfd/plugin.c | 66 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 14 deletions(-)

diff --git a/bfd/plugin.c b/bfd/plugin.c
index a0f172d363..f33bc3ddef 100644
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -136,6 +136,7 @@ struct plugin_list_entry
   asymbol **real_syms;
   int lto_nsyms;
   const struct ld_plugin_symbol *lto_syms;
+  bfd_boolean has_symbol_type;
 
   struct plugin_list_entry *next;
 
@@ -503,6 +504,14 @@ add_symbols (void * handle,
   return LDPS_OK;
 }
 
+static enum ld_plugin_status
+add_symbols_v2 (void *handle, int nsyms,
+		const struct ld_plugin_symbol *syms)
+{
+  current_plugin->has_symbol_type = TRUE;
+  return add_symbols (handle, nsyms, (struct ld_plugin_symbol *) syms);
+}
+
 int
 bfd_plugin_open_input (bfd *ibfd, struct ld_plugin_input_file *file)
 {
@@ -560,7 +569,8 @@ try_claim (bfd *abfd)
       current_plugin->claim_file (&file, &claimed);
       if (claimed)
 	{
-	  if (current_plugin->all_symbols_read)
+	  if (current_plugin->all_symbols_read
+	      && !current_plugin->has_symbol_type)
 	    {
 	      struct plugin_data_struct *plugin_data
 		= abfd->tdata.plugin_data;
@@ -602,7 +612,7 @@ try_load_plugin (const char *pname,
 		 bfd *abfd, bfd_boolean build_list_p)
 {
   void *plugin_handle;
-  struct ld_plugin_tv tv[12];
+  struct ld_plugin_tv tv[13];
   int i;
   ld_plugin_onload onload;
   enum ld_plugin_status status;
@@ -665,6 +675,10 @@ try_load_plugin (const char *pname,
   tv[i].tv_tag = LDPT_ADD_SYMBOLS;
   tv[i].tv_u.tv_add_symbols = add_symbols;
 
+  ++i;
+  tv[i].tv_tag = LDPT_ADD_SYMBOLS_V2;
+  tv[i].tv_u.tv_add_symbols = add_symbols_v2;
+
   if (get_lto_wrapper (plugin_list_iter))
     {
       ++i;
@@ -977,9 +991,15 @@ bfd_plugin_canonicalize_symtab (bfd *abfd,
   struct plugin_data_struct *plugin_data = abfd->tdata.plugin_data;
   long nsyms = plugin_data->nsyms;
   const struct ld_plugin_symbol *syms = plugin_data->syms;
-  static asection fake_section
-    = BFD_FAKE_SECTION (fake_section, NULL, "plug", 0,
+  static asection fake_text_section
+    = BFD_FAKE_SECTION (fake_text_section, NULL, "plug", 0,
 			SEC_ALLOC | SEC_LOAD | SEC_CODE | SEC_HAS_CONTENTS);
+  static asection fake_data_section
+    = BFD_FAKE_SECTION (fake_data_section, NULL, "plug", 0,
+			SEC_ALLOC | SEC_LOAD | SEC_DATA | SEC_HAS_CONTENTS);
+  static asection fake_bss_section
+    = BFD_FAKE_SECTION (fake_bss_section, NULL, "plug", 0,
+			SEC_ALLOC);
   static asection fake_common_section
     = BFD_FAKE_SECTION (fake_common_section, NULL, "plug", 0, SEC_IS_COMMON);
   int i, j;
@@ -1014,16 +1034,34 @@ bfd_plugin_canonicalize_symtab (bfd *abfd,
 	  break;
 	case LDPK_DEF:
 	case LDPK_WEAKDEF:
-	  s->section = &fake_section;
-	  if (real_nsyms)
-	    /* Use real LTO symbols if possible.  */
-	    for (j = 0; j < real_nsyms; j++)
-	      if (real_syms[j]->name
-		  && strcmp (syms[i].name, real_syms[j]->name) == 0)
-		{
-		  s->section = real_syms[j]->section;
-		  break;
-		}
+	  if (current_plugin->has_symbol_type)
+	    switch (syms[i].symbol_type)
+	      {
+	      case LDST_UNKNOWN:
+		/* What is the best fake section for LDST_UNKNOWN?  */
+	      case LDST_FUNCTION:
+		s->section = &fake_text_section;
+		break;
+	      case LDST_VARIABLE:
+		if (syms[i].section_flags == LDSSS_BSS)
+		  s->section = &fake_bss_section;
+		else
+		  s->section = &fake_data_section;
+		break;
+	      }
+	  else
+	    {
+	      s->section = &fake_text_section;
+	      if (real_nsyms)
+		/* Use real LTO symbols if possible.  */
+		for (j = 0; j < real_nsyms; j++)
+		  if (real_syms[j]->name
+		      && strcmp (syms[i].name, real_syms[j]->name) == 0)
+		    {
+		      s->section = real_syms[j]->section;
+		      break;
+		    }
+	    }
 	  break;
 	default:
 	  BFD_ASSERT (0);
-- 
2.25.1

[PATCH] Update include/plugin-api.h

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Wed, Mar 18, 2020 at 5:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> From: Martin Liska <mliska@suse.cz>
>
> 2020-03-12  Martin Liska  <mliska@suse.cz>
>
>         PR binutils/25640
>         * plugin-api.h (struct ld_plugin_symbol): Split
>         int def into 4 char fields.
>         (enum ld_plugin_symbol_type): New.
>         (enum ld_plugin_symbol_section_flags): New.
>         (enum ld_plugin_tag): Add LDPT_ADD_SYMBOLS_V2 and
>         LDPT_GET_SYMBOLS_V4.

I am checking in this patch to sync with GCC.

-- 
H.J.

[-- Attachment #2: 0001-Update-include-plugin-api.h.patch --]
[-- Type: application/x-patch, Size: 2305 bytes --]

Re: [PATCH] Update include/plugin-api.h

[Alan Modra]

On Thu, Mar 19, 2020 at 09:09:00AM -0700, H.J. Lu via Binutils wrote:
> On Wed, Mar 18, 2020 at 5:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > From: Martin Liska <mliska@suse.cz>
> >
> > 2020-03-12  Martin Liska  <mliska@suse.cz>
> >
> >         PR binutils/25640
> >         * plugin-api.h (struct ld_plugin_symbol): Split
> >         int def into 4 char fields.
> >         (enum ld_plugin_symbol_type): New.
> >         (enum ld_plugin_symbol_section_flags): New.
> >         (enum ld_plugin_tag): Add LDPT_ADD_SYMBOLS_V2 and
> >         LDPT_GET_SYMBOLS_V4.
> 
> I am checking in this patch to sync with GCC.

Silence warnings due to plugin API change.

	* testplug.c (parse_symdefstr): Use %hhi to read sym->def, and
	clear new fields.
	* testplug2.c (parse_symdefstr): Likewise.
	* testplug3.c (parse_symdefstr): Likewise.
	* testplug4.c (parse_symdefstr): Likewise.

diff --git a/ld/testplug.c b/ld/testplug.c
index 9dd0b91e45..c126f03607 100644
--- a/ld/testplug.c
+++ b/ld/testplug.c
@@ -239,12 +239,15 @@ parse_symdefstr (const char *str, struct ld_plugin_symbol *sym)
   /* Finally we'll use sscanf to parse the numeric fields, then
      we'll split out the strings which we need to allocate separate
      storage for anyway so that we can add nul termination.  */
-  n = sscanf (colon2 + 1, "%i:%i:%lli", &sym->def, &sym->visibility, &size);
+  n = sscanf (colon2 + 1, "%hhi:%i:%lli", &sym->def, &sym->visibility, &size);
   if (n != 3)
     return LDPS_ERR;
 
   /* Parsed successfully, so allocate strings and fill out fields.  */
   sym->size = size;
+  sym->unused = 0;
+  sym->section_kind = 0;
+  sym->symbol_type = 0;
   sym->resolution = LDPR_UNKNOWN;
   sym->name = malloc (colon1 - str + 1);
   if (!sym->name)
diff --git a/ld/testplug2.c b/ld/testplug2.c
index ecd9a44d73..27553d0781 100644
--- a/ld/testplug2.c
+++ b/ld/testplug2.c
@@ -218,12 +218,15 @@ parse_symdefstr (const char *str, struct ld_plugin_symbol *sym)
   /* Finally we'll use sscanf to parse the numeric fields, then
      we'll split out the strings which we need to allocate separate
      storage for anyway so that we can add nul termination.  */
-  n = sscanf (colon2 + 1, "%i:%i:%lli", &sym->def, &sym->visibility, &size);
+  n = sscanf (colon2 + 1, "%hhi:%i:%lli", &sym->def, &sym->visibility, &size);
   if (n != 3)
     return LDPS_ERR;
 
   /* Parsed successfully, so allocate strings and fill out fields.  */
   sym->size = size;
+  sym->unused = 0;
+  sym->section_kind = 0;
+  sym->symbol_type = 0;
   sym->resolution = LDPR_UNKNOWN;
   sym->name = malloc (colon1 - str + 1);
   if (!sym->name)
diff --git a/ld/testplug3.c b/ld/testplug3.c
index 05fdca02e2..928f4d6842 100644
--- a/ld/testplug3.c
+++ b/ld/testplug3.c
@@ -217,12 +217,15 @@ parse_symdefstr (const char *str, struct ld_plugin_symbol *sym)
   /* Finally we'll use sscanf to parse the numeric fields, then
      we'll split out the strings which we need to allocate separate
      storage for anyway so that we can add nul termination.  */
-  n = sscanf (colon2 + 1, "%i:%i:%lli", &sym->def, &sym->visibility, &size);
+  n = sscanf (colon2 + 1, "%hhi:%i:%lli", &sym->def, &sym->visibility, &size);
   if (n != 3)
     return LDPS_ERR;
 
   /* Parsed successfully, so allocate strings and fill out fields.  */
   sym->size = size;
+  sym->unused = 0;
+  sym->section_kind = 0;
+  sym->symbol_type = 0;
   sym->resolution = LDPR_UNKNOWN;
   sym->name = malloc (colon1 - str + 1);
   if (!sym->name)
diff --git a/ld/testplug4.c b/ld/testplug4.c
index adaedf47c3..ca899b11da 100644
--- a/ld/testplug4.c
+++ b/ld/testplug4.c
@@ -218,12 +218,15 @@ parse_symdefstr (const char *str, struct ld_plugin_symbol *sym)
   /* Finally we'll use sscanf to parse the numeric fields, then
      we'll split out the strings which we need to allocate separate
      storage for anyway so that we can add nul termination.  */
-  n = sscanf (colon2 + 1, "%i:%i:%lli", &sym->def, &sym->visibility, &size);
+  n = sscanf (colon2 + 1, "%hhi:%i:%lli", &sym->def, &sym->visibility, &size);
   if (n != 3)
     return LDPS_ERR;
 
   /* Parsed successfully, so allocate strings and fill out fields.  */
   sym->size = size;
+  sym->unused = 0;
+  sym->section_kind = 0;
+  sym->symbol_type = 0;
   sym->resolution = LDPR_UNKNOWN;
   sym->name = malloc (colon1 - str + 1);
   if (!sym->name)



-- 
Alan Modra
Australia Development Lab, IBM