llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets for SHT_NOBITS sections.

[pcc]

Summary

Without this, sectionWithinSegment() will return the wrong answer for bss
sections. This doesn't seem to matter now (for non-broken ELF files), but
it will matter with a change that I'm working on.

[rupprecht]

This is probably fine, but what's the full context for this change? I'm not able to tell from the unit test -- it doesn't look like it's testing .bss. Is it possible to add a test for that?

[pcc]

The change to segment-test-remove-section.test merely changes it from a broken ELF file (where the segment covers vaddrs 0x0000-0x3000 but the sections cover vaddrs 0x1000-0x4000) to a non-broken one (where both cover the same vaddrs). This causes sectionWithinSegment to return the right answer for the sections in the segment after my code change; without the test change, llvm-objcopy will move .text3 outside of the segment (since, from the vaddr point of view, it *is* outside of the segment).

I couldn't figure out a good way to test this (other than "we handle broken ELF files slightly differently", which doesn't seem very useful to test for). I tried adding a bss section to the test but I couldn't see a change in behaviour (the behaviour appears to be correct either way).

The change that I'm working on will add a feature that, among other things, strips all SHF_ALLOC sections that are not within a segment. Of course, this needs to classify bss sections correctly, otherwise we will end up stripping them as well.

I could take another look to see if there is a useful change in behaviour for non-broken ELF files. Otherwise this can wait until my other change is ready, I suppose.

[jakehehrlich]

If .bss has the desired offset this will work. Also .tbss, .tdata, PT_TLS, and PT_LOAD interact in very strange ways that we need to be careful about. In particular the TLS image will exist in a PT_LOAD read-only segment but the PT_TLS segment will have a bogus vaddr and the .tbss will extend into the part of the PT_LOAD that covers .rodata, .dynstr, etc...

I'll come back to thinking about this. Also please add me as a reviewer to changes like this (ELF llvm-objcopy/strip changes that change subtle behavior).

[jakehehrlich]

For your use case you can simply traverse the program headers for each to see if a section is indeed loadable. e.g. you want to strip all non-loadable allocated sections. That seems like a good thing but this probably isn't the right way to handle it IMO.

[pcc]

In D58426#1433713, @jakehehrlich wrote:

For your use case you can simply traverse the program headers for each to see if a section is indeed loadable. e.g. you want to strip all non-loadable allocated sections. That seems like a good thing but this probably isn't the right way to handle it IMO.

That's basically what the code ends up doing (via the call to sectionInSegment here: http://llvm-cs.pcc.me.uk/tools/llvm-objcopy/ELF/Object.cpp#920 ; the ParentSegment field is used here: https://github.com/pcc/llvm-project/blob/458fc008993a572c290f2742f349c1ba7c232b2f/llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L442 ). I'm not sure what other way you had in mind.

[jakehehrlich]

I mean in the handler for the option you can re-traverse the program headers for each section and handle your check.

[rupprecht]

(adding a few subscribers from D59351)

In D58426#1433507, @jakehehrlich wrote:

Also please add me as a reviewer to changes like this (ELF llvm-objcopy/strip changes that change subtle behavior).

I recommend setting up a herald rule if you want to be sure to be a reviewer, e.g. https://reviews.llvm.org/H418

[pcc]

In D58426#1435243, @jakehehrlich wrote:

I mean in the handler for the option you can re-traverse the program headers for each section and handle your check.

I don't understand why you think it would be better to have two code paths, one that allocates sections to segments incorrectly and another that allocates them correctly, rather than just having one code path that allocates them correctly.

[jakehehrlich]

I contest that this change is correct. This predicate is specifically designed to capture which bytes overlap in the file, not the run time semantic goal you're looking for. I'm also concerned that it will interact badly with PT_TLS, . It was for this exact reason (handling of PT_TLS) that we chose to use Offset and Filesize *instead* of VAddr and MemSize as was my first instinct.

Consider the following not-quote-counter example that I pulled from the first binary I found in fuchsia that uses TLS. You can make it into a counter example by simply adding a giant symbol to .tbss

[19] .tdata            PROGBITS         00000000001ae000  001ae000
     0000000000000148  0000000000000000 WAT       0     0     8
[20] .tbss             NOBITS           00000000001ae148  001ae148
     00000000000008f8  0000000000000000 WAT       0     0     8
[21] .data.rel.ro      PROGBITS         00000000001ae148  001ae148
     0000000000006ff0  0000000000000000  WA       0     0     8
[22] .got              PROGBITS         00000000001b5138  001b5138
     0000000000000258  0000000000000000  WA       0     0     8
