Bug 19938 - objcopy breaks sh_link and sh_info of SHT_SUNW_LDYNSYM and SHT_SUNW_symsort sections
Summary: objcopy breaks sh_link and sh_info of SHT_SUNW_LDYNSYM and SHT_SUNW_symsort s...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: 2.27
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-11 11:22 UTC by Rainer Orth
Modified: 2016-05-09 11:08 UTC (History)
6 users (show)

See Also:
Host: *-*-solaris2.*
Target: *-*-solaris2.*
Build: *-*-solaris2.*
Last reconfirmed:


Attachments
hello executable (2.46 KB, application/octet-stream)
2016-04-11 13:29 UTC, Rainer Orth
Details
Proposed patch (329 bytes, patch)
2016-04-11 14:55 UTC, Nick Clifton
Details | Diff
Revised patch (2.25 KB, patch)
2016-04-12 11:01 UTC, Nick Clifton
Details | Diff
Update readelf with Solaris specific information (1.59 KB, patch)
2016-04-12 11:04 UTC, Nick Clifton
Details | Diff
Revised patch (7.07 KB, patch)
2016-04-13 16:10 UTC, Nick Clifton
Details | Diff
Proposed extra patch (1.32 KB, patch)
2016-04-20 10:43 UTC, Nick Clifton
Details | Diff
Proposed extra patch (2.54 KB, patch)
2016-04-27 16:08 UTC, Nick Clifton
Details | Diff
extended patch (6.10 KB, patch)
2016-04-28 12:37 UTC, Nick Clifton
Details | Diff
Proposed extra patch (324 bytes, patch)
2016-05-06 15:30 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rainer Orth 2016-04-11 11:22:27 UTC
objcopy invalidates (at least) two Solaris-specific sections:

* hello is a trivial hello world program linked with Solaris ld.

ro@lokon 1274 > objcopy hello hello.copy
ro@lokon 1275 > elfdump hello > /dev/null
ro@lokon 1276 > elfdump hello.copy > /dev/null
hello.copy: .SUNW_ldynsym: sh_link: 0: does not point to a valid section
hello.copy: .SUNW_dynsymsort: sh_link: 0: does not point to a valid section
ro@lokon 1277 > elfdump -N .SUNW_ldynsym -N .SUNW_dynsymsort -c hello

Section Header[3]:  sh_name: .SUNW_ldynsym
    sh_addr:      0x80501ac       sh_flags:   [ SHF_ALLOC ]
    sh_size:      0xe0            sh_type:    [ SHT_SUNW_LDYNSYM ]
    sh_offset:    0x1ac           sh_entsize: 0x10 (14 entries)
    sh_link:      5               sh_info:    14
    sh_addralign: 0x4       

Section Header[8]:  sh_name: .SUNW_dynsymsort
    sh_addr:      0x80507ec       sh_flags:   [ SHF_ALLOC ]
    sh_size:      0x2c            sh_type:    [ SHT_SUNW_symsort ]
    sh_offset:    0x7ec           sh_entsize: 0x4 (11 entries)
    sh_link:      3               sh_info:    0
    sh_addralign: 0x4       
ro@lokon 1278 > elfdump -N .SUNW_ldynsym -N .SUNW_dynsymsort -c hello.copy

Section Header[3]:  sh_name: .SUNW_ldynsym
    sh_addr:      0x80501ac       sh_flags:   [ SHF_ALLOC ]
    sh_size:      0xe0            sh_type:    [ SHT_SUNW_LDYNSYM ]
    sh_offset:    0x1ac           sh_entsize: 0x10 (14 entries)
    sh_link:      0               sh_info:    0
    sh_addralign: 0x4       

Section Header[8]:  sh_name: .SUNW_dynsymsort
    sh_addr:      0x80507ec       sh_flags:   [ SHF_ALLOC ]
    sh_size:      0x2c            sh_type:    [ SHT_SUNW_symsort ]
    sh_offset:    0x7ec           sh_entsize: 0x4 (11 entries)
    sh_link:      0               sh_info:    0
    sh_addralign: 0x4       

Obviously objcopy loses the sh_link and sh_info fields, which are described
in the Oracle Solaris 11.3 Linkers and Libraries Guide, Ch. 13, Object File
Format, Table 13-9  ELF sh_link and sh_info Interpretation:

  http://docs.oracle.com/cd/E53394_01/html/E54813/chapter6-94076.html#scrolltoc

  Rainer
