[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 a comment.

Still quite a few comment changes, and one or two that were missed, so I'm going to ask you to update them again, before giving a LGTM. Sorry!



================
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:
> 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.
Ping here.


================
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:
> 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."
> 
Ping here.


================
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,
----------------
jhenderson wrote:
> 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.
Okay, my bad. That didn't come out quite as I meant/felt it would, and it now feels a little unnatural. As I believe that the LLVM standard is for full sentence, that first sentence needs to be either "This function finds a..." or "Find a...". Also, you've got "of list" when you need "list of".

Depending on which of the two suggestions you use for the first sentence, you should change the start of the second and third sentences accordingly. If the first approach is used, don't change the second sentence and merge the two: "It assumes ... OrderSegments, and returns ...". If using the second approach, you need to put "This function" instead of the first "It". Optionally, you can then fold the two sentences together as in the previous case.


================
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>
----------------
jhenderson wrote:
> 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.
You've used too many "returns" in this comment now. Should be "It returns either the ... ParentSegment, or an offset ..."


================
Comment at: tools/llvm-objcopy/Object.cpp:903
+  TotalSize = 0;
+  for (const auto Section : AllocatedSections) {
+    if (Section->Type != SHT_NOBITS)
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > const auto &
> AllocatedSections is a vector of pointers so we shouldn't need to add that. Is there a reason in to add the reference that I'm not aware of? It's not as if it harms anything by being there, I'm just curious. I went ahead and added it in case there's some reason it should be done so that this doesn't block an LGTM.
Good point, I missed this. Feel free to remove it again, if you prefer.


Repository:
  rL LLVM

https://reviews.llvm.org/D39713