[PATCH] D79038: [objdump][ELF] Handle sections not contained in PT_LOAD segments

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test:133-134
+
+## Check that the LMA is correctly calculated for sections not contained in a
+## PT_LOAD segment.
+
----------------
Could you outline in a comment what the "before" behaviour of this test case was. It will help with reviewing.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test:138
+# 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
----------------
Why --ignore-case?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2267
+        Sec.Addr =
+            Sec.Addr - Sec.ParentSegment->VAddr + Sec.ParentSegment->PAddr;
+    }
----------------
`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.


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

https://reviews.llvm.org/D79038