Comment 1 Nick Clifton 2016-04-11 13:21:57 UTC
Hi Rainer,

  Please could you upload a copy of the original hello executable so that I can run some tests ?

Cheers
  Nick
Comment 2 Rainer Orth 2016-04-11 13:29:27 UTC
Created attachment 9177 [details]
hello executable
Comment 3 Nick Clifton 2016-04-11 14:55:25 UTC
Created attachment 9178 [details]
Proposed patch

Hi Guys,

  Could someone check out this potential patch please and let me know if there are any problems with it ?

Cheers
  Nick
Comment 4 Ali Bahrami 2016-04-11 15:48:24 UTC
I don't know the bfd code, so please forgive a probably
off base comment...

This fix seems to be giving the section a pass based
on its section type being in the OS-specific range.
That would work, but is there a reason to not simply
translate any non-zero sh_link?

The Solaris strip always translates sh_link. It only
translates sh_info in a handful of cases that it specifically
knows are section indexes, and otherwise leaves it unaltered.
So far, this seems to have been OK (no complaints that I know
about), and is a bit more general.

In any event, thanks for this fix.
Comment 5 Nick Clifton 2016-04-11 16:04:58 UTC
H Ali,

> This fix seems to be giving the section a pass based
> on its section type being in the OS-specific range.

Correct.

> That would work, but is there a reason to not simply
> translate any non-zero sh_link?

Yes - for OS and Application specific sections the BFD library
does not know the meaning/interpretation of the sh_link and sh_info
fields, so it does not know how to translate them.

The pre-patched code would just set these fields to zero, which is
pretty much guaranteed to be wrong.  The patched code copies the
values from the input binary, which will work provided that all
of the sections from the input binary are preserved in the output
binary.  (Well probably - the sh_link and sh_info fields might be
datestamps or object counts for all we know, so that even preserving
their values is wrong).

If however the binary is being stripped for example, then copying the 
link and info values is probably the wrong thing to do as well.
In this case what possibly should happen is that the code should try 
to work out where the input section whose number matches the sh_info 
or sh_link fields number has been mapped to, and use these new values.
But that presumes that the numbers are section indicies, so it could 
still be wrong.

The truly correct thing to do would be to defer this decision to target
specific code that understands the meaning of these OS specific sections.
But that would require a lot of work...

Cheers
  Nick
Comment 6 Cary Coutant 2016-04-11 16:14:31 UTC
> > That would work, but is there a reason to not simply
> > translate any non-zero sh_link?
> 
> Yes - for OS and Application specific sections the BFD library
> does not know the meaning/interpretation of the sh_link and sh_info
> fields, so it does not know how to translate them.

The sh_link field, when non-zero, is always a section index. It should be translated if the referenced section is still in the new file, or replaced with zero if not.

The sh_info field is a section index for SHT_REL and SHT_RELA sections, and for any section that has the SHF_INFO_LINK flag set. We added that flag specifically so that tools like objcopy could update the field without knowing any vendor-specific section types.

-cary
Comment 7 Ali Bahrami 2016-04-11 16:27:18 UTC
   I understand your concerns. Our assumption is that sh_link
is always 0, or a section index, while the meaning of sh_info
is  somewhat more variable, and may or may not be. This is
ancient "From New Jersey" code, which is somewhat short of
a proof of correctness. In any event special case handling of
sh_info is clearly necessary.

With that in mind, this proposed fix is a step in the right
direction, but it's not enough, because we really need the
section indexes to be translated correctly for our OSABI
sections. In the examples provided by Rainer for instance,
the sh_link for both of those sections is definitely a section
index, and so in a situation where sections are added or
removed, preserving them without translating them is going to
be wrong, and perhaps even worse that just zeroing.

Can I ask you to add the necessary os-specific translations?
The list is pretty short, and can be found here in Table 13-9
(ELF sh_link and sh_info interpretation):

    https://docs.oracle.com/cd/E53394_01/html/E54813/chapter6-94076.html#scrolltoc

You will already have the gABI cases covered, so you only need
updates for the SHT_SUNW_ cases. The numeric values for these
types are as follows:

