Re: [BFD][PR21703]Override the new defined symbol with the old normal symbol when --allow-multiple-definition is provided

[Renlin Li <renlin dot li at foss dot arm dot com>]

Hi H.J,

Here is the updated patch. Two new test cases are added to test "-r" and "-shared" with 
version symbol.
X86, arm, aarch64 are tested Okay with the change and test cases.

By the way, by this change, some warnings about changing symbol size and type are 
eliminated. This is because the later defined symbol will be skipped instead of merging 
with old definition.

Regards,
Renlin

bfd/ChangeLog:

2017-10-17  Renlin Li  <renlin.li@arm.com>

	* elflink.c (_bfd_elf_merge_symbol): Handle multiple definition case.

ld/ChangeLog:

2017-10-17  Renlin Li  <renlin.li@arm.com>

	* testsuite/ld-elf/elf.exp: Run new tests.
	* testsuite/ld-elf/pr21703-1.s: New.
	* testsuite/ld-elf/pr21703-2.s: New.
	* testsuite/ld-elf/pr21703-3.s: New.
	* testsuite/ld-elf/pr21703-4.s: New.
	* testsuite/ld-elf/pr21703-r.sd: New.
	* testsuite/ld-elf/pr21703-shared.sd: New.
	* testsuite/ld-elf/pr21703.sd: New.
	* testsuite/ld-elf/pr21703.ver: New.

On 12/10/17 21:52, H.J. Lu wrote:
> On 10/12/17, Renlin Li <renlin.li@foss.arm.com> wrote:
> > Hi H.J.
> >
> > According to the logic, if there are versioned symbols with the same symbol
> > name and
> > version, e.g. sym_name@VERSION. It will treated as multiple definition of
> > symbol.
> > Until now, the behavior is the same as before. The linker will throw
> > multiple definition
> > error.
> >
> > With the change here, and with "--allow-multiple-definition" option
> > provided,
> > the first defined one will fully override a later definition.
> >
> > For default version symbol, it's slightly different.
> > You can only declare one version of a symbol as the default in this manner;
> > otherwise you
> > would effectively have multiple definitions of the same symbol.
> >
> > For sym_name@@VERSION, two symbols will be added. One is sym_name@@VERSION.
> > As above, you
> > cannot define sym_name@@VERSION twice.
> > what's more, _bfd_elf_add_default_symbol will add one "sym_name" indirect
> > symbol. The
> > check for this symbol definition is exclude explicitly in this patch as the
> > condition
> > indicates.
> >
> > pr21703-3.s
> > 	.text
> > 	.global foo
> > 	.symver	foo, foo@FOO
> > 	.type	foo, %function
> > foo:
> > 	.space	4
> > 	.size	foo, 4
> >
> > pr21703-4.s
> > 	.text
> > 	.global bar
> > 	.symver	bar, foo@FOO
> > 	.type	bar, %function
> > bar:
> > 	.space	16
> > 	.size	bar, 16
> >
> > With the following command line:
> >
> > as-new pr21703-3.o pr21703-3.s
> > as-new pr21703-4.o pr21703-4.s
> > ld-new -o pr21703 -z norelro  pr21703-3.o pr21703-4.o
> > --allow-multiple-definition
> >
> > without the patch,
> > The symbol table of final object is:
> >      Num:    Value          Size Type    Bind   Vis      Ndx Name
> >        0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
> >        1: 0000000000400078     0 SECTION LOCAL  DEFAULT    1
> >        2: 0000000000600090     0 OBJECT  LOCAL  DEFAULT    1
> > _GLOBAL_OFFSET_TABLE_
> >        3: 0000000000400078    16 FUNC    GLOBAL DEFAULT    1 foo@FOO
> >        4: 000000000060008c     0 NOTYPE  GLOBAL DEFAULT    1 __bss_start
> >        5: 000000000060008c     0 NOTYPE  GLOBAL DEFAULT    1 _edata
> >        6: 0000000000600090     0 NOTYPE  GLOBAL DEFAULT    1 _end
> >        7: 000000000040007c    16 FUNC    GLOBAL DEFAULT    1 bar
> >
> > foo@FOO is mapped to foo (the value) in pr21703-3.o with size from bar in
> > pr21703-4.o.
> > with the patch:
> >
> >      Num:    Value          Size Type    Bind   Vis      Ndx Name
> >        0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
> >        1: 0000000000400078     0 SECTION LOCAL  DEFAULT    1
> >        2: 0000000000600090     0 OBJECT  LOCAL  DEFAULT    1
> > _GLOBAL_OFFSET_TABLE_
> >        3: 0000000000400078     4 FUNC    GLOBAL DEFAULT    1 foo@FOO
> >        4: 000000000060008c     0 NOTYPE  GLOBAL DEFAULT    1 __bss_start
> >        5: 000000000060008c     0 NOTYPE  GLOBAL DEFAULT    1 _edata
> >        6: 0000000000600090     0 NOTYPE  GLOBAL DEFAULT    1 _end
> >        7: 000000000040007c    16 FUNC    GLOBAL DEFAULT    1 bar
> >
> > foo@FOO is fully mapped to foo.
> >
> > The above behavior is observed both on x64 and arm.
> >
> > Is this the kind of test you are suggesting?
>
> Yes, test both foo@FOO1 and foo@@OFOO2 with -r and -shared.
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 1a99058..9a60695 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1036,6 +1036,7 @@ _bfd_elf_merge_symbol (bfd *abfd,
   bfd_boolean newweak, oldweak, newfunc, oldfunc;
   const struct elf_backend_data *bed;
   char *new_version;
+  bfd_boolean default_sym = *matched;
 
   *skip = FALSE;
   *override = FALSE;
@@ -1557,6 +1558,18 @@ _bfd_elf_merge_symbol (bfd *abfd,
       sec = *psec;
     }
 
+  /* There are multiple definitions of a normal symbol.
+     Skip the default symbol as well.  */
+  if (olddef && !olddyn && !oldweak && newdef && !newdyn && !newweak
+      && !default_sym && h->def_regular)
+    {
+      /* Handle a multiple definition.  */
+      (*info->callbacks->multiple_definition) (info, &h->root,
+					       abfd, sec, *pvalue);
+      *skip = TRUE;
+      return TRUE;
+    }
+
   /* If both the old and the new symbols look like common symbols in a
      dynamic object, set the size of the symbol to the larger of the
      two.  */
diff --git a/ld/testsuite/ld-elf/elf.exp b/ld/testsuite/ld-elf/elf.exp
index 655f0da..eac29e0 100644
--- a/ld/testsuite/ld-elf/elf.exp
+++ b/ld/testsuite/ld-elf/elf.exp
@@ -70,6 +70,18 @@ run_ld_link_tests [list \
 	{symbol3w.s} {} "symbol3w.a" ] \
 ]
 
