This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] PR ld/19579/21306 Properly turn common symbol into definition
On Thu, Apr 6, 2017 at 8:18 AM, Alan Modra <amodra@gmail.com> wrote:
> On Thu, Apr 06, 2017 at 07:38:54AM -0700, H.J. Lu wrote:
>> On Thu, Apr 6, 2017 at 3:43 AM, Alan Modra <amodra@gmail.com> wrote:
>> > On Wed, Apr 05, 2017 at 09:46:38AM -0700, H.J. Lu wrote:
>> >> +bfd_boolean
>> >> +_bfd_elf_define_common_symbol (bfd *output_bfd,
>> >> + struct bfd_link_info *info,
>> >> + struct bfd_link_hash_entry *h)
>> >> +{
>> >> + bfd_boolean ret
>> >> + = bfd_generic_define_common_symbol (output_bfd, info, h);
>> >> + if (ret)
>> >> + {
>> >> + /* Since def_regular may not be set if it is overridden by a
>> >> + dynamic definition, we need to set def_regular when it is
>> >> + converted to a defined symbol. */
>> >> + struct elf_link_hash_entry *eh
>> >> + = (struct elf_link_hash_entry *) h;
>> >> + eh->def_regular = 1;
>> >> + }
>> >> + return TRUE;
>> >> +}
>> >
>> > This fails for ELF targets using the generic linker, like d30v-elf.
>>
>> Does such target support shared object, which is the issue here?
>
> No, the issue is that the above code uses a cast that is invalid. The
> generic linker does not use a hash table with elf_link_hash_entry (or
> superclass) elements.
>
>> > Isn't the real bug that somewhere we have code lacking an
>> > ELF_COMMON_DEF_P test?
>>
>> ELF targets check def_regular for definition in relocatable object.
>> It should be set for all definitions in relocatable objects
>
> Please read the ELF_COMMON_DEF_P comment. I disagree that it is
> *necessary* to set def_regular when a common symbol becomes a
> definition. It may be desirable to set the flag, to simplify other
> code, if there is no place where we want to distinguish symbols that
> actually are definitions in relocatable object files from symbols that
> *become* definitions. I'd need to be convinced of that before
> accepting a patch that sets def_regular for commons.
>
Here is a patch to use ELF_COMMON_DEF_P.
--
H.J.
From fe27591e54758240778acb557f2c786830200bcf Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 5 Apr 2017 09:30:40 -0700
Subject: [PATCH] ELF: Check ELF_COMMON_DEF_P for common symbols
Since common symbols that are turned into definitions don't have the
DEF_REGULAR flag set, we need to check ELF_COMMON_DEF_P for common
symbols.
bfd/
PR ld/19579
PR ld/21306
* elf32-s390.c (elf_s390_finish_dynamic_symbol): Check
ELF_COMMON_DEF_P for common symbols.
* elf64-s390.c (elf_s390_finish_dynamic_symbol): Likewise.
* elf64-x86-64.c (elf_x86_64_relocate_section): Likewise.
* elflink.c (_bfd_elf_merge_symbol): Revert commits
202ac193bbbecc96a4978d1ac3d17148253f9b01 and
07492f668d2173da7a2bda3707ff0985e0f460b6.
ld/
PR ld/19579
PR ld/21306
* testsuite/ld-elf/pr19579a.c (main): Updated.
xxxx
---
bfd/elf32-s390.c | 2 +-
bfd/elf64-s390.c | 2 +-
bfd/elf64-x86-64.c | 3 ++-
bfd/elflink.c | 7 ++-----
ld/testsuite/ld-elf/pr19579a.c | 2 +-
5 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/bfd/elf32-s390.c b/bfd/elf32-s390.c
index fd1bc13..ddb6f5b 100644
--- a/bfd/elf32-s390.c
+++ b/bfd/elf32-s390.c
@@ -3785,7 +3785,7 @@ elf_s390_finish_dynamic_symbol (bfd *output_bfd,
RELATIVE reloc. The entry in the global offset table
will already have been initialized in the
relocate_section function. */
- if (!h->def_regular)
+ if (!(h->def_regular || ELF_COMMON_DEF_P (h)))
return FALSE;
BFD_ASSERT((h->got.offset & 1) != 0);
rela.r_info = ELF32_R_INFO (0, R_390_RELATIVE);
diff --git a/bfd/elf64-s390.c b/bfd/elf64-s390.c
index b5fd05f..fbbf8d6 100644
--- a/bfd/elf64-s390.c
+++ b/bfd/elf64-s390.c
@@ -3582,7 +3582,7 @@ elf_s390_finish_dynamic_symbol (bfd *output_bfd,
RELATIVE reloc. The entry in the global offset table
will already have been initialized in the
relocate_section function. */
- if (!h->def_regular)
+ if (!(h->def_regular || ELF_COMMON_DEF_P (h)))
return FALSE;
BFD_ASSERT((h->got.offset & 1) != 0);
rela.r_info = ELF64_R_INFO (0, R_390_RELATIVE);
diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index 6d92c79..a4048f1 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -4926,7 +4926,8 @@ do_ifunc_pointer:
{
/* Symbol is referenced locally. Make sure it is
defined locally or for a branch. */
- fail = !h->def_regular && !branch;
+ fail = (!(h->def_regular || ELF_COMMON_DEF_P (h))
+ && !branch);
}
else if (!(bfd_link_pie (info)
&& (h->needs_copy || eh->needs_copy)))
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 9bf75c8..c00d712 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1544,16 +1544,13 @@ _bfd_elf_merge_symbol (bfd *abfd,
represent variables; this can cause confusion in principle, but
any such confusion would seem to indicate an erroneous program or
shared library. We also permit a common symbol in a regular
- object to override a weak symbol in a shared object. A common
- symbol in executable also overrides a symbol in a shared object. */
+ object to override a weak symbol in a shared object. */
if (newdyn
&& newdef
&& (olddef
|| (h->root.type == bfd_link_hash_common
- && (newweak
- || newfunc
- || (!olddyn && bfd_link_executable (info))))))
+ && (newweak || newfunc))))
{
*override = TRUE;
newdef = FALSE;
diff --git a/ld/testsuite/ld-elf/pr19579a.c b/ld/testsuite/ld-elf/pr19579a.c
index e4a6eb1..69d0f35 100644
--- a/ld/testsuite/ld-elf/pr19579a.c
+++ b/ld/testsuite/ld-elf/pr19579a.c
@@ -9,7 +9,7 @@ extern int *bar_p (void);
int
main ()
{
- if (foo[0] == 0 && foo == foo_p () && bar[0] == 0 && bar == bar_p ())
+ if (foo[0] == 0 && foo == foo_p () && bar[0] == -1 && bar == bar_p ())
printf ("PASS\n");
return 0;
}
--
2.9.3