ld: Skip the unversioned symbol from LTO

Message ID 20210826145247.3149277-1-hjl.tools@gmail.com
State New
Headers show
Series
  • ld: Skip the unversioned symbol from LTO
Related show

Commit Message

Alan Modra via Binutils Aug. 26, 2021, 2:52 p.m.
Skip the unversioned symbol from LTO if there is a versioned symbol and
the unversioned symbol has been added as an indirect symbol from IR.

bfd/

	PR ld/28264
	* elflink.c (_bfd_elf_merge_symbol): Skip the unversioned symbol
	from LTO if there is a versioned symbol and the unversioned symbol
	has been added as an indirect symbol from IR.

ld/

	PR ld/28264
	* testsuite/ld-plugin/lto.exp: Run PR ld/28264 test.
	* testsuite/ld-plugin/pr28264-1.d: New file.
	* testsuite/ld-plugin/pr28264-2.d: Likewise.
	* testsuite/ld-plugin/pr28264-3.d: Likewise.
	* testsuite/ld-plugin/pr28264.c: Likewise.
	* testsuite/ld-plugin/pr28264.ver: Likewise.
---
 bfd/elflink.c                      | 28 ++++++++++++++++++++++------
 ld/testsuite/ld-plugin/lto.exp     |  8 ++++++++
 ld/testsuite/ld-plugin/pr28264-1.d |  7 +++++++
 ld/testsuite/ld-plugin/pr28264-2.d |  4 ++++
 ld/testsuite/ld-plugin/pr28264-3.d |  4 ++++
 ld/testsuite/ld-plugin/pr28264.c   | 11 +++++++++++
 ld/testsuite/ld-plugin/pr28264.ver |  8 ++++++++
 7 files changed, 64 insertions(+), 6 deletions(-)
 create mode 100644 ld/testsuite/ld-plugin/pr28264-1.d
 create mode 100644 ld/testsuite/ld-plugin/pr28264-2.d
 create mode 100644 ld/testsuite/ld-plugin/pr28264-3.d
 create mode 100644 ld/testsuite/ld-plugin/pr28264.c
 create mode 100644 ld/testsuite/ld-plugin/pr28264.ver

-- 
2.31.1

Comments

Alan Modra via Binutils Aug. 27, 2021, 2:43 p.m. | #1
On Thu, Aug 26, 2021 at 07:52:47AM -0700, H.J. Lu wrote:
> +      if ((oldbfd->flags & BFD_PLUGIN) != 0

> +	  && h != hi

> +	  && h->versioned != unversioned

> +	  && hi->versioned == unversioned

> +	  && hi->root.type == bfd_link_hash_indirect)

> +	{

> +	  /* Skip the unversioned symbol from LTO if there is a

> +	     versioned symbol and the unversioned symbol has been

> +	     added as an indirect symbol from IR.  */

> +	  *skip = true;

> +	  return true;

> +	}


Normally a non-IR symbol definition takes precedence over an IR symbol
definition, so this looks wrong to me.

I haven't looked at what is going on here in much detail yet, but it
seems likely the correct thing to do is twiddle hi so it is no longer
indirect to an IR versioned symbol.  Then the LTO output object will
provide the correct definition, and if the LTO output also includes a
matching versioned symbol it will go back to being indirect at that
point.   Hmm, I suppose what you're doing is correct *if* the LTO
output always includes both versioned and unversioned symbols.  It
probably isn't a good idea to rely on that always being the case.

BTW,
> +	  && h != hi

> +	  && h->versioned != unversioned

> +	  && hi->versioned == unversioned

all of these tests are redundant.

The following should work.

      if ((oldbfd->flags & BFD_PLUGIN) != 0
	  && hi->root.type == bfd_link_hash_indirect)
	{
	  /* Don't leave the symbol as indirect.  */
	  hi->root.type = bfd_link_hash_undefined;
	  hi->root.u.undef.abfd = oldbfd;
	}

> +++ b/ld/testsuite/ld-plugin/pr28264-1.d

> @@ -0,0 +1,7 @@

> +Symbol table '\.dynsym' contains [0-9]+ entries:

> + +Num: +Value +Size Type +Bind +Vis +Ndx Name

> +#...

> + +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?bar@@VERSION.1

> + +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?foo@VERSION.1

> +#pass


I saw failures of the test on mips64-linux, mips-linux, and
x86_64-linux, depending on the version of compiler/startup/libraries
or something that affects the set of symbols in the linker hash table.
If you are unlucky, foo@VERSION.1 can come before bar@@VERSION.1

I also saw a weird fail on microblaze-linux
lto1: error: symver is only supported on ELF platforms
Maybe I need a newer microblaze compiler?

> +++ b/ld/testsuite/ld-plugin/pr28264-2.d

> @@ -0,0 +1,4 @@

