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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:273-274
+    Size:         8
+DynamicSymbols: []
+Symbols: []
+ProgramHeaders:
----------------
Do we need these lines?


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


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1322
+
+  for (SectionBase &Sec : Obj.sections()) {
+    Segment *Seg = Sec.ParentSegment;
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1061
+// Returns true IFF a section is wholly inside the range of a segment. A section
+// can be included by multiple nested segments. If an empty section lies on the
+// boundary between two segments, it will be attributed to both segments.
----------------
by -> in


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1291
         Seg.addSection(&Sec);
-        if (!Sec.ParentSegment || Sec.ParentSegment->Offset > Seg.Offset)
+        if (!Sec.ParentSegment || Seg.Offset > Sec.ParentSegment->Offset)
           Sec.ParentSegment = &Seg;
----------------
This change will mean that sections are no longer assigned to the "most parental" segment. For example in the below example, the Section's parent will be the PT_TLS segment rather than the PT_LOAD segment.

```
 | Section |
 | PT_TLS  |
| PT_LOAD   |
```

I had a quick look at the code, and there are some cases where the `ParentSegment` is important: in the `IHexWriter::finalize` function, the `sectionPhysicalAddr` static function, and the `layoutSectionsForOnlyKeepDebug`, there are checks against PT_LOAD segment types. There may be other issues too that I haven't found.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74755