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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 17 03:45:28 PDT 2019


jhenderson marked 4 inline comments as done.
jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/dynsym-error-remove-strtab.test:9
+
+# NO-ERR-NOT: {{.}}
+# SECTIONS: .dynsym
----------------
rupprecht wrote:
> 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
I guess so. It's just a nice contrast to the first test case that's all, but I can remove it if you think it's unnecessary.


================
Comment at: test/tools/llvm-objcopy/ELF/reloc-error-remove-symtab.test:40-41
+# SECTIONS:        Name: .rel.text
+# SECTIONS:        Link
+# SECTIONS-SAME: : 0
----------------
rupprecht wrote:
> Maybe just: `# SECTIONS: Link{{.*}}: 0`?
Same issue. It could match incorrectly against a later instance rather than the one we care about.


================
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",
----------------
rupprecht wrote:
> 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
I don't disagree, but can this be a subsequent patch, as it affects an error message that isn't currently changing?


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:12
 
+def allow_broken_links
+    : Flag<["-", "--"], "allow-broken-links">,
----------------
rupprecht wrote:
> 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.
Sounds reasonable.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60324





More information about the llvm-commits mailing list