[PATCH] D118193: [llvm-nm] add a new option -X to specify the type of object file llvm-nm should examine

[James Henderson via Phabricator via llvm-commits]

jhenderson 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.
+
----------------
DiggerLin wrote:
> 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?
I'm suggesting one of either:

Putting the XCOFF case in this test.

OR

Splitting the bitcode and ELF cases into separate files.

I don't have a particular preference as to which.


================
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
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > These files don't exist. Did you forget to add them?
> the  two files are in the patch "--export-symbols" ,
This patch doesn't need to rely on the `--export-symbols` patch though, so it shouldn't really be rebased on top of it. Preferable would be making it an independent patch, and just have the new test inputs duplicated between the two patches (obviously when landing the two patches, you only need one version of the test inputs, so the second-to-land patch won't have them in).


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1764-1766
+  if (!isa<XCOFFObjectFile>(Obj) && !isa<ELFObjectFileBase>(Obj) &&
+      !isa<IRObjectFile>(Obj))
+    return true;
----------------
DiggerLin wrote:
> 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. 
People won't routinely look at the command guide, so documenting the limitation there is insufficient.

I do agree with your comment about not wanting to duplicate the error multiple times though. I don't have a good solution, but a small improvement would be to add the limitation to the `--help` text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193