[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: 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.
----------------
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?


================
Comment at: tools/llvm-objcopy/Object.cpp:715
+// This function assumes that Segments have been sorted by OrderSegments. This
+// function returns an Offset past any segment thus far layed out.
+static uint64_t LayoutSegments(std::vector<Segment *> &Segments,
----------------
I think we can be a little more specific, and say "one past the end of the last segment" in this comment. Unless I've missed something?

Nit: here and elsewhere layed -> laid.


================
Comment at: tools/llvm-objcopy/Object.cpp:742
+// Finds a consistent layout of a list of sections. This function assumes that
+// the ->ParentSegment of each section has already been layed out in some
+// consistent fashion. The supplied starting Offset is used for the starting
----------------
Given that we now have a function called "LayoutSegments", I don't think you need to say "in some consistent fashion" here, since being laid out implies to me that the corresponding function has been called.


================
Comment at: tools/llvm-objcopy/Object.cpp:788
   }
+  // Now we can decide segment layout.
+  Offset = LayoutSegments(OrderedSegments, Offset);
----------------
I'm not sure you need to have this and the next comment - the function names are quite clear as to what they do.


================
Comment at: tools/llvm-objcopy/Object.cpp:792
+  Offset = LayoutSections(this->Sections, Offset);
+  // If we need to write the section header table out then we need align the
+  // Offset so that SHOffset is valid.
----------------
we need align -> we need to align.


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


================
Comment at: tools/llvm-objcopy/Object.cpp:870
+  auto End =
+      std::unique(std::begin(OrderedSegments), std::end(OrderedSegments));
+  OrderedSegments.erase(End, std::end(OrderedSegments));
----------------
Why do we need to std::unique this list?


================
Comment at: tools/llvm-objcopy/Object.cpp:873
+
+  // Modify the first segment so that p_offset = sh_offset of same segment by
+  // chopping the front part off. This allows our layout algorithm to proceed as
----------------
This first sentence doesn't quite make sense - segment's don't have a sh_offset.


================
Comment at: tools/llvm-objcopy/Object.cpp:894
+  // constructed from an iterator that skips values for which a predicate does
+  // not hold. Then pass such a range to LayoutSections instead of cosntructing
+  // AllocatedSections here.
----------------
cosntructing -> constructing


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


Repository:
  rL LLVM

https://reviews.llvm.org/D39713