[PATCH] D118193: [llvm-nm] add a new option -X to specify the type of object file llvm-nm should examine
Digger Lin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 3 13:10:34 PST 2022
DiggerLin marked 9 inline comments as done.
DiggerLin added inline comments.
================
Comment at: llvm/test/tools/llvm-nm/option-X-elf-bitcode.test:1
+## Test the option "-X" of llvm-nm for bitcode and elf object file.
+
----------------
jhenderson wrote:
> Nit: the file format should be all caps.
>
> It's not clear to me why this test contains test cases for two formats (ELF and bitcode), but not all formats supported by this option. In other words, could you please explain why your XCOFF case is in a separate test file to these two (and why you've kept ELF and bitcode in the same one)?
As I mention before, the -X only support for XCOFF, bitcode, ELF object file, not support for other object file in the patch.
xcoff is test on the llvm/test/tools/llvm-nm/XCOFF/option-X.test,
you suggestion moving the content of test llvm/test/tools/llvm-nm/XCOFF/option-X.test into the file too?
================
Comment at: llvm/test/tools/llvm-nm/option-X-elf-bitcode.test:3-4
+
+# RUN: llvm-as -o %t32.bc %p/Inputs/bitcode-sym32.ll
+# RUN: llvm-as -o %t64.bc %p/Inputs/bitcode-sym64.ll
+
----------------
jhenderson wrote:
> These files don't exist. Did you forget to add them?
the two files are in the patch "--export-symbols" ,
================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1764-1766
+ if (!isa<XCOFFObjectFile>(Obj) && !isa<ELFObjectFileBase>(Obj) &&
+ !isa<IRObjectFile>(Obj))
+ return true;
----------------
jhenderson wrote:
> It's fine not to add support for COFF and Mach-O in this patch, but something to consider is what might happen if a user attempted to use that option with an object in that format. The option would be silently ignored, but it wouldn't be clear why to them. I'd consider adding a warning here, if the option has been specified, but the input object is not a supported format.
>
> Alternatively, just add support for all supported formats up-front.
I think we have some description in the command guide.
.. option:: -X
Specify the type of XCOFF object file, ELF object file, or IR object file input
from command line or from archive files that llvm-nm should examine.
if I change the code to
```
if (!isa<XCOFFObjectFile>(Obj) && !isa<ELFObjectFileBase>(Obj) &&
!isa<IRObjectFile>(Obj)) {
if(BitMode !=BitModeTy::Any)
error("-X option is currently only implemented for XCOFF, ELF, and IR object files");
return true;
}
```
it will print out a warning info for each object files in a archive file, there many a lot of same warning print out. It do not look reasonable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118193/new/
https://reviews.llvm.org/D118193
More information about the llvm-commits
mailing list