[23] .bss              NOBITS           00000000001b6000  001b5390
     0000000000000210  0000000000000000  WA       0     0     8

LOAD           0x00000000001ad000 0x00000000001ad000 0x00000000001ad000
               0x0000000000008390 0x0000000000009210  RW     0x1000
TLS            0x00000000001ae000 0x00000000001ae000 0x00000000001ae000
               0x0000000000000148 0x0000000000000a40  R      0x8

By your predicate .data.rel.ro is *not* in the PT_TLS segment but it if .tbss had been larger your predicate *would* say that .data.rel.ro is in PT_TLS. That is incorrect behavior and this is the reason that we very deliberately used Offset and FileSize and not VAddr and MemSize.

Bugs of this form are mitigated by trying to find the "most parental" segment but the heuristic used there is to favor the segment with the smallest offset. If a PT_TLS segment was in the program header table first, had the same offset as the encompassing PT_LOAD (e.g. .data and .got.plt was empty), and had there was a large symbol defined in .tbss you're predicate would cause layout to fail.

The offset of NOBITS sections shouldn't matter but we ofcourse do (via ParentSegment) use this information in stripping now. This accounts for cases where we want to avoid stripping a section that isn't allocated but is in a segment which can for instance happen with some PT_NOTE segments in some questionable cases where the PT_NOTE segment is added after static linking occurs. There might be a bug lurking in situations where NOBITS have unconventional (but still correct) offsets and we're now not stripping them correctly but that's a recent occurrence and a different bug.

[pcc]

Handle TLS

[pcc]

It seems quite easy to handle TLS. See the change that I just uploaded. Similar code would be necessary to correctly extract partitions if for example the first partition had a .tbss whose address space extended into the second partition.

It seems that a number of tests that were added in D59293 now fail with this change. I haven't formed an opinion yet on that change, although I suppose that if we wanted to preserve the behaviour introduced in D59293 we might consider allocating non-NOBITS sections to segments on the basis of offset/filesz, and NOBITS sections on the basis of vaddr/memsz.

[jakehehrlich]

If you think about TLS as being in a sort of different address space (which was not a way I had thought about this before) then this is the function that determines if a section is in the same address area as a segment so its a semantically consistent idea I think. Layout should also always succeed because the PT_TLS will be given the respective PT_LOAD as a parent which will fix its offset correctly and then any sections assigned only to PT_TLS and not a PT_LOAD will still be laid out correctly (which should always be the case for any overlapping segment, we should have tests for this already). We are however changing what this function means even if in practice it's true/false in almost all of the same cases. For non-NOBITS Segment.VAddr - Section.Addr == Segment.Offset - Section.OriginalOffset and those sections will never fall in the range past Segment.FileSize so the predicate should return true in all the same cases for non-NOBITS. It would also now assign sections to the "expected" segment even if today that has no affect one way or the other.

Things get more tricky if you change segmentOverlapsSegment to behave that way. In that case its critical that PT_TLS have a PT_LOAD as its parent. The subtle reason behind why we use VAddr in one case and Offset in another is going to be confusing to explain to people (as if the reason we used Offset and not VAddr wasn't already complicated enough)

I was going to point the non-allocated section issue out as well but it seems you've already seen that. Since non-allocate sections don't have meaningful vaddrs this is tricky. Independent of this change as it exists right now I am 100% ok with using Offset/FileSize for most sections and using VAddr/MemSize for NOBITS (since in principal NOBITS should be allowed to have any offset). I think that would be a better heuristic for assigning NOBITS sections to segments. It should be the case that weather or not a NOBITS section is assigned to a segment, layout should not be affected anyhow. This seems like a less semantically consistent operation however. As the code exists today it assigns a size of 1 to NOBITS so they always wind up assigned to the wrong PT_LOAD if they wind up assigned at all. This only matters in so far as the offset is a bit wonky but this causes no general problem since the offset of a NOBITS section is meaningless (though an expected answer exists and we should match it).

An alternative is to do this change on allocated sections (e.g. vaddr/memsize and account for TLS), and then to use offset/filesize on non-allocated sections. I think this would solve the issues being seen in D59293 and would always use the meaningful offset unless a non-allocated NOBITS section existed...but such a thing should just be assumed to never exist as part of a segment I think.

I still however maintain that if you're relaying on what segment a NOBITS section falls into using ParentSegment then you're doing something wrong. That said there might be an offset bug we should fix for expectation's sake. If we're only trying to fix the offset bug for expectations sake then I support using VAddr/MemSize for NOBITS only as the correct solution. Could you describe your use case a bit more perhaps?

[jhenderson]

My thought on this is that file offsets are generally better, but as pointed out by @bd1976llvm pointed out on the mailing list, a recent discussion on the gABI mailing list shows that SHT_NOBITS section offsets should be considered meaningless and ignored. That means that we can only use addresses for determining NOBITS section parentalness.

It is perfectly valid to put non-alloc sections in segments, (and indeed, I have made changes to that effect recently), just they don't make much sense in PT_LOAD segments. A loader or binary utility can read the program headers to find information in the ELF without having to rely on section headers, which might have been stripped (see the purpose of PT_NOTE for example). Such sections/segments will have no address, but clearly a section can be determined to be in the segment via its file offset.

Conclusion:
I think we should continue to rely on offsets for all sections EXCEPT SHT_NOBITS sections, which instead use the address field. We might still need some special-case logic for PT_TLS/.tbss, and possibly for zero-file-sized segments but this will otherwise work, I think.

[jakehehrlich]

I can think of two ways

James Way (which works in all cases I'm aware of without faulty assumptions and should produce the correct result)
0) Return false if a non-allocated NOBITS section is used here

1. If the section is NOBITS use address and check that SectionIsTLS == SegmentIsTLS
2. If the section is not NOBITS disregard SectionIsTLS == SegmentIsTLS and use offset

Another Way:
0) Return false if a non-allocated NOBITS section is used here

