[PATCH] D33964: [LLVM][llvm-objcopy] Added basic plumbing to get things started

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

One more issue that I've put inline, along with a couple of comment typos, and then there's only the tests to sort out. Good work!

It would be good to get somebody else's eyes on this though, given the scale of the new code.



================
Comment at: tools/llvm-objcopy/Object.cpp:91
+  // to clarify the case when an empty section lies on a boundary between two
+  // segments and ensures that the section "belongs" to the second section and
+  // not the first.
----------------
"belongs" to the second section and -> "belongs" to the second segment and


================
Comment at: tools/llvm-objcopy/Object.cpp:205
+        // There can be gaps at the start of a segment before the first section.
+        // So first we asign the alignment of the segment and then assign the
+        // location of the section from there
----------------
asign -> assign


================
Comment at: tools/llvm-objcopy/Object.cpp:224
+    }
+    if (Section->Align)
+      Offset = alignTo(Offset, Section->Align);
----------------
I think this only should happen when the section is not in a segment. Otherwise, the section will move independently of the segment, and thus break our file image preservation. In most cases this is a no-op anyway, because the alignment should already be met, but in the case of slightly dodgy input (misaligned section within a segment), we should probably avoid making things worse by breaking the memory image.

If you do this, the comment at the start of the for loop should be updated.


Repository:
  rL LLVM

https://reviews.llvm.org/D33964