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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D36558#862704, @jakehehrlich wrote:

> I think we should probably settle for adding a few tests (possibly more, please recommend more if you can think of any) here and then wait to add some stripping capability next. After stripping is implemented we can write better tests. I plan on adding some stripping capabilities right after dynamic stuff has been submitted. I'll start writing code for it soon and put it up for review as soon as possible.
>
> What kind of stripping would be most useful for testing at first? I think removing a section by name and then removing a symbol by name would be best. That way we can tailor tests to see what happens when specific sections/symbols are removed. After that we can implement more useful types of stripping that remove multiple sections and symbols at once.
>
> So my proposal is the following:
>
> 1. Compromise between adding a bunch of very similar .test files and unit testing by adding a few more .test files (please recommend more) and waiting for better tests to come up after we have section and symbol removal
> 2. Write section removal by name next.
> 3. Write a collection of tests using section removal  works well and to get more test coverage
> 4. Write symbol removal by name after that.
> 5. Write a collection of tests using symbol removal to get better coverage.
> 6. Add more advanced kinds of stripping like --strip-all and --strip-debug


I think what you've proposed here sounds reasonable. Section stripping is something I've made use of in the past, but not symbol stripping, so I think section stripping should be first, definitely. It would also allow us to test this area (nested segments) more directly as well.

As for suggested tests, I think you've done a pretty good job of covering the cases I can think of. One I didn't see, so might be missing, was two adjacent segments (i.e. the end of one is the same value as the start of the next), perhaps with different alignments, so that the first can move but not the second (or they can both move, but are not tied together, so one moves further than the other). That would test the second half of "segmentOverlapsSegment" I think. It might just be part of one of the other tests. As there are several tests now that test similar, but slightly different cases, I think you should add a comment in each test to describe what exactly is being tested (e.g. adjacent segments, segments that are identical, chains of segments that partially overlap), essentially describing why that particular case is interesting. For example, in the test I've suggested, the comment would read something like "Check the case where two non-empty segments are adjacent in the file, i.e. the end of one is the start of the next. In this case, the two should move independently of each other."



================
Comment at: test/tools/llvm-objcopy/same-segment.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy %t %t2
----------------
I'd call this test identical-segment.test (same-segment implies that two different things refer to one segment).


================
Comment at: test/tools/llvm-objcopy/tripple-overlap.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy %t %t2
----------------
tripple-overlap.test -> triple-overlap.test


================
Comment at: tools/llvm-objcopy/Object.cpp:432
+  for (auto &Segment : OrderedSegments) {
+    if (Segment->Type == PT_GNU_STACK)
+      continue;
----------------
Why is PT_GNU_STACK special?


Repository:
  rL LLVM

https://reviews.llvm.org/D36558