#define SHT_SUNW_ancillary      0x6fffffee
#define SHT_SUNW_cap            0x6ffffff5
#define SHT_SUNW_capinfo        0x6ffffff0
#define SHT_SUNW_symsort        0x6ffffff1
#define SHT_SUNW_tlssort        0x6ffffff2
#define SHT_SUNW_LDYNSYM        0x6ffffff3
#define SHT_SUNW_move           0x6ffffffa
#define SHT_SUNW_COMDAT         0x6ffffffb
#define SHT_SUNW_syminfo        0x6ffffffc
#define SHT_SUNW_verdef         0x6ffffffd
#define SHT_GNU_verdef          SHT_SUNW_verdef
#define SHT_SUNW_verneed        0x6ffffffe
#define SHT_GNU_verneed         SHT_SUNW_verneed
#define SHT_SUNW_versym         0x6fffffff
#define SHT_GNU_versym          SHT_SUNW_versym

It would be correct to preserve and not alter the values
for any other section types in the OS-specific range.
Comment 8 Ali Bahrami 2016-04-11 16:34:54 UTC
My understand of sh_link and sh_info is the same as
what Cary outlined above, with one comment. We've seen
cases were sh_info should have been treated as a section
index, where SHF_INFO_LINK was not set. This is wrong,
but we still do have some code that "knows" what sh_info
contains without regard for SHF_INFO_LINK, just to
work around that.

I'd be content for objcopy/strip to follow the rule
Cary outlined though --- if we're not setting SHF_INFO_LINK
properly, that's a bug we can deal with ourselves (and I
think we're OK in that regard).
Comment 9 Nick Clifton 2016-04-12 11:01:18 UTC
Created attachment 9181 [details]
Revised patch

Hi Guys,

  Right - here is a revised patch.  Could someone try it out please ?

  This version of the patch tries to match up the sh_link and sh_info fields
  in special output sections with matching equivalents based upon the input
  sections.  If it fails to find a match, it issues an error message.

  I have to consider this patch to only be an interim solution however as
  there appears to be another bug:  The SHF_STRINGS flag bit used by, for
  example, the .dynstr section, does not appear to be copied correctly.  This
  is for a standard ELF section, which I would have expected to be copied
  correctly.  So more investigation is needed in order to find out what is
  going on.

Cheers
  Nick
Comment 10 Nick Clifton 2016-04-12 11:04:00 UTC
Created attachment 9182 [details]
Update readelf with Solaris specific information

Hi Guys,

  This is a supplimental patch that I intend to check in at some point.  It
  updates readelf so that it can recognise Solaris specific values in the
  hello executable.  There are other values specific to Solaris that this
  patch does not address, but I would prefer to wait for a test binary that
  uses them before adding the necessary code.

Cheers
  Nick
Comment 11 Nick Clifton 2016-04-13 16:10:02 UTC
Created attachment 9189 [details]
Revised patch

Hi Guys,

  Right, here is a third version of the patch.  This revision adds a fix for
  setting the SHF_STRINGS bit in the flags of the .strtab and .shstrtab sections
  (for Solaris only.  For other OSes the flags are left alone).

  It also adds a framework for allowing individual backends to set the sh_info
  and sh_link fields of special sections.  Dummy implementations are provided
  for Solaris targets, although with this version of the patch these functions
  do not actually do anything.

  I think that this version of the patch is sufficient for now.  It needs 
  someone with more experience of Solaris than me to actually implement the 
  target specific info/link function, and the current generic code does a 
  sufficiently good job to allow an objcopy'ed executable to still work.

  Any comments ?

Cheers
  Nick
Comment 12 Ali Bahrami 2016-04-13 17:37:00 UTC
I've sent an email with a more detailed reply.

I think that propagating SHF_STRINGS, if set, should be
done for all platforms, and not just for Solaris. We do
seem to be unique in setting it today, but note that
its a generic ELF flag, and it would be legal on any
platform.

Given support for the rules Cary gave earlier, I don't
believe that Solaris needs the special back end handling
that this patch adds. I don't know if it would be useful
on other platforms, or whether its nice to have as a bit
of insurance for future issues.
Comment 13 cvs-commit@gcc.gnu.org 2016-04-14 11:05:53 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=84865015459b4e9e8ac67f9b91617fbd856d5119