+run_ld_link_tests [list \
+    [list "PR ld/21703" \
+	"--allow-multiple-definition tmpdir/pr21703-1.o tmpdir/pr21703-2.o" "" "" \
+	{pr21703-1.s pr21703-2.s} {{readelf {-s} pr21703.sd}} "pr21703" ] \
+    [list "PR ld/21703 -r" \
+	"-r --allow-multiple-definition tmpdir/pr21703-3.o tmpdir/pr21703-4.o" "" "" \
+	{pr21703-3.s pr21703-4.s} {{readelf {-s} pr21703-r.sd}} "pr21703.o" ] \
+    [list "PR ld/21703 shared" \
+	"-shared --allow-multiple-definition --version-script pr21703.ver tmpdir/pr21703-3.o tmpdir/pr21703-4.o" "" "" \
+	{pr21703-3.s pr21703-4.s} {{readelf {--dyn-syms} pr21703-shared.sd}} "pr21703.so" ] \
+]
+
 if { [check_shared_lib_support] } then {
     run_ld_link_tests {
 	{"Build pr14170a.o" "" "" "" {pr14170a.s} {} "pr14170.a" }
diff --git a/ld/testsuite/ld-elf/pr21703-1.s b/ld/testsuite/ld-elf/pr21703-1.s
new file mode 100644
index 0000000..92a4718
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr21703-1.s
@@ -0,0 +1,6 @@
+	.text
+	.globl	foo
+	.type	foo, %function
+foo:
+	.space	4
+	.size	foo, 4
diff --git a/ld/testsuite/ld-elf/pr21703-2.s b/ld/testsuite/ld-elf/pr21703-2.s
new file mode 100644
index 0000000..1d65304
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr21703-2.s
@@ -0,0 +1,6 @@
+	.text
+	.globl	foo
+	.type	foo, %function
+foo:
+	.space	16
+	.size	foo, 16
diff --git a/ld/testsuite/ld-elf/pr21703-3.s b/ld/testsuite/ld-elf/pr21703-3.s
new file mode 100644
index 0000000..6da6de8
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr21703-3.s
@@ -0,0 +1,15 @@
+	.text
+	.global foo
+	.type	foo, %function
+foo:
+	.space	4
+	.size	foo, 4
+
+	.global foo1
+	.type	foo1, %function
+foo1:
+	.space	32
+	.size	foo1, 32
+
+	.symver	foo, foo@FOO
+	.symver	foo1, foo@@FOO1
diff --git a/ld/testsuite/ld-elf/pr21703-4.s b/ld/testsuite/ld-elf/pr21703-4.s
new file mode 100644
index 0000000..9390e94
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr21703-4.s
@@ -0,0 +1,15 @@
+	.text
+	.global bar
+	.type	bar, %function
+bar:
+	.space	16
+	.size	bar, 16
+
+	.global bar1
+	.type	bar1, %function
+bar1:
+	.space	8
+	.size	bar1, 8
+
+	.symver	bar, foo@FOO
+	.symver	bar1, foo@@FOO1
diff --git a/ld/testsuite/ld-elf/pr21703-r.sd b/ld/testsuite/ld-elf/pr21703-r.sd
new file mode 100644
index 0000000..6758088
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr21703-r.sd
@@ -0,0 +1,9 @@
+Symbol table '.symtab' contains .* entries:
+#...
+.*: [0-9a-fA-F]* +4 +FUNC +GLOBAL +DEFAULT +. foo@FOO
+.*: [0-9a-fA-F]* +32 +FUNC +GLOBAL +DEFAULT +. foo1
+.*: [0-9a-fA-F]* +32 +FUNC +GLOBAL +DEFAULT +. foo@@FOO1
+.*: [0-9a-fA-F]* +8 +FUNC +GLOBAL +DEFAULT +. bar1
+.*: [0-9a-fA-F]* +4 +FUNC +GLOBAL +DEFAULT +. foo
+.*: [0-9a-fA-F]* +16 +FUNC +GLOBAL +DEFAULT +. bar
+#pass
diff --git a/ld/testsuite/ld-elf/pr21703-shared.sd b/ld/testsuite/ld-elf/pr21703-shared.sd
new file mode 100644
index 0000000..9b6b1b9
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr21703-shared.sd
@@ -0,0 +1,8 @@
+Symbol table '\.dynsym' contains [0-9]+ entries:
+ +Num: +Value +Size Type +Bind +Vis +Ndx Name
+#...
+ +[0-9]+: +[0-9a-f]+ +4 +FUNC +GLOBAL +DEFAULT +[0-9] +foo@FOO
+ +[0-9]+: +[0-9a-f]+ +0 +OBJECT +GLOBAL +DEFAULT +ABS +FOO1
+ +[0-9]+: +[0-9a-f]+ +32 +FUNC +GLOBAL +DEFAULT +[0-9] +foo@@FOO1
+ +[0-9]+: +[0-9a-f]+ +0 +OBJECT +GLOBAL +DEFAULT +ABS +FOO
+#...
diff --git a/ld/testsuite/ld-elf/pr21703.sd b/ld/testsuite/ld-elf/pr21703.sd
new file mode 100644
index 0000000..955cf17
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr21703.sd
@@ -0,0 +1,4 @@
+Symbol table '.symtab' contains .* entries:
+#...
+.*: [0-9a-fA-F]* +4 +FUNC +GLOBAL +DEFAULT +. foo
+#pass
diff --git a/ld/testsuite/ld-elf/pr21703.ver b/ld/testsuite/ld-elf/pr21703.ver
new file mode 100644
index 0000000..c36f292
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr21703.ver
@@ -0,0 +1,4 @@
+FOO
+{ global: foo; local: *; };
+FOO1
+{ global: foo; local: *; };