[v2] ld: Change indirect symbol from IR to undefined

Message ID CAMe9rOrn2cFHbnE12Ms2-PMEsnk4c4vkM12TLZcvj8ns0DREeg@mail.gmail.com
State New
Headers show
Series
  • [v2] ld: Change indirect symbol from IR to undefined
Related show

Commit Message

Alan Modra via Binutils Aug. 27, 2021, 3 p.m.
On Fri, Aug 27, 2021 at 7:43 AM Alan Modra <amodra@gmail.com> wrote:
>

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

>         }


Fixed.

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


Fixed.

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


There are

config/elfos.h:#define ASM_OUTPUT_SYMVER_DIRECTIVE(FILE, NAME, NAME2)      \
varasm.c:#ifdef ASM_OUTPUT_SYMVER_DIRECTIVE
varasm.c:  ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,

But microblaze-linux target doesn't include elfos.h.   It looks like a
microblaze-linux bug.

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


Fixed.

Here is the v2 patch.

-- 
H.J.

Comments

Alan Modra via Binutils Aug. 28, 2021, 12:47 a.m. | #1
On Fri, Aug 27, 2021 at 08:00:22AM -0700, H.J. Lu wrote:
> Here is the v2 patch.


OK, looks good.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra via Binutils Aug. 28, 2021, 12:53 a.m. | #2
On Sat, Aug 28, 2021 at 10:17:04AM +0930, Alan Modra wrote:
> On Fri, Aug 27, 2021 at 08:00:22AM -0700, H.J. Lu wrote:

> > Here is the v2 patch.

> 

> OK, looks good.


I've also just committed the following, a fix to my pr26978 patch.

	PR 28264
	PR 26978
	* linker.c (_bfd_generic_link_add_one_symbol <MIND>): Check
	that string is non-NULL.

diff --git a/bfd/linker.c b/bfd/linker.c
index 755ff19923b..f8257ea11b4 100644
--- a/bfd/linker.c
+++ b/bfd/linker.c
@@ -1682,7 +1682,7 @@ _bfd_generic_link_add_one_symbol (struct bfd_link_info *info,
 	      cycle = true;
 	      break;
 	    }
-	  if (strcmp (h->u.i.link->root.string, string) == 0)
+	  if (string != NULL && strcmp (h->u.i.link->root.string, string) == 0)
 	    break;
 	  /* Fall through.  */
 	case MDEF:

-- 
Alan Modra
Australia Development Lab, IBM

Patch

From c751494bc3ba087bd61b7bb4e7127bc1fc2fd23e Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 26 Aug 2021 07:43:23 -0700
Subject: [PATCH v2] ld: Change indirect symbol from IR to undefined

bfd/

	PR ld/28264
	* elflink.c (_bfd_elf_merge_symbol): Change indirect symbol from
	IR to undefined.

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-4.d: Likewise.
	* testsuite/ld-plugin/pr28264.c: Likewise.
	* testsuite/ld-plugin/pr28264.ver: Likewise.
---
 bfd/elflink.c                      | 23 +++++++++++++++++------
 ld/testsuite/ld-plugin/lto.exp     |  9 +++++++++
 ld/testsuite/ld-plugin/pr28264-1.d |  5 +++++
 ld/testsuite/ld-plugin/pr28264-2.d |  5 +++++
 ld/testsuite/ld-plugin/pr28264-3.d |  4 ++++
 ld/testsuite/ld-plugin/pr28264-4.d |  4 ++++
 ld/testsuite/ld-plugin/pr28264.c   | 11 +++++++++++
 ld/testsuite/ld-plugin/pr28264.ver |  8 ++++++++
 8 files changed, 63 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-4.d
 create mode 100644 ld/testsuite/ld-plugin/pr28264.c
 create mode 100644 ld/testsuite/ld-plugin/pr28264.ver

diff --git a/bfd/elflink.c b/bfd/elflink.c
index c9d5da2dab4..6fe90412b28 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1272,14 +1272,25 @@  _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
+	  && hi->root.type == bfd_link_hash_indirect)
+	{
+	  /* Change indirect symbol from IR to undefined.  */
+	  hi->root.type = bfd_link_hash_undefined;
+	  hi->root.u.undef.abfd = oldbfd;
+	}
     }
 
   /* 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..c343132c460 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -238,6 +238,15 @@  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} \
+	  {readelf {--dyn-syms --wide} pr28264-4.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..5ff89694c98
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264-1.d
@@ -0,0 +1,5 @@ 
+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]+ _?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..f2f629d4a32
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264-2.d
@@ -0,0 +1,5 @@ 
+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
+#pass
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-4.d b/ld/testsuite/ld-plugin/pr28264-4.d
new file mode 100644
index 00000000000..408b853831c
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264-4.d
@@ -0,0 +1,4 @@ 
+#failif
+#...
+ +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +DEFAULT .*[0-9]+ _?bar
+#...
diff --git a/ld/testsuite/ld-plugin/pr28264.c b/ld/testsuite/ld-plugin/pr28264.c
new file mode 100644
index 00000000000..4efa934fd3e
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28264.c
@@ -0,0 +1,11 @@ 
+void
+__attribute__ ((symver ("foo@VERSION.1")))
+foo (void)
+{
+}
+
+void
+__attribute__ ((symver ("bar@@VERSION.1")))
+bar1 (void)
+{
+}
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:
+    *;
+};
-- 
2.31.1