[PATCH] D42872: Fix handling of zero-size segments in llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

This is nearly there from my point of view. There are a few odd points in the inline comments which I'd like addressing before giving this the thumbs up, but they're only minor details. The solution is what I was imagining, and it's nice to see that everything just works (fingers-crossed!) with it.



================
Comment at: test/tools/llvm-objcopy/marker-segment.test:53
+    Flags: [ PF_R ]
+    Sections:
+      - Section: .xdata
----------------
This segment should probably have a non-zero VAddr.


================
Comment at: tools/llvm-objcopy/Object.cpp:459
+template <class ELFT>
+void ELFBuilder<ELFT>::setParentSegments(Segment &Child) {
+  for (auto &Parent : Obj.segments()) {
----------------
setParentSegments -> setParentSegment - a Segment can only have one Parent (it might of course have "grandparents" etc, but it's not directly relevant here).


================
Comment at: tools/llvm-objcopy/Object.cpp:503
+  auto &ElfHdr = Obj.ElfHdrSegment;
+  ElfHdr.Type = PT_PHDR;
+  ElfHdr.Flags = 0;
----------------
A quick comment explaining why we use PT_PHDR as the type would be wise.


================
Comment at: tools/llvm-objcopy/Object.cpp:516
+  PrHdr.Flags = 0;
+  PrHdr.OriginalOffset = PrHdr.Offset = PrHdr.VAddr = Ehdr.e_phoff;
+  PrHdr.PAddr = 0;
----------------
We should add comments for why we chose VAddr 0/e_phoff in these two locations.


================
Comment at: tools/llvm-objcopy/Object.cpp:519
+  PrHdr.FileSize = PrHdr.MemSize = Ehdr.e_phentsize * Ehdr.e_phnum;
+  PrHdr.Align = 0;
+  PrHdr.Index = Index++;
----------------
This should have Align = sizeof(Elf_Addr), as the spec requires all ELF fields to be "naturally" aligned (i.e. size 8 fields on an 8-byte boundary, 4 on a 4-byte etc). By setting Align to the size of an address, this will happen automatically.


================
Comment at: tools/llvm-objcopy/Object.cpp:945-948
   // The size of ELF + program headers will not change so it is ok to assume
   // that the first offset of the first segment is a good place to start
   // outputting sections. This covers both the standard case and the PT_PHDR
   // case.
----------------
This comment needs updating/removing.


================
Comment at: tools/llvm-objcopy/Object.h:563
 
+  Segment ElfHdrSegment;
+  Segment ProgramHdrSegment;
----------------
We should have a comment here explaining why we have these two special segments.


Repository:
  rL LLVM

https://reviews.llvm.org/D42872