commit 84865015459b4e9e8ac67f9b91617fbd856d5119
Author: Nick Clifton <nickc@redhat.com>
Date:   Thu Apr 14 12:04:09 2016 +0100

    Fix copying Solaris binaries with objcopy.
    
    	PR target/19938
    bfd	* elf-bbfd.h (struct elf_backend_data): New field:
    	elf_strtab_flags.
    	New field: elf_backend_set_special_section_info_and_link
    	* elfxx-target.h (elf_backend_strtab_flags): Define if not already
    	defined.
    	(elf_backend_set_special_section_info_and_link): Define if not
    	already defined.
    	(elfNN_bed): Use elf_backend_set_special_section_info_and_link and
    	elf_backend_strtab_flags macros to initialise fields in structure.
    	* elf.c (_bfd_elf_make_section_from_shdr): Check for SHF_STRINGS
    	being set even if SHF_MERGE is not set.
    	(elf_fake_sections): Likewise.
    	(section_match): New function.  Matches two ELF sections based
    	upon fixed characteristics.
    	(find_link): New function.  Locates a section in a BFD that
    	matches a section in a different BFD.
    	(_bfd_elf_copy_private_bfd_data): Copy the sh_info and sh_link
    	fields of reserved sections.
    	(bfd_elf_compute_section_file_positions): Set the flags for the
    	.shstrtab section based upon the elf_strtab_flags field in the
    	elf_backend_data structure.
    	(swap_out_syms): Likewise for the .strtab section.
    	* elflink.c (bfd_elf_final_link): Set the flags for the
    	.strtab section based upon the elf_strtab_flags field in the
    	elf_backend_data structure.
    	* elf32-i386.c (elf32_i386_set_special_info_link): New function.
    	(elf_backend_strtab_flags): Set to SHF_STRINGS for Solaris
    	targets.
    	(elf_backend_set_special_section_info_and_link): Define for
    	Solaris targets.
    	* elf32-sparc.c: Likewise.
    	* elf64-x86-64.c: Likewise.
    
    binutils* testsuite/binutils-all/i386/compressed-1b.d: Allow for the
    	string sections possibly having the SHF_STRINGS flag bit set.
    	* testsuite/binutils-all/i386/compressed-1c.d: Likewise.
    	* testsuite/binutils-all/readelf.s: Likewise.
    	* testsuite/binutils-all/readelf.s-64: Likewise.
    	* testsuite/binutils-all/x86-64/compressed-1b.d: Likewise.
    	* testsuite/binutils-all/x86-64/compressed-1c.d: Likewise.
    
    gas	* testsuite/gas/i386/ilp32/x86-64-unwind.d: Allow for the string
    	sections possibly having the SHF_STRINGS flag bit set.
    	* testsuite/gas/i386/x86-64-unwind.d: Likewise.
Comment 14 Nick Clifton 2016-04-14 11:21:15 UTC
Right - I have checked in the third version of the patch.

I am not planning on taking this any further for now.  I think that objcopy should work in most situations, but if any further tests can be found that
demonstrate problems then please feel free to update this issue.
Comment 15 Nick Clifton 2016-04-20 10:43:38 UTC
Created attachment 9206 [details]
Proposed extra patch

Hi Ali, Hi Rainer.

  Please try out the extra patch on top of the latest mainline sources.  (I have
  not checked it in as I would like to be a bit more certain this time that it
  does actually work this time...)

  The problem, I think, was that some of the input sections had the 
  SHF_INFO_LINK flag (correctly) set, but the corresponding output sections did 
  not.  This meant that the code that tried to match input sections to output 
  sections was rejecting a match with the proper output section, because of a 
  flag mistmatch.  So no matching section was found and no copying of the info 
  and link fields was attempted.  The patch fixes this problem my masking out
  the SHF_INFO_LINK bit when comparing sections.  It also makes sure that if
  the input section had the SHF_INFO_LINK flag set, and a match could be found,
  that the output section has the flag set too.

  This problem pointed out another bug to me - the target specific function 
  for setting the info and link fields would only be called if this match could
  be found.  Really it should have a chance to set the fields even if no match
  could be found.  The patch takes care of this problem too.

  I have tested the problem by objcopy'ing libc.so.1 and comparing the output,
  and by stripping libc.so.1 and comparing the output.  The results look better
  but they are still not perfect.  The .SUNW_version section has its sh_entsize
  field set to 0 in the copied/stripped executables for example.  I do not know
  if this is important.

  I also do not know anything about Solaris capabilities, so I am not sure if
  these sections are being copied correctly.

  Anyway please try out the patch and let me know what else needs to be done.

Cheers
  Nick
