This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

Hi Alan,

On 06/07/17 06:43, Alan Modra wrote:
On Tue, Jul 04, 2017 at 08:50:50PM +0100, Renlin Li wrote:
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1569,6 +1569,15 @@ _bfd_elf_merge_symbol (bfd *abfd,
        *size_change_ok = TRUE;

+  /* The old symbol definition is overriding the new one if the symbol is a
+     normal symbol.  */
+  if (olddef && !olddyn && !oldweak
+      && newdef && info->allow_multiple_definition)
+    {
+      *override = TRUE;
+      return TRUE;
+    }
    /* If we are looking at a dynamic object, and we have found a
       definition, we need to see if the symbol was already defined by
       some other object.  If so, we want to use the existing

This doesn't look correct to me.  override is really meant to handle
cases involving dynamic symbols.  I think skip should be set instead.

It might be easier to get the condition correct, and better for
maintenance, if you write

   if (<multiple definition detected>)
       if (!info->allow_multiple_definition)
	into->callbacks->multiple_definition (info, h, abfd, sec, *pvalue);
       *skip = TRUE;
       return TRUE;

ie. handle multiple definitions completely in _bfd_elf_merge_symbol.
I haven't written in the condition because I'll probably get it wrong
without testing.  :)

I was hesitating to use OVERRIDE flags at that time. SKIP seems more clean.
To do multiple definition handling here is a good idea.
There are similar code in _bfd_elf_merge_symbol which handles the multiple common symbol case.

I didn't include the condition to check allow_multiple_definition in the patch as it's done in multiple_definition function.

The condition to check multiple definition of normal symbol is quite strict (somehow looks not so smart). Both symbols definitions are neither weak nor dynamic. And the symbol version case is excluded as well.

I did the following check.
1, define the first symbol as weak.
2, create shared object to make the first symbol a dynamic symbol.

It all compiles Okay without using allow_multiple_definition option. And the arm version of foo() overrides the thumb version. And for the original case links only when allow_multiple_definition is given, the old definition overrides the new definition.

ld cross and native checked Okay for arm environment. glibc builds Okay with the patched linker.

Any other suggestions?



2017-07-07  renlin li  <>

    PR ld/21703
    * elflink.c (_bfd_elf_merge_symbol): Handle multiple symbol definition case.
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 471e8ad..e4ab670 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1547,6 +1547,17 @@ _bfd_elf_merge_symbol (bfd *abfd,
       sec = *psec;
+  /* There are multiple definitions of a normal symbol.  */
+  if (olddef && !olddyn && !oldweak && newdef && !newdyn && !newweak
+      && !*matched)
+    {
+      /* Handle a multiple definition.  */
+      (*info->callbacks->multiple_definition) (info, &h->root,
+					       abfd, sec, sym->st_value);
+      *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.  */

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]