[PATCH] D60324: [llvm-objcopy] Add switch to allow removing referenced sections

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

lgtm, just a couple minor comments



================
Comment at: test/tools/llvm-objcopy/ELF/dynsym-error-remove-strtab.test:9
+
+# NO-ERR-NOT: {{.}}
+# SECTIONS: .dynsym
----------------
Isn't this kind of implicit? If there's an error, then llvm-objcopy will return a non-zero status and the test will fail


================
Comment at: test/tools/llvm-objcopy/ELF/reloc-error-remove-symtab.test:40-41
+# SECTIONS:        Name: .rel.text
+# SECTIONS:        Link
+# SECTIONS-SAME: : 0
----------------
Maybe just: `# SECTIONS: Link{{.*}}: 0`?


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:436
+          llvm::errc::invalid_argument,
+          "String table %s cannot be removed because it is "
+          "referenced by the symbol table %s",
----------------
We could make these error messages more helpful by saying something like:

String table cannot be removed ... Rerun with -R $name to also remove this section, or --allow-broken-links to suppress this error


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:12
 
+def allow_broken_links
+    : Flag<["-", "--"], "allow-broken-links">,
----------------
The examples given seem to apply to strip as well, so can you add the option there too? And at least one test that uses strip to make sure it's hooked up.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60324/new/

https://reviews.llvm.org/D60324