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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: test/tools/llvm-objcopy/replace-text-symtab.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -only-keep-debug %t %t2
----------------
jhenderson wrote:
> It's not clear to me what the purpose of the "symtab" bit is in the test name. Also, since this is testing the only-keep-debug switch, I think the test should say so in the name. The "replace" part isn't really relevant. Same sort of comment could apply to the other two tests.
So my intent here was a bit more subtle but certainly I haven't made that clear in the test. I was specifically trying to trigger the case where the symbol table needs to replace a section reference for a symbol. So maybe this should be "replace-text-ref-in-symtab.test"?

only-keep-debug just happens to be the option I used to do the replacing. Thinking about it, I never actually tried testing only-keep-debug. I only tested certain code paths. I should add that test.


================
Comment at: tools/llvm-objcopy/Object.cpp:163
+  if (SymbolNames == Old) {
+    error("String table " + SymbolNames->Name +
+          " cannot be replaced because it is referenced by "
----------------
jhenderson wrote:
> I don't think this case is tested at all.
In practice this can't come up with the options we have now but I think it's important to throw an error if someone tries to replace the wrong thing in code. It's an artifact of a more general interface existing that can be used. I don't really like that being the case but having the error and never being able to test it seems better than not having the error and one day the code silently failing.  I can be convinced otherwise though.


================
Comment at: tools/llvm-objcopy/Object.cpp:246
+  if (Symbols == Old) {
+    error("Symbol table " + Symbols->Name + " cannot be replaced because it is "
+                                            "referenced by the relocation "
----------------
jhenderson wrote:
> Test needed for this error case.
This is another one of those errors that I can't test with the current interface. There isn't a way to get yaml2obj to produce a relocation that dosn't have .symtab as its link and there isn't a way to make .symtab allocated so there isn't a way to get --only-keep-debug to trigger this error.


================
Comment at: tools/llvm-objcopy/Object.cpp:686
+  auto Iter =
+      std::stable_partition(std::begin(Sections), std::end(Sections),
+                            [=](const SecPtr &Sec) { return !ToRemove(*Sec); });
----------------
jhenderson wrote:
> Why do we do this extra sorting? Wouldn't it be easier (and probably faster) to just have an `if(!ToRemove(*Sec)){continue;}` inside the later for loop?
derp...Yeah that's way better. I was just blindly trying to refactor the code from removeSections and it pretty clearly wasn't all that relevant in the end.


================
Comment at: tools/llvm-objcopy/Object.cpp:691
+    SymbolTable = nullptr;
+  if (ToRemove(*SectionNames)) {
+    if (WriteSectionHeaders)
----------------
jhenderson wrote:
> And for these cases?
This is another error case I can't test because yaml2obj can't produce an allocated .shstrtab and --only-keep-debug can't make a non-allocated section into a NOBITS section. Maybe I should add a hidden --make-nobits=<section> option for the sake of testing this stuff.


Repository:
  rL LLVM

https://reviews.llvm.org/D40523