This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][ELF] -O binary: use LMA instead of sh_offset to decide where to write section contents
ClosedPublic

Authored by MaskRay on Dec 4 2019, 2:42 PM.

Details

Summary

.text sh_address=0x1000 sh_offset=0x1000
.data sh_address=0x3000 sh_offset=0x2000

In an objcopy -O binary output, the distance between two sections equal
their LMA differences (0x3000-0x1000), instead of their sh_offset
differences (0x2000-0x1000). This patch changes our behavior to match
GNU.

This rule gets more complex when the containing PT_LOAD has
p_vaddr!=p_paddr. GNU objcopy essentially computes
sh_offset-p_offset+p_paddr for each candidate section, and removes the
gap before the first address.

Added two tests to binary-paddr.test to catch the compatibility problem.

Diff Detail

Event Timeline

MaskRay created this revision.Dec 4 2019, 2:42 PM
MaskRay updated this revision to Diff 232219.Dec 4 2019, 2:45 PM

Delete unneeded parts of the test

Harbormaster completed remote builds in B41889: Diff 232219.

I'd like to do some experiments with GNU objcopy on this subject before reviewing, but my Linux machine is unavailable until some time next week, so I'm going to have to wait until then. Sorry!

No hurry! We have an objcopy -I ihex -O binary user. I noticed the difference (when the difference between sh_address'es does not equal the difference between sh_offset's) while migrating them to use llvm-objcopy. They have switched to a checked-in binary instead as using objcopy does not make their build any better (the original ihex is an opaque binary anyway). I posted the patch because it actually seems to simplify code and may probably address problems of future users.

I've been playing around with the YAML from the test to experiment with other situations using GNU objcopy, and I'm not convinced that your statement about the algorithm is quite accurate, and indeed I see behaviour differences between GNU objcopy and llvm-objcopy with this patch. Unfortunately, I haven't been able to figure out what the actual algorithm is, so it's possible I'm just misunderstanding something. Here is an example I used:

Input:

--- !ELF
FileHeader:
  Class:           ELFCLASS64
  Data:            ELFDATA2LSB
  Type:            ET_EXEC
  Machine:         EM_X86_64
Sections:
  - Name:            .text
    Type:            SHT_PROGBITS
    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
    Address:         0x1000
    AddressAlign:    0x0000000000001000
    Content:         "c3c3c3c3"
  - Name:            .data
    Type:            SHT_PROGBITS
    Flags:           [ SHF_ALLOC ]
    Address:         0x3000
    AddressAlign:    0x0000000000001000
    Content:         "3232"
  - Name: .baz
    Type: SHT_PROGBITS
    Size: 0x2000
ProgramHeaders:
  - Type: PT_LOAD
    Flags: [ PF_R, PF_W ]
    VAddr: 0x2000
    PAddr: 0x4000
    FileSize: 0x1800
    MemSize: 0x1800
    Sections:
      - Section: .data

GNU objcopy output:

000000 c3c3 c3c3 0000 0000 0000 0000 0000 0000
000010 0000 0000 0000 0000 0000 0000 0000 0000
*
003000 3232
003002

llvm-objcopy output:

000000 c3c3 c3c3 0000 0000 0000 0000 0000 0000
000010 0000 0000 0000 0000 0000 0000 0000 0000
*
004000 3232
004002

I also noticed several differences that may be unrelated to this issue when sections are not in segments. Speaking of which, did you mean for this to be the case for .text in this test?

MaskRay updated this revision to Diff 232953.Dec 9 2019, 3:44 PM
MaskRay edited the summary of this revision. (Show Details)

Fix formula: sh_offset - p_offset + p_paddr

See bfd/elf.c:1182 for a reference.

/* We used to use the same adjustment for SEC_LOAD

	   sections, but that doesn't work if the segment
	   is packed with code from multiple VMAs.
	   Instead we calculate the section LMA based on
	   the segment LMA.  It is assumed that the
	   segment will contain sections with contiguous
	   LMAs, even if the VMAs are not.  */

newsect->lma = (phdr->p_paddr + hdr->sh_offset - phdr->p_offset);

For sections not in a PT_LOAD, the BFD code uses a different formula.

		newsect->lma = (phdr->p_paddr
				+ hdr->sh_addr - phdr->p_vaddr);

We don't need care about that because -O binary does not dump non-SHF_ALLOC sections.

