[PATCH] D39713: [llvm-objcopy] Change -O binary to respect section removal and behave like GNU objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/two-seg-remove-end.test:59
+# CHECK: 01000 de ad be ef
+# CHECK: 02000 32 32 32 32
+
----------------
It's not particularly important, but this and several of the other tests look like they're not testing for the correct number of digits in the address.


================
Comment at: tools/llvm-objcopy/Object.cpp:857
 template <class ELFT> void BinaryObject<ELFT>::finalize() {
-  // Put all segments in offset order.
-  auto CompareSegments = [](const SegPtr &A, const SegPtr &B) {
-    return A->Offset < B->Offset;
-  };
-  std::sort(std::begin(this->Segments), std::end(this->Segments),
-            CompareSegments);
-
-  uint64_t Offset = 0;
-  for (auto &Segment : this->Segments) {
-    if (Segment->Type == llvm::ELF::PT_LOAD &&
-        Segment->firstSection() != nullptr) {
-      Offset = alignToAddr(Offset, Segment->VAddr, Segment->Align);
-      Segment->Offset = Offset;
-      Offset += Segment->FileSize;
+  // We need a temporary list of segments that has a special order to it
+  // so that we know that anytime ->ParentSegment is set that segment has
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > I'm not a massive fan of this duplicate comment, but I also see that it's not necessarily trivial to remove the duplication. About the best idea I had was to pass a predicate into OrderSegments, which has the responsibility of returning the copied and sorted vector, filtered according to the predicate. If this sounds sensible to you, that would be a good way of getting rid of some of this duplication.
> > 
> > Looking further down, this sounds quite similar to what you are proposing with the LayoutSections in your new TODO. If you want to defer this to later, please add a TODO here as well.
> Yeah This should be solvable by the same filter range idea that I propose below. I'll add a TODO for that since I think I'd like the filter range to reside in Iterator.h not inside this code.
Sounds fair. Thinking about it, this loop is basically just a std::copy_if followed by a std::transform.


================
Comment at: tools/llvm-objcopy/Object.cpp:870
+  auto End =
+      std::unique(std::begin(OrderedSegments), std::end(OrderedSegments));
+  OrderedSegments.erase(End, std::end(OrderedSegments));
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Why do we need to std::unique this list?
> Notice that we construct OrderedSegments by adding the ParentSegment of each section. There might be a single segment for multiple sections so we need to dedup these so that LayoutSegment doesn't do crazy things.
Okay, makes sense. Please add a comment to this effect.


================
Comment at: tools/llvm-objcopy/Object.cpp:905
+  // Now that every section has been layed out we just need to compute the total
+  // size. This might not have anything to do with Offset however because a
+  // segment might go well past a used allocated section.
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > I'd be explicit, and say that "total file size". I'm not sure I understand the second sentence at all (and therefore why we can't just use Offset).
> Say the last section of the last segment was removed. Then Offset will be past the last section but to be consistent with GNU objcopy we need TotalSize to be such that the output ends at the end of the section, not the end of the segment. Offset == TotalSize is true if there either isn't a gap at the end of the section or if an extra allocated section is added that isn't in a segment.
Ok, I understand. I still think the comment needs rewriting though. How about something like:

"Now that every section has been laid out we just need to compute the total file size. This might not be the same as the offset returned by LayoutSections, because we want to truncate the last segment to the end of its last section, to match GNU objcopy's behaviour."



================
Comment at: tools/llvm-objcopy/Object.cpp:708
+// This function assumes that Segments have been sorted by OrderSegments. This
+// function returns an Offset one past the end of the last segment.
+static uint64_t LayoutSegments(std::vector<Segment *> &Segments,
----------------
I don't think you need to repeat "This function" at the start of the second sentence. Simply "It", or combining the two sentences would suffice.


================
Comment at: tools/llvm-objcopy/Object.cpp:739
+// starting Offset is used for the starting offset of any section that does not
+// have a ParentSegment.
+template <class SecPtr>
----------------
Please comment what exactly is returned here. By my understanding, it will be the end offset of the last segment, if there are no sections outside segments, or the end offset of the last section.


================
Comment at: tools/llvm-objcopy/Object.cpp:768
+  // so that we know that anytime ->ParentSegment is set that segment has
+  // already had it's offset properly set.
+  std::vector<Segment *> OrderedSegments;
----------------
it's -> its


================
Comment at: tools/llvm-objcopy/Object.cpp:903
+  TotalSize = 0;
+  for (const auto Section : AllocatedSections) {
+    if (Section->Type != SHT_NOBITS)
----------------
const auto &


Repository:
  rL LLVM

https://reviews.llvm.org/D39713