Comment 16 Nick Clifton 2016-04-20 10:48:16 UTC
Hi Ali, Hi Rainer.

  [Revised text with typos fixed.  I really should make more of an effort
   to proofread these things].
---------------------------------------------------------------------------
  Please try out the extra patch on top of the latest mainline sources.  (I have
  not checked it in as I would like to be a bit more certain this time that it
  does actually work).

  The problem, I think, was that some of the input sections had the 
  SHF_INFO_LINK flag (correctly) set, but the corresponding output sections did 
  not.  This meant that the code that tried to match input sections to output 
  sections was rejecting a match, because of a flag mistmatch.  So no matching 
  section was found and no copying of the info and link fields took place.  The
  patch fixes this problem by masking out the SHF_INFO_LINK bit when comparing
  sections.  It also makes sure that if the input section had the SHF_INFO_LINK
  flag set, and a match could be found, that the output section has the flag
  set too.

  This problem pointed out another bug to me - the target specific function 
  for setting the info and link fields would only be called if this match could
  be found.  Really it should have a chance to set the fields even if no match
  could be found.  The patch takes care of this problem too.

  I have tested the problem by objcopy'ing libc.so.1 and comparing the output,
  and by stripping libc.so.1 and comparing the output.  The results look better
  but they are still not perfect.  The .SUNW_version section has its sh_entsize
  field set to 0 in the copied/stripped executables for example.  I do not know
  if this is important.

  I also do not know anything about Solaris capabilities, so I am not sure if
  these sections are being copied correctly.

  Anyway please try out the patch and let me know what else needs to be done.
Comment 17 Thomas Preud'homme 2016-04-27 10:48:18 UTC
Hi Nick,

I traced a gdb try_catch test regression to this commit. The reason is that objcopy breaks the sh_link for .ARM.exidx sections. I believe this is due to the set of fields checked by section_match not identifying a section uniquely. The case I'm seeing (ctype.o from libstdc++ compiled for arm-none-eabi) has the following section table extract before running objcopy:

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
(...)
  [10] .text._ZNKSt5ctypeIcE8do_widenEc PROGBITS        00000000 000084 000004 00 AXG  0   0  4
  [11] .ARM.extab.text._ZNKSt5ctypeIcE8do_widenEc PROGBITS        00000000 000088 000000 00  AG  0   0  1
  [12] .ARM.exidx.text._ZNKSt5ctypeIcE8do_widenEc ARM_EXIDX       00000000 000088 000008 00 ALG 10   0  4
  [13] .rel.ARM.exidx.text._ZNKSt5ctypeIcE8do_widenEc REL             00000000 01144c 000008 08   I 141  12  4
  [14] .text._ZNKSt5ctypeIcE9do_narrowEcc PROGBITS        00000000 000090 000004 00 AXG  0   0  4
  [15] .ARM.extab.text._ZNKSt5ctypeIcE9do_narrowEcc PROGBITS        00000000 000094 000000 00  AG  0   0  1
  [16] .ARM.exidx.text._ZNKSt5ctypeIcE9do_narrowEcc ARM_EXIDX       00000000 000094 000008 00 ALG 14   0  4
  [17] .rel.ARM.exidx.text._ZNKSt5ctypeIcE9do_narrowEcc REL             00000000 011454 000008 08   I 141  16  4
(...)

As is required for .ARM.exidx sections, they link back to the code section they are related to. Now after running a straight objcopy without any option, I get:

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
(...)
  [10] .text._ZNKSt5ctypeIcE8do_widenEc PROGBITS        00000000 000084 000004 00 AXG  0   0  4
  [11] .ARM.extab.text._ZNKSt5ctypeIcE8do_widenEc PROGBITS        00000000 000088 000000 00  AG  0   0  1
  [12] .ARM.exidx.text._ZNKSt5ctypeIcE8do_widenEc ARM_EXIDX       00000000 000088 000008 00 ALG 14   0  4
  [13] .rel.ARM.exidx.text._ZNKSt5ctypeIcE8do_widenEc REL             00000000 01144c 000008 08   I 141  12  4
  [14] .text._ZNKSt5ctypeIcE9do_narrowEcc PROGBITS        00000000 000090 000004 00 AXG  0   0  4
  [15] .ARM.extab.text._ZNKSt5ctypeIcE9do_narrowEcc PROGBITS        00000000 000094 000000 00  AG  0   0  1
  [16] .ARM.exidx.text._ZNKSt5ctypeIcE9do_narrowEcc ARM_EXIDX       00000000 000094 000008 00 ALG 10   0  4
  [17] .rel.ARM.exidx.text._ZNKSt5ctypeIcE9do_narrowEcc REL             00000000 011454 000008 08   I 141  16  4