jhenderson added inline comments.Dec 10 2019, 1:39 AM
llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
25

This section isn't n a segment. Is that deliberate? If so, it probably needs a comment saying this.

34

Does this comment still make sense? The numbers don't line up with the p_vaddr below, and the comment along with it (Also sh_address -> sh_addr).

111

Same comment as above: is this section deliberately not in a segment?

Assuming it is, there may be scope for a test where both sections are in segments.

llvm/tools/llvm-objcopy/ELF/Object.cpp
2257

The minimum LMA of what? Please specify that in the comment.

MaskRay updated this revision to Diff 233137.Dec 10 2019, 10:13 AM
MaskRay marked 3 inline comments as done.

Fix tests.

In GNU objcopy, LMA = sh_offset-p_offset+p_paddr for a section contained in a PT_LOAD.

The rules that llvm-objcopy and GNU objcopy use to decide whether a section is contained in a PT_LOAD are different.
We use sh_offset for non-SHT_NOBITS sections, but GNU objcopy appears to use sh_addr (see readelf -l output)
So some properties (VAddr: 0x20000) can cause different behaviors, but we probably don't bother testing that because "VAddr != sh_addr of the first section" is unrealistic.

Changed the tests to have same behaviors with llvm-objcopy and GNU objcopy.

MaskRay updated this revision to Diff 233138.Dec 10 2019, 10:14 AM
MaskRay marked an inline comment as done.

Improve a comment.

MaskRay marked an inline comment as done.Dec 10 2019, 10:14 AM
MaskRay added inline comments.
llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
34

Changed to:

  1. The computed LMA is sh_offset-p_offset+p_paddr = 0x2000-0x2000+0x4000 = 0x4000.
Harbormaster completed remote builds in B42227: Diff 233138.
jhenderson accepted this revision.Dec 11 2019, 2:50 AM

LGTM, but it would probably be wise to get a second pair of eyes on this change.

This revision is now accepted and ready to land.Dec 11 2019, 2:50 AM

Is it ok for me to commit if there is still no other people interested in this patch for, say, 10 hours? 😂

I tested it on an internal objcopy -I ihex -O binary use case that motivated me to send the patch. With this patch, the section content layout is now the same.

Is it ok for me to commit if there is still no other people interested in this patch for, say, 10 hours? 😂

I tested it on an internal objcopy -I ihex -O binary use case that motivated me to send the patch. With this patch, the section content layout is now the same.

Let's give it until after the weekend. We've been stung on a few occasions by issues with binary output, so I want to give people ample opportunity to point out anything else we've missed!

Is it ok for me to commit if there is still no other people interested in this patch for, say, 10 hours? 😂

I tested it on an internal objcopy -I ihex -O binary use case that motivated me to send the patch. With this patch, the section content layout is now the same.

Let's give it until after the weekend. We've been stung on a few occasions by issues with binary output, so I want to give people ample opportunity to point out anything else we've missed!

+1 to just committing after the weekend. Since you mentiond ihex, adding Eugene who implemented the reading/writing so might have something to say about the patch, although it looks like this the ihex patches didn't touch binary output.

Do you have a reduced ihex test case to add too?

I'll watch out for other failures internally post-commit.

Is it ok for me to commit if there is still no other people interested in this patch for, say, 10 hours? 😂

I tested it on an internal objcopy -I ihex -O binary use case that motivated me to send the patch. With this patch, the section content layout is now the same.

Let's give it until after the weekend. We've been stung on a few occasions by issues with binary output, so I want to give people ample opportunity to point out anything else we've missed!

+1 to just committing after the weekend. Since you mentiond ihex, adding Eugene who implemented the reading/writing so might have something to say about the patch, although it looks like this the ihex patches didn't touch binary output.

Do you have a reduced ihex test case to add too?

I'll watch out for other failures internally post-commit.

ihex is not directly related the the -O binary issue. It just makes it easy to craft an ELF with (sec2.sh_addr-sec1.sh_addr != sec2.sh_offset-sec1.sh_offset), which may not easy with a linker.

Address can be specified

// ':' + Length + Address + Type + Checksum with empty data ':LLAAAATTCC'

OK, I'll commit after the weekend.

MaskRay updated this revision to Diff 233989.Dec 15 2019, 9:47 PM

Fix tests. Verified all 3 output files are identical to GNU objcopy's

This revision was automatically updated to reflect the committed changes.