[PATCH] D36558: [llvm-objcopy] Add support for nested and overlapping segments

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Lots of small nits, and one or two slightly larger issues to address here. I like the overall approach though.

I'm not all that happy with the level of test coverage available, but at the same time, I'm not sure that there's much that can be done without unit-testing a lot of this, or creating large numbers of virtually identical Lit tests, which seems a little excessive.



================
Comment at: tools/llvm-objcopy/Object.cpp:217
+// Returns true IFF a segment's original offset is inside of another segment's
+// range
+static bool segmentOverlappsSegment(const Segment &Child,
----------------
Nit: full stop.


================
Comment at: tools/llvm-objcopy/Object.cpp:218
+// range
+static bool segmentOverlappsSegment(const Segment &Child,
+                                    const Segment &Parent) {
----------------
segmentOverlappsSegment -> segmentOverlapsSegment 


================
Comment at: tools/llvm-objcopy/Object.cpp:221-222
+
+  return Parent.OriginalOffset <= Child.OriginalOffset &&
+         Parent.OriginalOffset + Parent.FileSize >= Child.OriginalOffset;
+}
----------------
I don't think this is quite right - in the case of two adjacent segments (i.e. where the end of one and the start of the next are the same), the second ends up being treated as a child of the first. I think the second clause should be strictly greater than.


================
Comment at: tools/llvm-objcopy/Object.cpp:253-254
   }
+  // Now we do an O(n^2) loop though the segments in order to match up nested
+  // segments up.
+  for (auto &Child : Segments) {
----------------
Couple of typos in this comment: 1) though -> through; 2) match up nested segments up. -> match up segments.


================
Comment at: tools/llvm-objcopy/Object.cpp:257
+    for (auto &Parent : Segments) {
+      // Every segment will be a child of itself but we don't want a segment
+      // to be it's own parent so we avoid that situation.
----------------
"will be a child of itself" should probably be "will overlap with itself", to match the function name.


================
Comment at: tools/llvm-objcopy/Object.cpp:260
+      if (&Child != &Parent && segmentOverlappsSegment(*Child, *Parent)) {
+        // We want a cononical "most parental" segment but this requires
+        // inspecting the ParentSegment
----------------
cononical -> canonical


================
Comment at: tools/llvm-objcopy/Object.cpp:261
+        // We want a cononical "most parental" segment but this requires
+        // inspecting the ParentSegment
+        if (Child->ParentSegment != nullptr) {
----------------
Nit: full stop.


================
Comment at: tools/llvm-objcopy/Object.cpp:269
+            Child->ParentSegment = Parent.get();
+          }
+        } else {
----------------
I think there's one case missing here - we don't want the case where two segments with identical offsets and file sizes to be parents of each other. In this situation, we also can't simply say something like "if (Parent->ParentSegment == Child) do nothing", you could end up with segment 1 being parent of segment 2 which is parent of segment 3 which is parent of segment 1. I think in that situation, the segment that is first in the table should be treated as the parent of the other two.


================
Comment at: tools/llvm-objcopy/Object.cpp:278
 
+
 template <class ELFT>
----------------
Nit: unnecessary blank line.


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


================
Comment at: tools/llvm-objcopy/Object.cpp:509
+    // We assume that segments have been ordered by OriginalOffset and Size
+    // such that a parrent segment will always come before a child segment in
+    // Segments. This means that the Offset of the ParentSegment should already
----------------
parrent -> parent


================
Comment at: tools/llvm-objcopy/Object.cpp:511
+    // Segments. This means that the Offset of the ParentSegment should already
+    // be set and we can set our offset relative to it
+    if (Segment->ParentSegment != nullptr) {
----------------
Full stop.


================
Comment at: tools/llvm-objcopy/Object.cpp:513
+    if (Segment->ParentSegment != nullptr) {
+      auto PSeg = Segment->ParentSegment;
+      Segment->Offset = PSeg->Offset + Segment->Offset - PSeg->Offset;
----------------
Could you rename PSeg to Parent, please?


================
Comment at: tools/llvm-objcopy/Object.cpp:514
+      auto PSeg = Segment->ParentSegment;
+      Segment->Offset = PSeg->Offset + Segment->Offset - PSeg->Offset;
+    } else {
----------------
Should this be:
`Segment->Offset = PSeg->Offset + Segment->OriginalOffset - PSeg->OriginalOffset;`
?


Repository:
  rL LLVM

https://reviews.llvm.org/D36558