[PATCH] D40523: [llvm-objcopy] Add --only-keep-debug

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I just realised one fairly major aspect of this that isn't tested, namely the behaviour when there are segments in play. Could you add tests that demonstrate what the behaviour is here - I expect the contents to effectively be wiped, by changing the file size to zero (although I'm not sure if that is what actually happens here - could you point out where it does, if it does). GNU objcopy's behaviour is a little weird around here - the segment offsets in a simple case I just tried were outside the file! I don't think we need to match it, but I would expect something sensible.

In https://reviews.llvm.org/D40523#939690, @jakehehrlich wrote:

> I'm writing code for --add-gnu-debuglink right now which requires section adding. Section adding raises some interesting questions like "what should OriginalOffset be?" If I can resolve those issues in a consistent way then "--edit-shdr <section>=<stuff>" can be "-R <section> -add-section <section>=<stuff>". Adding a specific section like the debuglink section is a tad different but the major technical challenge is the same. That would let us add tests for these strange tests without modifying yaml2obj. In general it would let us get around all the cases where yaml2obj holds us back.


Yes, I like this idea. However, we'd need the ability to explicitly specify where the new section is placed in the ELF. Otherwise, we end up moving the section at the same time as modifying it, which may not be desirable.

In https://reviews.llvm.org/D40523#939723, @jakehehrlich wrote:

> 4. I feel like I did something else that was mentioned as a thing I should do but I can't remember what it was.


I think it was the test renaming.



================
Comment at: test/tools/llvm-objcopy/only-keep-debug.test:30
+  Global:
+    - Name: foo
+      Section: .text
----------------
Could you make this symbol at a non-zero offset, and then test that the value is preserved, please.


================
Comment at: test/tools/llvm-objcopy/only-keep-debug.test:37-46
+# CHECK: Name: .debugfoo
+# CHECK: Name: .note
+# CHECK: Name: .blarg
+# CHECK: Name: .data
+# CHECK-NEXT: Type: SHT_NOBITS
+# CHECK: Name: .text
+# CHECK-NEXT: Type: SHT_NOBITS
----------------
You should probably check that the types of .debugfoo, .note, etc haven't been changed as well.

I would also recommend adding non-zero content to all sections, and testing that it is preserved for those that are not replaced.


================
Comment at: tools/llvm-objcopy/Object.cpp:694
+  // It's not necessarily safe to just change the section type so instead we
+  // make whole new section and delete the old way. This means removing any
+  // remaining references and replacing them with the new one as well.
----------------
make whole -> make a whole

What does "delete the old way" mean?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:171
 
+  // Replace sections before removing sections
+  if (OnlyKeepDebug) {
----------------
jhenderson wrote:
> Full stop. Why not do the replacing after the removal? Then there's less to replace, potentially.
Ping.


Repository:
  rL LLVM

https://reviews.llvm.org/D40523