[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]

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