[PATCH] bfd: Mark symbols with mismatching types

[PATCH] bfd: Mark symbols with mismatching types

[Egeyar Bagcioglu]

Introduce flag for defined symbols whose type is mismatched with a
dynamic definition. Mark such symbols. Prevent such symbols from being
made dynamic in sparc.

Sparc makes use of dynamic symbols during relocations. Unless detected,
above-mentioned symbols are confused with library symbols by the dynamic
linker.

This change is tested for sparc64-unknown-linux-gnu.

bfd/ChangeLog:

2017-04-11  Egeyar Bagcioglu  <egeyar.bagcioglu@oracle.com>

        * elf-bfd.h (elf_link_hash_entry): Add conflict_with_library.
        * elflink.c (_bfd_elf_merge_symbol): Set conflict_with_library.
        * elfxx-sparc.c (allocate_dynrelocs): Avoid the call to
        bfd_elf_link_record_dynamic_symbol in case of conflict_with_library.
---
 bfd/elf-bfd.h     |    2 ++
 bfd/elflink.c     |    1 +
 bfd/elfxx-sparc.c |    6 ++++--
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index af377ee..150f038 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -213,6 +213,8 @@ struct elf_link_hash_entry
   /* Symbol is defined by a shared library with non-default visibility
      in a read/write section.  */
   unsigned int protected_def : 1;
+  /* Symbol is defined also with a different type in a shared library */
+  unsigned int conflict_with_library : 1;
 
   /* String table index in .dynstr if this is a dynamic symbol.  */
   unsigned long dynstr_index;
diff --git a/bfd/elflink.c b/bfd/elflink.c
index c00d712..7a9979a 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1257,6 +1257,7 @@ _bfd_elf_merge_symbol (bfd *abfd,
       && h->type != STT_NOTYPE
       && !(newfunc && oldfunc))
     {
+      h->conflict_with_library = 1;
       *skip = TRUE;
       return TRUE;
     }
diff --git a/bfd/elfxx-sparc.c b/bfd/elfxx-sparc.c
index c978aad..c4e7b7c 100644
--- a/bfd/elfxx-sparc.c
+++ b/bfd/elfxx-sparc.c
@@ -2310,7 +2310,8 @@ allocate_dynrelocs (struct elf_link_hash_entry *h, void * inf)
 	 Undefined weak syms won't yet be marked as dynamic.  */
       if (h->dynindx == -1
 	  && !h->forced_local
-          && !resolved_to_zero)
+          && !resolved_to_zero
+          && !h->conflict_with_library)
 	{
 	  if (! bfd_elf_link_record_dynamic_symbol (info, h))
 	    return FALSE;
@@ -2422,7 +2423,8 @@ allocate_dynrelocs (struct elf_link_hash_entry *h, void * inf)
 	 Undefined weak syms won't yet be marked as dynamic.  */
       if (h->dynindx == -1
 	  && !h->forced_local
-          && !resolved_to_zero)
+          && !resolved_to_zero
+          && !h->conflict_with_library)
 	{
 	  if (! bfd_elf_link_record_dynamic_symbol (info, h))
 	    return FALSE;
-- 
1.7.1

Re: [PATCH] bfd: Mark symbols with mismatching types

[Alan Modra]

On Wed, Apr 12, 2017 at 10:11:42PM -0700, Egeyar Bagcioglu wrote:
> Introduce flag for defined symbols whose type is mismatched with a
> dynamic definition. Mark such symbols. Prevent such symbols from being
> made dynamic in sparc.
> 
> Sparc makes use of dynamic symbols during relocations. Unless detected,
> above-mentioned symbols are confused with library symbols by the dynamic
> linker.

Please provide a C testcase.  I can see what you're trying to prevent
but the testcase I wrote to investigate the problem on sparc and other
targets does not result in bad dynamic symbols, even though the linker
does hit the code where you are setting conflict_with_library.
Obviously I'm missing some detail.

A testcase is necessary to prove that you really do need a new flag in
elf_link_hash_entry.  If the problem only shows up on sparc, then a
different fix in the sparc backend is likely better.  If the problem
is generic then yes, we may need a new flag, but even then there is
likely a better fix..  I'm not asking for a patch that integrates a
test into the ld testsuite, just a set of C files and gcc command
lines to build it.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] bfd: Mark symbols with mismatching types

[Egeyar Bagcioglu]

