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

Wed Nov 29 02:40:45 PST 2017

jhenderson added a comment.

> Maybe I should add a hidden --make-nobits=<section> option for the sake of testing this stuff.

I kind of like this idea, but if you wanted to do it, I might take it one step further and do something like --change-type=<section>,<new type> or similar. Maybe even something like --edit-shdr=<section index or name>,<new section properties> where <new section properties> is similar to the assembler .section directive, so takes the form <new name>,<flags>,<type>,..., and then I see no reason to make it hidden, since a tool that can effectively binary edit the section header fields is quite useful for wider testing, at the very least.

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

> The type is how we know how to cast things. It's effectively our RTTI (perhaps that wasn't a good choice). To be honest I think changing to SHT_NOBITS would probably be harmless but that wouldn't be the case for other section types and I'm not 100% sure it's safe. Moreover I don't know that I want to add the mental burdon of worrying about weather or not a class's section type changed or not at some point. Also when we call writeSection every writeSection would have to check if it was a NOBITS section or not. We could also check that case in the Object::writeSections. Layout would happen correctly I think so no changes would need to occur there. removeSectionReference could throw an error as when it should as well. In general it isn't clear to me what all the repercussions would be to just changing the type. If we create a new section then it's clear that there won't be any surprising issues.

Okay, that makes a lot of sense (although your function is "replaceWithNoBits", not "replace with arbitrary type"). I missed the fact that you are creating generic "Section" instances, rather than one of the various section sub-classes, and that it has no contents, so what you do there is reasonable. I do think it needs some code comments to explain some of this though.

> Yeah that doesn't wind up being what it does in practice and I don't think we really want it to do that since there are other things like .note, .comment, .gnu.warning, etc might be nice to have. As near as I can tell it only makes the allocated sections into NOBITS sections. In general since the debug information is quite big I think keeping around extra possibly helpful information isn't too big of a deal. It also saves us from explicitly keeping a bunch of particular magic sections.

In other words - don't rely on the documentation being complete or accurate!

================
Comment at: test/tools/llvm-objcopy/replace-text-symtab.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -only-keep-debug %t %t2
----------------
jakehehrlich wrote:
> 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.
OK. The test rename, and a new test specifically testing only-keep-debug would work for me. Please rename the other tests in a similar manner.

================
Comment at: tools/llvm-objcopy/Object.cpp:163
+  if (SymbolNames == Old) {
+    error("String table " + SymbolNames->Name +
+          " cannot be replaced because it is referenced by "
----------------
jakehehrlich wrote:
> 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.
Okay, that makes sense. I'd have probably written this as an assert, since we can't get into the case currently, but I'm happy with your approach here.

Once again though, this is something that we could unit-test if we figured out how to set up unit tests for llvm-objcopy.

================
Comment at: tools/llvm-objcopy/Object.cpp:685
+    std::function<bool(const SectionBase &)> ToReplace) {
+
+  if (SymbolTable != nullptr && ToReplace(*SymbolTable))
----------------
Unnecessary blank line?

Repository:
  rL LLVM

https://reviews.llvm.org/D40523