1. If allocated use address and check that TLS flags match. We assume that we're in a non ET_REL binary since there's a program header.
2. If non-allocated use offset to account for the case where a segment still covers the section.

Both of those should work. I think I prefer James's way.

[pcc]

Okay, finally getting back to this.

I still however maintain that if you're relaying on what segment a NOBITS section falls into using ParentSegment then you're doing something wrong. That said there might be an offset bug we should fix for expectation's sake. If we're only trying to fix the offset bug for expectations sake then I support using VAddr/MemSize for NOBITS only as the correct solution. Could you describe your use case a bit more perhaps?

My use case is producing a correct section table when extracting a partition from a combined output file (see Partitions.rst in D60242). In a combined output file there is a main partition whose ELF headers are at file offset 0, as well as one or more loadable partitions whose ELF headers are stored in a section of a specific type. A tool such as llvm-objcopy can extract a partition by:

• reading the section headers using the ELF header at file offset 0;
• if extracting a loadable partition:
  • finding the section containing the required partition ELF header by looking it up in the section table;
  • reading the program headers using the ELF header found in the section;
• if extracting the main partition:
  • reading the program headers using the ELF header at file offset 0;
• filtering the section table according to which sections are in the program headers that it read: if ParentSegment != nullptr or section is not SHF_ALLOC, then it goes in.

That is why ParentSegment needs to be accurate. It doesn't matter which segment it points to, but it needs to point to one of them.

Return false if a non-allocated NOBITS section is used here
If the section is NOBITS use address and check that SectionIsTLS == SegmentIsTLS
If the section is not NOBITS disregard SectionIsTLS == SegmentIsTLS and use offset

I think this will work for me. I will implement it.

[pcc]

Address review comments

[jakehehrlich]

I think this is good. The current behavior treats sectionWithinSegment as not mattering in this case because this predicate is used for getting file offsets right and ignores the SHT_NOBITS case. This is a strict improvement that just sets the predicate in such a way that llvm-objcopy will output the conventional offset now.

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

 // Returns true IFF a section is wholly inside the range of a segment
 static bool sectionWithinSegment(const SectionBase &Section,
                                  const Segment &Segment) {
   // 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 = Section.Size ? Section.Size : 1;
+
+  if (Section.Type == SHT_NOBITS) {
+    if (!(Section.Flags & SHF_ALLOC))
+      return false;
+
+    bool SectionIsTLS = Section.Flags & SHF_TLS;
+    bool SegmentIsTLS = Segment.Type == PT_TLS;
+    if (SectionIsTLS != SegmentIsTLS)
+      return false;
+
+    return Segment.VAddr <= Section.Addr &&
+           Segment.VAddr + Segment.MemSize >= Section.Addr + SecSize;
+  }
+
   return Segment.Offset <= Section.OriginalOffset &&
          Segment.Offset + Segment.FileSize >= Section.OriginalOffset + SecSize;
 }
 
 // Returns true IFF a segment's original offset is inside of another segment's
 // range.
 static bool segmentOverlapsSegment(const Segment &Child,
                                    const Segment &Parent) {