(...)

See how wideenc .ARM.exidx section now link to narrowEcc code section. You'll notice that the code section both have the same type (PROGBIGS which is logical for 2 text sections), flags (AXG, expected for 2 code sections), alignment (4, expected for 2 text sections), size (4, quite likely when compiling a big file with -ffunction-sections) and entry size (0, expected for 2 text sections).

I therefore believe that the set of fields tested in not enough to identify a section uniquely. We could add address and offset which is more likely to yield correct results (I don't know if two sections can share the same address and offset but I guess if they do they would have different type and flags).

Best regards,

Thomas
Comment 18 Nick Clifton 2016-04-27 16:08:41 UTC
Created attachment 9221 [details]
Proposed extra patch

Hi Thomas,

  Ah - this is exactly the kind of situation that I thought might happen, and the
  reason why I created the new backend function.

  Please try out this revised patch which I hope will fix the problem.  (For the
  ARM that is, this patch does not address the ABI and p_addr problems that still
  exist with Solaris targets).

Cheers
  Nick
Comment 19 Nick Clifton 2016-04-27 16:15:01 UTC
(In reply to Thomas Preud'homme from comment #17)

> I therefore believe that the set of fields tested in not enough to identify
> a section uniquely.

True.

>  We could add address and offset which is more likely to
> yield correct results (I don't know if two sections can share the same
> address and offset but I guess if they do they would have different type and
> flags).

Or if they are overlay sections.

The real problem is that at the time that the code to set the sh_link and sh_info fields is called, the output string tables have not been assigned to the output bfd.  So it is very hard to find the names of the output sections.  
If we did have the output section names available then we could compare the 
EXIDX section name with the various .text.* section names and find a match.  
(Maybe I should try harder to find a way to obtain those section names).

For now, the heuristic in the patch that I uploaded simply associates the EXIDX
section with the nearest executable section before it in the section table.  
This works provided that the sections have not been scrambled or sorted 
somehow.  I have my fingers crossed that this will be enough...

Cheers
  Nick
Comment 20 Thomas Preud'homme 2016-04-27 16:32:37 UTC
(In reply to Nick Clifton from comment #18)
> Created attachment 9221 [details]
> Proposed extra patch
> 
> Hi Thomas,
> 
>   Ah - this is exactly the kind of situation that I thought might happen,
> and the
>   reason why I created the new backend function.
> 
>   Please try out this revised patch which I hope will fix the problem.  (For
> the
>   ARM that is, this patch does not address the ABI and p_addr problems that
> still
>   exist with Solaris targets).

It does in this case but I'm not a big fan of the approach. Why cannot we have a mapping table from input to output asection and use it instead of doing a loop over the output section to find one that looks like a given input section? That would alleviate the need for the strings and should be more robust.

Best regards,

Thomas
Comment 21 Nick Clifton 2016-04-28 12:37:33 UTC
Created attachment 9222 [details]
extended patch

Right, here is another revision of the patch.  In this version:

  * The EI_ABIVERSION field of the input file is copied to the output file.

  * The section header copying code now makes two passes over the input
    sections, trying to find a match between an input section and an output
    section.  The first pass looks for a direct mapping between the sections,
    based upon the pointers stored in the structures.  (Thanks to Thomas for
    suggesting this).  The second pass tries to match up sections based upon
    their characteristics (address, type, flags, etc).  If both passes fail
    the backend copying function is given one last chance to set the fields,
    but with a NULL matching input section.

  * Updates readelf to report Solaris specific program header types.

  * General code tidying and cleanup.

Ali, Rainer. Thomas - please could you try out this version and let me know if
you are happy with it.

One thing that this patch does not do is zero the p_paddr fields of the program
headers for Solaris targets.  I did look at doing this, but it would a lot of
hacking on very complicated, fragile code in the BFGD library.  Something that
I think would be very likely to introduce new bugs.

Cheers
  Nick
Comment 22 Thomas Preud'homme 2016-04-28 15:53:28 UTC
(In reply to Nick Clifton from comment #21)
> Created attachment 9222 [details]
> extended patch
> 
> Right, here is another revision of the patch.  In this version:
> 
>   * The EI_ABIVERSION field of the input file is copied to the output file.
> 
>   * The section header copying code now makes two passes over the input
>     sections, trying to find a match between an input section and an output
>     section.  The first pass looks for a direct mapping between the sections,
>     based upon the pointers stored in the structures.  (Thanks to Thomas for
>     suggesting this).  The second pass tries to match up sections based upon
>     their characteristics (address, type, flags, etc).  If both passes fail
>     the backend copying function is given one last chance to set the fields,
>     but with a NULL matching input section.
> 
>   * Updates readelf to report Solaris specific program header types.
> 
>   * General code tidying and cleanup.
> 
> Ali, Rainer. Thomas - please could you try out this version and let me know
> if
> you are happy with it.
> 
> One thing that this patch does not do is zero the p_paddr fields of the
> program
> headers for Solaris targets.  I did look at doing this, but it would a lot of
> hacking on very complicated, fragile code in the BFGD library.  Something
> that
> I think would be very likely to introduce new bugs.
> 
> Cheers
>   Nick

Hi Nick,

As for the previous version, the patch does fix the problem we have at hand. I'm glad my suggestion was useful to you.

I skimmed over your patch as I was curious to learn about objcopy and I realized that it is not working exactly as I thought. I had in mind that setup_section () in binutils/objcopy.c would create a record input section number -> output section number for each section it copy and that we could use this to skip find_link altogether. Seeing how old is binutils I guess this must be impossible for a reason I'm missing.

Thank you for such a quick fix.

Best regards,

Thomas
Comment 23 Ali Bahrami 2016-04-28 20:26:30 UTC
> Ali, Rainer. Thomas - please could you try out this version and let me know if
> you are happy with it.
>
> One thing that this patch does not do is zero the p_paddr fields of the program
> headers for Solaris targets.  I did look at doing this, but it would a lot of
> hacking on very complicated, fragile code in the BFGD library.  Something that
> I think would be very likely to introduce new bugs.

Hi Nick,

   Rainer and I just compared notes, and we're happy with it.
These little breakages have been a chronic pain in the neck
for us, and it's going to be really helpful for us once we
pull in this patch. Thanks for all the work you put in to help
us with this.

For the record, I don't believe that Solaris ever sets
p_paddr, or looks at it, so the fix for this could probably
be as simple as "if (solaris) phdr->p_paddr = 0". On the
other hand, it's purely cosmetic. The values you're writing
there seem to correspond to something sensible (same as
p_vaddr), and there's really no reason to put any extra
work into it. Leave it alone.

Thanks again...

- Ali
Comment 24 cvs-commit@gcc.gnu.org 2016-04-29 08:26:08 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=5522f910cb539905d6adfdceab208ddfa5e84557

commit 5522f910cb539905d6adfdceab208ddfa5e84557
Author: Nick Clifton <nickc@redhat.com>
Date:   Fri Apr 29 09:24:42 2016 +0100

    Enhance support for copying and stripping Solaris and ARM binaries.
    
    	PR 19938
    bfd	* elf-bfd.h (struct elf_backend_data): Rename
    	elf_backend_set_special_section_info_and_link to
    	elf_backend_copy_special_section_fields.
    	* elfxx-target.h: Likewise.
    	* elf.c (section_match): Ignore the SHF_INFO_LINK flag when
    	comparing section flags.
    	(copy_special_section_fields): New function.
    	(_bfd_elf_copy_private_bfd_data): Copy the EI_ABIVERSION field.
    	Perform two scans over special sections.  The first one looks for
    	a direct mapping between the output section and an input section.
    	The second scan looks for a possible match based upon section
    	characteristics.
    	* elf32-arm.c (elf32_arm_copy_special_section_fields): New
    	function.  Handle setting the sh_link field of SHT_ARM_EXIDX
    	sections.
    	* elf32-i386.c (elf32_i386_set_special_info_link): Rename to
    	elf32_i386_copy_solaris_special_section_fields.
    	* elf32-sparc.c (elf32_sparc_set_special_section_info_link):
    	Rename to elf32_sparc_copy_solaris_special_section_fields.
    	* elf64-x86-64.c (elf64_x86_64_set_special_info_link): Rename to
    	elf64_x86_64_copy_solaris_special_section_fields.
    
    binutils* readelf.c (get_solaris_segment_type): New function.
    	(get_segment_type): Call it.
Comment 25 Nick Clifton 2016-04-29 08:27:05 UTC
Ok - I have applied the revised patch and I am closing this PR ... for now.

Cheers
  Nick
Comment 26 cctsai57 2016-05-06 09:17:11 UTC
Hi,

This fix makes gcc's libstdc++ testsuite many fails for the ARM target.
I try to simplify them by the following flow:

# Two source files. The first is simplified from gcc source:
# libstdc++-v3/src/shared/hashtable-aux.cc:
$ cat -n hashtable-aux.cc 
     1	extern const unsigned long __prime_list[] = 
     2	{
     3	    2ul, 3ul, 5ul, 7ul, 11ul, 13ul, 17ul, 19ul, 23ul, 29ul, 31ul,
     4	    37ul, 41ul, 43ul, 47ul, 53ul, 59ul, 61ul, 67ul, 71ul, 73ul
     5	};

$ cat -n b.cc 
     1	#include <iostream>
     2	
     3	extern const unsigned long __prime_list[];
     4	
     5	int main()
     6	{
     7	  std::cout << "__prime_list[]\n";
     8	  for (int i = 0; i < 20; ++i)
     9	    std::cout << __prime_list[i] << ", ";
    10	  std::cout << "\n";
    11	
    12	  return 0;
    13	}

$ arm-linux-gnueabihf-g++ -shared -fpic hashtable-aux.cc -o hashtable-aux.so
$ arm-linux-gnueabihf-g++ b.cc ./hashtable-aux.so

# Run under the QEMU
$ export QEMU_LD_PREFIX=$(arm-linux-gnueabihf-g++ -print-sysroot)
$ qemu-arm ./a.out
__prime_list[]
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 

The items in the __prime_list[] are all wrong zero!  I guess that the problem
is the lost R_ARM_COPY for __prime_list.

See a.out ELF:

$ arm-linux-gnueabihf-readelf --relocs a.out
Relocation section '.rel.dyn' at offset 0x668 contains 2 entries:
 Offset     Info    Type            Sym.Value  Sym. Name
00020d0c  00001615 R_ARM_GLOB_DAT    00000000   __gmon_start__
00020d70  00000d14 R_ARM_COPY        00020d70   _ZSt4cout@GLIBCXX_3.4


Befor this fix, the old a.out ELF is:

$ arm-linux-gnueabihf-readelf --relocs a.out
Relocation section '.rel.dyn' at offset 0x668 contains 3 entries:
 Offset     Info    Type            Sym.Value  Sym. Name
00020d14  00001615 R_ARM_GLOB_DAT    00000000   __gmon_start__
00020d20  00000a14 R_ARM_COPY        00020d20   __prime_list
00020d78  00000d14 R_ARM_COPY        00020d78   _ZSt4cout@GLIBCXX_3.4


The tool's version:
  gcc: 6.1.1
  binutils: 2.26.51 (master branch)
  glibc: 2.23
Comment 27 Nick Clifton 2016-05-06 15:30:24 UTC
Created attachment 9239 [details]
Proposed extra patch

(In reply to cctsai57 from comment #26)
 
> This fix makes gcc's libstdc++ testsuite many fails for the ARM target.
> I try to simplify them by the following flow:
 
I think that this is due to a snafu on my part, where I accidentally included
an experimental patch for a different bug in the comment for this PR.

Please could you try out this patch to revert that accidental commit and let
me know if it works for you.

Cheers
  Nick
Comment 28 cctsai57 2016-05-09 04:16:43 UTC
(In reply to Nick Clifton from comment #27)
> Created attachment 9239 [details]
> Proposed extra patch

This attachment 9239 [details] patch works fine for me. I rerun the gcc's testsuite
for the ARM target, and the previous new 130 libstdc++ FAILs now are PASS.
Thanks.

Cheers
  cctsai
Comment 29 cvs-commit@gcc.gnu.org 2016-05-09 11:08:31 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=73597c183c78ed0bea291897de6d8867ec640208

commit 73597c183c78ed0bea291897de6d8867ec640208
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon May 9 12:07:32 2016 +0100

    Revert accidental commit.
    
    	PR 19938
    	* elf32-arm.c (elf32_arm_adjust_dynamic_symbol): Revert accidental
    	commit.