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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Honestly, I'm a little confused by the complexity of this patch. It seems to me like changing a section to SHT_NOBITS should be pretty straightforward, since it's just changing the value of one field in a section header. Perhaps you could explain this a little more, and where necessary, add some code comments to the same effect.

Also, looking at the man page (I haven't tested this), but it seems to me like it's not just allocatable sections that get replaced, but any non-debug section. In other words, I'd expect the inverse of the predicate for --strip-debug to be used as the predicate for replacing. Is that not what you've observed?



================
Comment at: test/tools/llvm-objcopy/replace-text-symtab.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -only-keep-debug %t %t2
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.cpp:163
+  if (SymbolNames == Old) {
+    error("String table " + SymbolNames->Name +
+          " cannot be replaced because it is referenced by "
----------------
I don't think this case is tested at all.


================
Comment at: tools/llvm-objcopy/Object.cpp:166
+          "the symbol table " +
+          this->Name);
+  }
----------------
Is the "this->" needed here?


================
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 "
----------------
Test needed for this error case.


================
Comment at: tools/llvm-objcopy/Object.cpp:327
+                                           "referenced by the section " +
+          this->Name);
+  }
----------------
Unnecessary "this->". Is this how clang-format formats this block? It's quite surprising if it is!


================
Comment at: tools/llvm-objcopy/Object.cpp:684
+void Object<ELFT>::replaceWithNoBits(
+    std::function<bool(const SectionBase &)> ToRemove) {
+  auto Iter =
----------------
I don't think this should be called "ToRemove", since it isn't removing anything, just replacing.


================
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); });
----------------
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?


================
Comment at: tools/llvm-objcopy/Object.cpp:690
+  if (SymbolTable != nullptr && ToRemove(*SymbolTable))
+    SymbolTable = nullptr;
+  if (ToRemove(*SectionNames)) {
----------------
Test for this case?


================
Comment at: tools/llvm-objcopy/Object.cpp:691
+    SymbolTable = nullptr;
+  if (ToRemove(*SectionNames)) {
+    if (WriteSectionHeaders)
----------------
And for these cases?


================
Comment at: tools/llvm-objcopy/Object.cpp:698-708
+    auto Tmp = std::move(Sec);
+    Sec = llvm::make_unique<Section>(ArrayRef<uint8_t>{});
+    Sec->Name = Tmp->Name;
+    Sec->Type = SHT_NOBITS;
+    Sec->Flags = Tmp->Flags;
+    Sec->Addr = Tmp->Addr;
+    Sec->Offset = Tmp->Offset;
----------------
Why do we need to do all this? Can't we just have the one line: `Sec->Type = SHT_NOBITS` and not create a completely new section?


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


Repository:
  rL LLVM

https://reviews.llvm.org/D40523