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

Ping ~

Regards,
Renlin

On 12/07/17 15:21, Renlin Li wrote:
> Hi,
>
> On 12/07/17 15:17, Renlin Li wrote:
> > Hi H.J,
> >
> > On 11/07/17 19:04, H.J. Lu wrote:
> > > On Tue, Jul 11, 2017 at 10:31 AM, Renlin Li <renlin.li@foss.arm.com> wrote:
> > > > ld/ChangeLog:
> > > >
> > > > 2017-07-11  Renlin Li  <renlin.li@arm.com>
> > > >
> > > >          PR ld/21703
> > > >          * testsuite/ld-elf/elf.exp : Run new test case.
> > > >          * testsuite/ld-elf/pr21703-1.s: New.
> > > >          * testsuite/ld-elf/pr21703-2.s: New.
> > > >          * testsuite/ld-elf/pr21703.sd: New.
> > > >
> > > > bfd/ChangeLog:
> > > >
> > > > 2017-07-11  Renlin Li  <renlin.li@arm.com>
> > > >
> > > >
> > > >          PR ld/21703
> > > >          * elflink.c (_bfd_elf_merge_symbol): Handle multiple symbol
> > > > definition
> > > >          case.
> >
> >
> > > Please add some a test to cover versioned_sym in:
> >
> > This is because _bfd_elf_merge_symbol will be called for versioned symbol with the short
> > name.
> >
> > For example, ld/testsuite/ld-elf/pr18735.s
> > It defines foo and an alias foo@FOO.
> > in addition to that, _bfd_elf_add_default_symbol will be called for foo@FOO to create a
> > new symbol which short name (foo in this case). For the original condition, this will
> > generate a multiple definition error.
> >
> > "matched" flag seems introduced by you. Here I use it to differentiate the versioned
> > symbol case from normal symbol definition.
> >
> >
> > >    /* There are multiple definitions of a normal symbol.  */
> > > +  if (olddef && !olddyn && !oldweak && newdef && !newdyn && !newweak
> > > +      && !versioned_sym && h->def_regular)
> > > +    {
> > >
> > > What should happen if symbol is defined in linker script or command
> > > line?
> > This is a good point. I missed this case.
> > I have read the code a little bit.
> >
> > Multiple definition handling is broken for this case.
> >
> > the following command throws error about multiple definitions.
> > ld --defsym foo=0x10 pr21703-1.o -o tmpdir/dump
> >
> > The command links fine, generating foo symbol with *MERGED* information from two
> > definition.
> > ld pr21703-1.o --defsym foo=0x10 -o tmpdir/dump
> > 00000010     4 OBJECT  GLOBAL DEFAULT  ABS foo
> >
> > The functions to process symbol definitions from object file and command line is different.
> > Multiple-definition is not handled for command line symbol definition. And the symbol
> > value is overwritten by the symbol absolute value. The definition section is overwritten
> > as well to ABS section.
> >
> > It's intended. Alan had a patch to allow multiple symbol definition without provide
> > --allow-multiple-definition option.
> > https://sourceware.org/ml/binutils/2014-12/msg00247.html
> >
> > The partial symbol information overwritten is the same problem as the original bug I
> > reported.
>
> So there is another question. What's the expected override behavior for command line,
> linker script and object/(static/dynamic)library defined symbols?
>
> Is there a defined rule for those cases that I have missed?
>
> Thanks!
> Renlin.
>
> > With or without the patch, the behavior remains the same.
> > The symbol defined in command line option don't set h->def_regular flag.
> > h->ldscript_def flag is set instead.
> >
> > Regards,
> > Renlin
commit fab9e096df4daf216a676a6730ab5eee52980d6b
Author: Renlin Li <renlin.li@arm.com>
Date:   Mon Jul 31 22:23:15 2017 +0100

    tmp

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..759f219 100644
--- a/ld/testsuite/ld-elf/elf.exp
+++ b/ld/testsuite/ld-elf/elf.exp
@@ -70,6 +70,12 @@ 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" ] \
+]
+
 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.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