[objcopy][ELF] Handle sections not contained in PT_LOAD segments

[LemonBoy]

Summary

In D71035 the LMA calculation was changed to more closely match the behaviour of GNU
tools. The code assumed that every parent section would be a PT_LOAD one, but it's possible
to create legal ELF binaries where this rule doesn't hold anymore.

Handle this case by using the same equation used by GNU objdump, the fix allows objdump
to create correctly-sized raw binaries now.

[jakehehrlich]

I don't understand the issue here personally. Can you elaborate? The formula also doesn't make sense to me since Sec.Addr isn't really well defined at that point in the code. e.g. this *is* the layout algorithm so you shouldn't be using Sec.Addr here IMO

[MaskRay]

The code assumed that every parent section would be a PT_LOAD one, but it's possible to create legal ELF binaries where this rule doesn't hold anymore.

It is possible, but I hope we can find legitimate and reasonable use cases. p_memsz(PT_GNU_RELRO) makes sense (-z stack-size=), so this seems to suggest that we should patch template <class ELFT> void ELFBuilder<ELFT>::setParentSegment(Segment &Child) to take into account this case (don't set ParentSegment(PT_LOAD) to PT_GNU_STACK)

[LemonBoy]

I don't understand the issue here personally

As explained by @MaskRay you can create a segment with zero starting address and arbitrarily large memsize by using the -z stack-size= switch, the libc interprets that value as the minimum size for new threads. The segment acts as a catch-all parent segment for many sections, such as the .bss in the test case, and causes problems in the LMA calculation code.

Sec.Addr isn't really well defined at that point in the code

There's an unfortunate typo in the patch, the equation is meant to simply rebase the sh_addr by subtracting the phdr vaddr and adding its paddr.

this seems to suggest that we should patch template <class ELFT> void ELFBuilder<ELFT>::setParentSegment(Segment &Child) to take into account this case

I decided against that because I couldn't find any definition of what makes a segment eligible to be a parent. If we use the same logic that readelf uses then the PT_STACK_SIZE is indeed a parent for .bss

Section to Segment mapping:
 Segment Sections...
  00     .text 
  01     .bss

[LemonBoy]

Fix a typo in the address calculation formula and fix the hexdump in the test case.

[jhenderson]

Please fix the review title and description to say "llvm-objcopy" not "llvm-objdump".

It seems confusing that there should be different logic between sections within PT_LOAD and those within other segment types. What is actually the difference between them here?

I decided against that because I couldn't find any definition of what makes a segment eligible to be a parent.

The definition is internal to llvm-objcopy, and isn't really well written down in comments anywhere, IIRC, just with the code. There are no rules on "eligibility" for a segment to be a parent, i.e. it doesn't have to be a specific type. Simply all that happens is that if a segment at least partially overlaps another segment, the "left-most" one is picked. If one starts at the same offset, the larger is considered the parent, with the first in the program header the final tie-breaker. Finally, there is a "most parental" segment, i.e. there are no "grandparents" in the code - if segment A has a parent, segment B, and would be the parent of segment C, segment C's parent will be segment B instead.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test

133–134

Could you outline in a comment what the "before" behaviour of this test case was. It will help with reviewing.

138

Why --ignore-case?

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

2267

Sec.Addr here is just the initial address contained in the section header, right? That deserves a comment at least, because it is definitely not obvious that that is the case in this context, where we appear to be setting the value.

[jhenderson]

By the way, I don't have any particular insight into how the behaviour should work here, as I don't use binary output myself.

[LemonBoy]

What is actually the difference between them here?

The difference doesn't really matter here as the segment PAddr and VAddr are zero, the test case passes by simply avoiding the Sec.Addr assigment for sections with non-PT_LOAD segments, that's why it's been suggested to patch setParentSegment and keep the old logic.
The added logic takes into account one more edge case, as explained by this comment in BFD:

/* 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.  */

If you prefer a less-invasive patch I can simply go for the setParentSegment route.

[MaskRay]

In D79038#2013052, @LemonBoy wrote:

What is actually the difference between them here?

The difference doesn't really matter here as the segment PAddr and VAddr are zero, the test case passes by simply avoiding the Sec.Addr assigment for sections with non-PT_LOAD segments, that's why it's been suggested to patch setParentSegment and keep the old logic.
The added logic takes into account one more edge case, as explained by this comment in BFD:

/* 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.  */

If you prefer a less-invasive patch I can simply go for the setParentSegment route.

I did some shallow analysis of bfd/elf.c's rules related to bfd_section::lma/file offset/etc. They are overly complex and I am pretty sure we can do better than them. Their distinction of PT_LOAD ver non-PT_LOAD is artificial and can really be simplified if they don't do that. I second to James that we should not make PT_LOAD different.

Unfortunately, the PT_GNU_RELRO use case (p_vaddr=p_paddr=0, large p_memsz) means we still have to specialize something. It seems that we can simply treat PT_GNU_RELRO as an (ugly) exception.

[LemonBoy]

Simplify the patch by making sure PT_GNU_STACK segments cannot be considered parents of any section.

[MaskRay]

New code looks good to me.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test

138

https://reviews.llvm.org/D68693#2013750

149

Reduce indentation so that there is exactly one space after the longest key (Machine:)

174

The comment is stale.

[LemonBoy]

Minor touchups for the test case.

[jhenderson]

Latest version LGTM, with one comment nit.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test

138

Thanks. Evidently I forgot that.

Bizarre that this tool somehow is so inconsistent in behaviour compared to other simialr tools (see also problems I've had before with argument ordering).

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

1068

"segments, they" -> either "segments. They" or "segments: they"

[LemonBoy]

with one comment nit

It's probably better if you (or somebody else) with commit access amends the comment just before committing so that the PR doesn't have to be approved again.

[MaskRay]

I will address the comments and push.

[MaskRay]

The test does not make sense: .bss is not covered by a PT_LOAD.

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        0000000000008000 0000b0 000008 00  AX  0   0  8
  [ 2] .bss              NOBITS          0000000000008008 0000b8 000004 00  WA  0   0  1

Program Headers:
  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
  LOAD           0x0000b0 0x0000000000008000 0x0000000000008000 0x000008 0x000008 R E 0x8
  GNU_STACK      0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x1000000 RW  0

If we extend the existing PT_LOAD to cover .bss, then we can apply D74755 instead. We don't need the special case for PT_GNU_STACK.

The description is also wrong now. I may say something like:

A PT_GNU_STACK segment can have very large p_memsz (specified by ld -z stack-size=)
 and zero p_vaddr/p_paddr.
 
 `sectionWithinSegment` prefers assigning the parent segment of a section
 to the PT_GNU_STACK over a PT_LOAD. In the test, this causes .bss to have a large LMA and make the output much larger.

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test

138

I would use only 0-9 and omit --ignore-case

# RUN: yaml2obj --docnum=6 %s -o %t6
# RUN: llvm-objcopy -O binary %t6 %t6.out
# RUN: od -A x -t x1 %t6.out | FileCheck %s --check-prefix=GNUSTACK

# GNUSTACK:      000000 90 90
# GNUSTACK-NEXT: 000002

[LemonBoy]

test does not make sense: .bss is not covered by a PT_LOAD.

That's the whole point of this patch, the binary that ptomptrd me to investigate why the binary image was so big has a single PT_LOAD segment covering only .text.

The description is also wrong now. I may say something like:

Sure thing, I actually forgot to update it.

[LemonBoy]

Ping?

[MaskRay]

In D79038#2016086, @LemonBoy wrote:

test does not make sense: .bss is not covered by a PT_LOAD.

That's the whole point of this patch, the binary that ptomptrd me to investigate why the binary image was so big has a single PT_LOAD segment covering only .text.

The description is also wrong now. I may say something like:

Sure thing, I actually forgot to update it.

Why does your executable have a .bss not covered by a PT_LOAD? I think it is invalid but I am open to justifications.

An alternative is to apply D74755. Last time I checked D74755 would fix your problem as well.

[LemonBoy]

In D79038#2032024, @MaskRay wrote:

In D79038#2016086, @LemonBoy wrote:

test does not make sense: .bss is not covered by a PT_LOAD.

That's the whole point of this patch, the binary that ptomptrd me to investigate why the binary image was so big has a single PT_LOAD segment covering only .text.

The description is also wrong now. I may say something like:

Sure thing, I actually forgot to update it.

Why does your executable have a .bss not covered by a PT_LOAD? I think it is invalid but I am open to justifications.

An alternative is to apply D74755. Last time I checked D74755 would fix your problem as well.

There's a bit more context here, the ELF is indeed wrong but GNU objcopy manages to extract the correct raw binary. Feel free to apply whatever patch fixes the problem and/or mark this use case as invalid, I don't have much time for this stuff right now.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test

- Name:            .data
     AddressAlign:    0x1000
     Content:         "3232"
 ProgramHeaders:
   - Type: PT_LOAD
     Flags: [ PF_R, PF_W ]
     VAddr: 0x3000
     Sections:
       - Section: .data
+
+## Check that the LMA is correctly calculated for sections not contained in a
+## PT_LOAD segment.
+
+# RUN: yaml2obj --docnum=4 %s -o %t4
+# RUN: llvm-objcopy -O binary %t4 %t4.out
+# RUN: od -A x -t x1 %t4.out | FileCheck %s --check-prefix=CHECK4 --ignore-case
+# RUN: wc -c %t4.out | FileCheck %s --check-prefix=SIZE4
+
+# CHECK4:      90 90 90 90 90 90 90 c3
+# SIZE4:       8
+
+--- !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:         0x8000
+    AddressAlign:    0x8
+    Content:         90909090909090C3
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_ALLOC, SHF_WRITE ]
+    Address:         0x8008
+    AddressAlign:    0x1
+    Size:            0x4
+ProgramHeaders:
+  - Type: PT_LOAD
+    Flags: [ PF_R, PF_X ]
+    VAddr: 0x8000
+    Sections:
+      - Section: .text
+  - Type: PT_GNU_STACK
+    Flags: [ PF_R, PF_W ]
+    VAddr: 0x0000
+    FileSize: 0x0000
+    Align: 0x0000
+    # The huge size makes this phdr a parent of .bss
+    MemSize: 0x1000000

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

 // Returns true IFF a section is wholly inside the range of a segment
 static bool sectionWithinSegment(const SectionBase &Sec, const Segment &Seg) {
   // If a section is empty it should be treated like it has a size of 1. This is
   // to clarify the case when an empty section lies on a boundary between two
   // segments and ensures that the section "belongs" to the second segment and
   // not the first.
   uint64_t SecSize = Sec.Size ? Sec.Size : 1;
 
   if (Sec.Type == SHT_NOBITS) {
     if (!(Sec.Flags & SHF_ALLOC))
       return false;
 
     bool SectionIsTLS = Sec.Flags & SHF_TLS;
     bool SegmentIsTLS = Seg.Type == PT_TLS;
     if (SectionIsTLS != SegmentIsTLS)
       return false;
 
Error BinaryWriter::finalize() {
   OrderedSegments.erase(End, std::end(OrderedSegments));
 
   // Compute the section LMA based on its sh_offset and the containing segment's
   // p_offset and p_paddr. Also compute the minimum LMA of all sections as
   // MinAddr. In the output, the contents between address 0 and MinAddr will be
   // skipped.
   uint64_t MinAddr = UINT64_MAX;
   for (SectionBase &Sec : Obj.allocSections()) {
-    if (Sec.ParentSegment != nullptr)
+    if (Sec.ParentSegment != nullptr) {
+      if (Sec.ParentSegment->Type == PT_LOAD)
         Sec.Addr =
             Sec.Offset - Sec.ParentSegment->Offset + Sec.ParentSegment->PAddr;
+      else
+        Sec.Addr =
+            Sec.Addr - Sec.ParentSegment->VAddr + Sec.ParentSegment->PAddr;
+    }
     MinAddr = std::min(MinAddr, Sec.Addr);
   }
 
   // Now that every section has been laid out we just need to compute the total
   // file size. This might not be the same as the offset returned by
   // layoutSections, because we want to truncate the last segment to the end of
   // its last section, to match GNU objcopy's behaviour.
   TotalSize = 0;