[PATCH] D74755: [llvm-objcopy] Attribute an empty section to a segment ending at its address

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 5 inline comments as done.
MaskRay added a comment.

> I haven't tried to create test cases that involve any of these, but it seems like it would be possible.

It is possible but it does not matter.

For the PT_LOAD which include the PT_TLS, note that `p_offset(PT_LOAD) = p_offset(PT_TLS)` on all versions of GNU ld and gold, and lld>=9.0 (precisely, after D56828 <https://reviews.llvm.org/D56828>).

We will set `ParentSegment` to the PT_TLS if `p_offset(PT_LOAD) < p_offset(PT_TLS)`. GNU strip/objcopy<2.31 does not handle such PT_TLS (there are various other toolchain validation issues) => this makes such a layout quite problematic => it is not worth working around the layout in llvm-objcopy. (Alan Modra kindly worked around lld's old layout in binutils 2.31 but I hope they can drop that hack (https://sourceware.org/bugzilla/show_bug.cgi?id=22829))

--------

It is also possible to place PT_TLS before PT_LOAD to set `ParentSegment` to the PT_TLS, but it is also an impractical scenario. The solution I pick here is consistent with:

  static bool compareSegmentsByOffset(const Segment *A, const Segment *B) {
    // Any segment without a parent segment should come before a segment
    // that has a parent segment.
    if (A->OriginalOffset < B->OriginalOffset)
      return true;
    if (A->OriginalOffset > B->OriginalOffset)
      return false;
    return A->Index < B->Index;
  }

Linkers don't place a PT_TLS before a PT_LOAD. One can use PHDRS command to place a PT_TLS before a PT_LOAD, but we can say that is unsupported by llvm-objcopy.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/strip-all.test:82
 # CHECK: Name: .text
+# CHECK: Name: .blarg
 # CHECK: Name: .gnu.warning.foo
----------------
jhenderson wrote:
> I think this behaviour change shows the test needs changing, as it no longer tests the case "non-alloc not in segment is removed".
Added `Size: 1` to `.blarg` instead.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1322
+
+  for (SectionBase &Sec : Obj.sections()) {
+    Segment *Seg = Sec.ParentSegment;
----------------
jhenderson wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > I believe this code captures the intention of the original code. Though, no tests break with this change.
> > > 
> > > I find it difficult to construct a test case that will break by deleting this piece of code.
> > > 
> > > @jhenderson It seems you may have such tests?
> > Deleted. I think this is not useful.
> FWIW, I didn't have any specific test that broke with this. I only had the example that I posted that I came up with after considering the code change.
I think there are only artificial impractical tests that can break. See my comment above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74755/new/

https://reviews.llvm.org/D74755