> +#failif

> +#...

> + +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?foo

> +#...


> +++ b/ld/testsuite/ld-plugin/pr28264-3.d

> @@ -0,0 +1,4 @@

> +#failif

> +#...

> + +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?foo

> +#...


The above two files are identical, presumably one should test that bar
is not present.

-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/bfd/elflink.c b/bfd/elflink.c
index c9d5da2dab4..3a6e0d3270d 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1272,14 +1272,30 @@  _bfd_elf_merge_symbol (bfd *abfd,
       olddyn = (oldsec->symbol->flags & BSF_DYNAMIC) != 0;
     }
 
-  /* Handle a case where plugin_notice won't be called and thus won't
-     set the non_ir_ref flags on the first pass over symbols.  */
   if (oldbfd != NULL
-      && (oldbfd->flags & BFD_PLUGIN) != (abfd->flags & BFD_PLUGIN)
-      && newdyn != olddyn)
+      && (oldbfd->flags & BFD_PLUGIN) != (abfd->flags & BFD_PLUGIN))
     {
-      h->root.non_ir_ref_dynamic = true;
-      hi->root.non_ir_ref_dynamic = true;
+      if (newdyn != olddyn)
+	{
+	  /* Handle a case where plugin_notice won't be called and thus
+	     won't set the non_ir_ref flags on the first pass over
+	     symbols.  */
+	  h->root.non_ir_ref_dynamic = true;
+	  hi->root.non_ir_ref_dynamic = true;
+	}
+
+      if ((oldbfd->flags & BFD_PLUGIN) != 0
+	  && h != hi
+	  && h->versioned != unversioned
+	  && hi->versioned == unversioned
+	  && hi->root.type == bfd_link_hash_indirect)
+	{
+	  /* Skip the unversioned symbol from LTO if there is a
+	     versioned symbol and the unversioned symbol has been
+	     added as an indirect symbol from IR.  */
+	  *skip = true;
+	  return true;
+	}
     }
 
   /* NEWDEF and OLDDEF indicate whether the new or old symbol,
diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
index 68d2db1caeb..3bb19232307 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -238,6 +238,14 @@  if { [at_least_gcc_version 10 0] } {
 	 "-flto -fno-common $lto_no_fat" \
 	 {pr25355.c} \
 	 [list [list "nm" "$plug_opt" "pr25355.d"]]] \
+	[list "pr28264.so" \
+	 "-shared -Wl,--version-script=pr28264.ver" \
+	 "-flto $lto_no_fat -fPIC" \
+	 {pr28264.c} \
+	 {{readelf {--dyn-syms --wide} pr28264-1.d} \
+	  {readelf {--dyn-syms --wide} pr28264-2.d} \
+	  {readelf {--dyn-syms --wide} pr28264-3.d}} \
+	 {pr28264.so}] \
     ]]
 }
 
diff --git a/ld/testsuite/ld-plugin/pr28264-1.d b/ld/testsuite/ld-plugin/pr28264-1.d
new file mode 100644
index 00000000000..107a0813ac4
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264-1.d
@@ -0,0 +1,7 @@ 
+Symbol table '\.dynsym' contains [0-9]+ entries:
+ +Num: +Value +Size Type +Bind +Vis +Ndx Name
+#...
+ +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?bar@@VERSION.1
+ +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?foo@VERSION.1
+#pass
+
diff --git a/ld/testsuite/ld-plugin/pr28264-2.d b/ld/testsuite/ld-plugin/pr28264-2.d
new file mode 100644
index 00000000000..b6a8f1f7848
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264-2.d
@@ -0,0 +1,4 @@ 
+#failif
+#...
+ +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?foo
+#...
diff --git a/ld/testsuite/ld-plugin/pr28264-3.d b/ld/testsuite/ld-plugin/pr28264-3.d
new file mode 100644
index 00000000000..b6a8f1f7848
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264-3.d
@@ -0,0 +1,4 @@ 
+#failif
+#...
+ +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?foo
+#...
diff --git a/ld/testsuite/ld-plugin/pr28264.c b/ld/testsuite/ld-plugin/pr28264.c
new file mode 100644
index 00000000000..0135a00bd0e
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264.c
@@ -0,0 +1,11 @@ 
+void
+__attribute__ ((symver ("foo@VERSION.1")))
+foo ()
+{
+}
+
+void
+__attribute__ ((symver ("bar@@VERSION.1")))
+bar1 ()
+{
+}
diff --git a/ld/testsuite/ld-plugin/pr28264.ver b/ld/testsuite/ld-plugin/pr28264.ver
new file mode 100644
index 00000000000..1608fcae92d
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264.ver
@@ -0,0 +1,8 @@ 
+VERSION.1
+{
+  global:
+    foo;
+    bar;
+  local:
+    *;
+};