[PATCH] D33964: [LLVM][llvm-objcopy] Added basic plumbing to get things started

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:95-101
+      if (Seg.Offset <= Section->OriginalOffset &&
+          Seg.Offset + Seg.FileSize >=
+              Section->OriginalOffset + Section->Size) {
+        Seg.addSection(&*Section);
+        if (!Section->ParentSegment ||
+            Section->ParentSegment->Offset > Seg.Offset) {
+          Section->ParentSegment = &Seg;
----------------
>From a code readability point, it might be worth either breaking this into a small helper function, or to explain the algorithm in comments somewhere.

I'm not asking you to add them at this point, but this is the sort of thing that would be great to have unit-tests around!


================
Comment at: tools/llvm-objcopy/Object.cpp:193
+      // image is a copy of the file image, we preserve the file image as well.
+      Offset = FirstInSeg->Offset + Section->OriginalOffset -
+               FirstInSeg->OriginalOffset;
----------------
I've just thought about yet another problem with this:

In some cases, the ELF header and/or Program Header table can appear in a segment before the first section. Consequently, we need to preserve the relative offset to the start of the section, rather than the first segment. At the moment, any space before the first section within the segment is ignored and lost.

This applies to Segment::finalize() too.


================
Comment at: tools/llvm-objcopy/Object.cpp:219
+  // Finally now that all offsets and indexes have been set we can finalize any
+  // reamining issues.
+  uint64_t Offset = SHOffset + sizeof(Elf_Shdr);
----------------
typo: remaining


================
Comment at: tools/llvm-objcopy/Object.cpp:261-271
+  Elf_Shdr &Shdr = *reinterpret_cast<Elf_Shdr *>(Buf);
+  Shdr.sh_name = 0;
+  Shdr.sh_type = SHT_NULL;
+  Shdr.sh_flags = 0;
+  Shdr.sh_addr = 0;
+  Shdr.sh_offset = 0;
+  Shdr.sh_size = 0;
----------------
Nothing is using Shdr here. Is this missing a write, or can it be removed?


================
Comment at: tools/llvm-objcopy/Object.h:57
+    bool operator()(const SectionBase *Lhs, const SectionBase *Rhs) const {
+      return Lhs->Addr < Rhs->Addr;
+    }
----------------
Two sections with identical addresses will be considered identical by the set, which means that only one will be present. I'm pretty sure overlapping sections are bad, but an empty section could easily share the same start address without any worry. I think you need to compare the pointers themselves to prove they are not identical, i.e. the check becomes:

```if (Lhs->Addr == Rhs->Addr)
  return Lhs < Rhs;
return Lhs->Addr < Rhs->Addr;```

Optionally, you could test other members as well, e.g. Offset or Size, but ultimately, you're going to need to test that they are not the same section somewhere.

Please also add a test case for this (e.g. two identical empty sections at the same address and offset, or one empty and one non-empty section at the same address and offset).


================
Comment at: tools/llvm-objcopy/Object.h:76
+  const SectionBase *firstSection() const {
+    if (Sections.size())
+      return *Sections.begin();
----------------
`if (!Sections.empty())` maybe?


================
Comment at: tools/llvm-objcopy/Object.h:136-141
+  uint64_t Entry;
+  uint64_t SHOffset;
+  uint32_t Type;
+  uint32_t Machine;
+  uint32_t Version;
+  uint32_t Flags;
----------------
The size of some of these fields is dependent on the Elf Type (64 versus 32). I wonder if these should be based on ELFT? On the other hand, I'm quite happy that we just use the largest size for now.

At the moment, since the template is only instantiated for Elf64LE, this is fine, but it won't work once 32-bit support is added, unless the point where they are assigned to the Elf Header struct is modified to cast the values accordingly.


Repository:
  rL LLVM

https://reviews.llvm.org/D33964