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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:714
+// Finds a consistent layout for a of list segments starting from an Offset.
+// This function assumes that Segments have been sorted by OrderSegments. This
+// function returns an Offset past any segment thus far layed out.
----------------
jhenderson wrote:
> I wonder if it might be useful to have an is_sorted assert here? What do you think?
> 
> Alternatively, why not just sort them here as were doing before?
Adding the assert seems like a good idea. I separated sorting out because I need to know the OriginalOffset of the first segment in order to give LayoutSegments the starting offset.


================
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
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.cpp:870
+  auto End =
+      std::unique(std::begin(OrderedSegments), std::end(OrderedSegments));
+  OrderedSegments.erase(End, std::end(OrderedSegments));
----------------
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.


================
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.
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D39713