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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 14 01:59:44 PST 2018

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.



More information about the llvm-commits mailing list