On 04/14/2017 01:27 AM, Alan Modra wrote:
> On Wed, Apr 12, 2017 at 10:11:42PM -0700, Egeyar Bagcioglu wrote:
>> Introduce flag for defined symbols whose type is mismatched with a
>> dynamic definition. Mark such symbols. Prevent such symbols from being
>> made dynamic in sparc.
>>
>> Sparc makes use of dynamic symbols during relocations. Unless detected,
>> above-mentioned symbols are confused with library symbols by the dynamic
>> linker.
> Please provide a C testcase.  I can see what you're trying to prevent
> but the testcase I wrote to investigate the problem on sparc and other
> targets does not result in bad dynamic symbols, even though the linker
> does hit the code where you are setting conflict_with_library.
> Obviously I'm missing some detail.
>
> A testcase is necessary to prove that you really do need a new flag in
> elf_link_hash_entry.  If the problem only shows up on sparc, then a
> different fix in the sparc backend is likely better.  If the problem
> is generic then yes, we may need a new flag, but even then there is
> likely a better fix..  I'm not asking for a patch that integrates a
> test into the ld testsuite, just a set of C files and gcc command
> lines to build it.
>
Hello Alan,

There is already a failing test case due to this problem. You can see 
"FAIL: Run pr2404 with PIE" when running ld-elf/shared.exp on sparc. The 
test is for the specific case of having conflicting symbols. 
'_bfd_elf_merge_symbol' currently just skips the merge of such symbols 
without any further action.

However, on sparc backend, we take the liberty of making some symbols 
dynamic while adjusting and optimizing relocations. This is becoming a 
problem in case of such conflicting symbols. A call to a library 
function is causing dynamic linker to see the symbol (of a variable) in 
the executable. The dynamic linker assumes versionless symbol for the 
variable is the first version of the function that it is looking for.

As the sparc backend works as intended for all other symbols, I would 
like to change the treatment of only this case that is already detected 
in the common code (elflink.c). It would be expensive to re-detect such 
interference between symbols in sparc backend: We would need to run each 
symbol against the hash table. Such an act would be redundant and have a 
high complexity. Therefore, we need to somehow mark such symbols in the 
common code.

If your hesitation is regarding adding another flag, I can suggest using 
the "forced_local" flag. This might be acceptable too, because such 
symbols being dynamic would affect the dynamic linker in other 
architectures as well.

If you have any other suggestions, I am open to discuss and learn.

Thanks,
Egeyar

Re: [PATCH] bfd: Mark symbols with mismatching types

[Alan Modra]

On Fri, Apr 14, 2017 at 04:57:18PM +0200, Egeyar Bagcioglu wrote:
> On 04/14/2017 01:27 AM, Alan Modra wrote:
> >On Wed, Apr 12, 2017 at 10:11:42PM -0700, Egeyar Bagcioglu wrote:
> >>Introduce flag for defined symbols whose type is mismatched with a
> >>dynamic definition. Mark such symbols. Prevent such symbols from being
> >>made dynamic in sparc.
> >>
> >>Sparc makes use of dynamic symbols during relocations. Unless detected,
> >>above-mentioned symbols are confused with library symbols by the dynamic
> >>linker.
> >Please provide a C testcase.  I can see what you're trying to prevent
> >but the testcase I wrote to investigate the problem on sparc and other
> >targets does not result in bad dynamic symbols, even though the linker
> >does hit the code where you are setting conflict_with_library.
> >Obviously I'm missing some detail.
> >
> >A testcase is necessary to prove that you really do need a new flag in
> >elf_link_hash_entry.  If the problem only shows up on sparc, then a
> >different fix in the sparc backend is likely better.  If the problem
> >is generic then yes, we may need a new flag, but even then there is
> >likely a better fix..  I'm not asking for a patch that integrates a
> >test into the ld testsuite, just a set of C files and gcc command
> >lines to build it.
> >
> Hello Alan,
> 
> There is already a failing test case due to this problem. You can see "FAIL:
> Run pr2404 with PIE" when running ld-elf/shared.exp on sparc. The test is
> for the specific case of having conflicting symbols. '_bfd_elf_merge_symbol'
> currently just skips the merge of such symbols without any further action.

OK, other targets pass this test.  x86_64-linux and powerpc64le-linux
I've tested natively myself.  This says you do not need a new flag,
but rather a sparc backend change..

> However, on sparc backend, we take the liberty of making some symbols
> dynamic while adjusting and optimizing relocations.

Demonstrating that it isn't safe to copy x86_64 backend code.  The
testcase shows the sparc backend making bfd_link_hash_defined symbols
dynamic in allocate_dynrelocs.  That's just plain wrong.  As the
comment says "Undefined weak syms won't yet be marked as dynamic".
You should *only* be changing bfd_link_hash_undefweak there.  Fix that
and you won't need conflict_with_library.  Note that the fix likely
will require changes to _bfd_sparc_elf_relocate_section too.

-- 
Alan Modra
Australia Development Lab, IBM