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

Wed Feb 9 01:08:16 PST 2022

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Looks good, barring a couple of suggestions. You might want to confirm with @MaskRay too.

================
Comment at: llvm/test/tools/llvm-nm/option-X.test:62-64
+# RUN:   FileCheck --check-prefixes=BIT32 %s --implicit-check-not={{.}}
+# RUN: llvm-nm --format=just-symbols -X32 %t.a | \
+# RUN:   FileCheck --check-prefixes=ARC32,BIT32 %s -DFILE=%basename_t --implicit-check-not={{.}}
----------------
Here and below for the 64-bit equivalent, I'd make the `BIT32` prefix `XCOFF32`, since it's specifically testing XCOFF, and not other formats (`ARC32` can stay the same if you want, or change it as you feel is appropriate).

================
Comment at: llvm/test/tools/llvm-nm/option-X.test:66
+# ARC32:      [[FILE]].tmp_xcoff32.o:
+# BIT32:      var_0x1DF
+
----------------
FWIW, in this particular case, I'd be okay if you wanted to use a second -D option to name the variable, so that it is a little more explicit. Up to you either way, whichever you prefer.

================
Comment at: llvm/test/tools/llvm-nm/option-X.test:104
+# RUN: not llvm-nm --format=just-symbols -X33 %t_xcoff32.o 2>&1 |  FileCheck %s --check-prefixes=ERR
+# ERR:      error: : -X value should be one of: 32, 64, 32_64, (default) any 
+# ERR-NEXT: var_0x1DF
----------------
I'm pretty sure the double colon is a bug, but I guess it's a bug elsewhere in the code not touched by this patch. You might want to fix it in a separate patch, if you have time.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193