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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

To clarify what I understood James to be proposing there should be two additional segments and they should not be added to Segments but instead OrderedSegments.



================
Comment at: tools/llvm-objcopy/Object.cpp:461-462
   uint32_t Index = 0;
+  bool IsHeaderInMemory = false;
+  uint64_t PageAlign = 0;
   for (const auto &Phdr : unwrapOrError(ElfFile.program_headers())) {
----------------
This is not needed. PageAlign is needed at all. IsHeaderInMemory shouldn't actully be computed, we should just add special segments to OrderedSegments that cover the headers and then if there already happened to be segments covering the program headers then so be it and if not that's fine also.


================
Comment at: tools/llvm-objcopy/Object.cpp:477-490
+
+    // ELF header may be mentioned in program headers by a dedicated segment
+    // (PT_PHDR), or by a loadable segment with a zero file offset.
+    if (Seg.Type == PT_PHDR ||
+      (Seg.Type == PT_LOAD && Seg.Offset == 0 && Seg.FileSize > 0))
+      IsHeaderInMemory = true;
+
----------------
None of this is needed.


================
Comment at: tools/llvm-objcopy/Object.cpp:505
+    // to protect ourselves from writing to this area.
+    Segment &ElfHeader = Obj.addDummySegment();
+    ElfHeader.Type = PT_LOAD;
----------------
Don't add a header to segments here. Instead you should have two Segments as new members of Object, one for the ELF header and one for the program header table.  The ELF header one should have pretty much all fields zero but importantly it *must* have the Offset as zero, the OriginalOffset as zero, the VAddr  as zero, and the FileSize must be Elf_Ehdr. MemSize should be equal to FileSize for consistency. The index doesn't really matter here but I think doing what you're doing now (namely, Index = Index++) is best because it's sort of like we just happened to add two new members to the program header table.


================
Comment at: tools/llvm-objcopy/Object.cpp:515
+    else
+      ElfHeader.FileSize = sizeof(Elf_Ehdr);
+    ElfHeader.MemSize = 0;
----------------
The FileSize should always be sizeof(Elf_Ehdr). It should never be PageAlign. I think you may be misunderstanding the spec. Here's the spec: http://www.sco.com/developers/gabi/2012-12-31/contents.html


================
Comment at: tools/llvm-objcopy/Object.cpp:520
+  }
+
   // Now we do an O(n^2) loop through the segments in order to match up
----------------
You'll also need to construct a program header table. Setting this up correctly is much trickier than the ELF header. You'll want to use ElfFile.getHeader() here. The Offset and OriginalOffset should be Ehdr->e_phoff, the FileSize and MemSize should be Ehdr->e_phentsize * Ehdr->e_phnum. The Type, for giggles, should be PT_PHDR. The VAddr actually matters here because it has to meet the important p_offset % p_align = p_vaddr % p_align.  We also don't know what alignment it should meet this condition for. Luckily if we set p_vaddr to be the same as p_offset it will always hold *and* it won't effect layout if we do that either. I'd also like the index of this to be unique the way you did it for ElfHeader.


================
Comment at: tools/llvm-objcopy/Object.cpp:524
   for (auto &Child : Obj.segments()) {
     for (auto &Parent : Obj.segments()) {
       // Every segment will overlap with itself but we don't want a segment to
----------------
Any artificially constructed segments (of which there will be two) will need to have their parent segments set. I think you should factor this inner loop out into a function so that you can later call it for the two constructed segments.


================
Comment at: tools/llvm-objcopy/Object.cpp:782
   Ehdr.e_phentsize = sizeof(Elf_Phdr);
-  Ehdr.e_phnum = size(Obj.segments());
+  Ehdr.e_phnum = Obj.countFileSegments();
   Ehdr.e_shentsize = sizeof(Elf_Shdr);
----------------
This should go back to the old way after you get rid of the dummy segments stuff.


================
Comment at: tools/llvm-objcopy/Object.cpp:796-800
+  for (auto &Seg : Obj.segments()) {
+    if (!Seg.DummySegment)
+      writePhdr(Seg);
+  }
 }
----------------
This should go back to the old way after you get rid of the dummy segments stuff.


================
Comment at: tools/llvm-objcopy/Object.cpp:951-954
   std::vector<Segment *> OrderedSegments;
   for (auto &Segment : Obj.segments())
     OrderedSegments.push_back(&Segment);
   OrderSegments(OrderedSegments);
----------------
You're going to need to modify this by adding two additional segments. If these two additional segments are members of Object you can just add their addresses at the end of the loop (which will also keep their Index order consistent).


================
Comment at: tools/llvm-objcopy/Object.h:242
+  // but are not present in the resulting file.
+  bool DummySegment = false;
+
----------------
We shouldn't need DummySegments as a new type of segment.


================
Comment at: tools/llvm-objcopy/Object.h:245
   Segment(ArrayRef<uint8_t> Data) : Contents(Data) {}
+  Segment() : DummySegment(true) {}
 
----------------
We don't need this.


================
Comment at: tools/llvm-objcopy/Object.h:588
 
+  size_t countFileSegments() {
+    return std::count_if(Segments.begin(), Segments.end(),
----------------
We don't need or want this.


Repository:
  rL LLVM

https://reviews.llvm.org/D42872