[llvm-nm]Add support for --no-demangle

[jhenderson]

Summary

GNU nm has --no-demangle, so llvm-nm should too. It disables the --demangle switch. The patch also allows --demangle to be specified multiple times (the last of all --no-demangle/--demangle switches takes precedence).

[rupprecht]

tools/llvm-nm/llvm-nm.cpp

2083 ↗

(On Diff #193307)

Can you add a test case that ensures getPosition() returns the last occurrence?
e.g. -C --no-demangle -C should be demangled because NoDemangle.getPosition() is 1, and Demangle.getPosition() is 2 and not 0 (or whatever indexing it uses)

[mattd]

LGTM, as long as @rupprecht's comment is addressed.

[grimar]

Seems llvm-nm is inconsistent.

• -no-dyldinfo can't be used with -add-dyldinfo/-dyldinfo-only
• -no-sort always overrides all other kinds of sorts.

And this patch implements the correct behavior of the normal option the first time it seems.

tools/llvm-nm/llvm-nm.cpp

2085 ↗

(On Diff #193307)

I am not sure this piece of code should be right here.

It is placed before the llvm::InitializeAll* calls, what is a bit too far from where the other values (like OutputFormat)
are set. What about moving it below? For example before the llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);.
I would expect such options that have -no-* flags to be grouped and together.

[jhenderson]

In D60134#1452786, @grimar wrote:

And this patch implements the correct behavior of the normal option the first time it seems.

Thanks. I took the behaviour from --no-demangle in llvm-symbolizer, so I can't exactly take all the credit!

tools/llvm-nm/llvm-nm.cpp

2083 ↗

(On Diff #193307)

Sounds good.

2085 ↗

(On Diff #193307)

Thanks. I placed it here, because the equivalent code in llvm-symbolizer is at this location. I can move it though.

[jhenderson]

In D60134#1452806, @jhenderson wrote:

In D60134#1452786, @grimar wrote:

And this patch implements the correct behavior of the normal option the first time it seems.

Thanks. I took the behaviour from --no-demangle in llvm-symbolizer, so I can't exactly take all the credit!

We spoke too soon. Updating the test as @rupprecht suggested highlighted an issue where the switches cannot appear multiple times on the command-line. There's no good reason why they shouldn't (aside from tidiness), and I have encountered build systems and scripts where appending options to an existing command-line is about the only choice available, even if it means duplicate options, so we should allow multiple occurrences. I'll update the patch accordingly.

[jhenderson]

Address review comments:

• Update test
• Add cl::ZeroOrMore to switches
• Move logic a bit later

[jhenderson]

Given the issue I ran into, could I get this reviewed again before submitting, please.

[grimar]

LGTM, comment inline.

test/tools/llvm-nm/X86/demangle.ll

23 ↗

(On Diff #193466)

It feels to me a bit excessive to test all possible combination with aliases.
I think lines 4 and 5 of this test are used to demonstrate that -C and --demangle are equal.
(and the testing of cl::opt vs cl::alias interaction is done in llvm\unittests\Support\CommandLineTest.cpp already).
So I would not do it. And do just:

; RUN: llvm-nm --demangle --no-demangle %t.o | FileCheck --check-prefix="MANGLED" %s
; RUN: llvm-nm --no-demangle --demangle %t.o | FileCheck --check-prefix="DEMANGLED" %s
; RUN: llvm-nm --no-demangle --demangle --no-demangle %t.o | FileCheck --check-prefix="MANGLED" %s
; RUN: llvm-nm --demangle --no-demangle --demangle %t.o | FileCheck --check-prefix="DEMANGLED" %s

Otherwise, we might end up with a crazy amount of lines in the tests.
Up to you probably.

[rupprecht]

In D60134#1452819, @jhenderson wrote:

In D60134#1452806, @jhenderson wrote:

In D60134#1452786, @grimar wrote:

And this patch implements the correct behavior of the normal option the first time it seems.

Thanks. I took the behaviour from --no-demangle in llvm-symbolizer, so I can't exactly take all the credit!

We spoke too soon. Updating the test as @rupprecht suggested highlighted an issue where the switches cannot appear multiple times on the command-line. There's no good reason why they shouldn't (aside from tidiness), and I have encountered build systems and scripts where appending options to an existing command-line is about the only choice available, even if it means duplicate options, so we should allow multiple occurrences. I'll update the patch accordingly.

Oh, that's weird. Why isn't that the default?
Not necessarily just for this reason, but it would fix it -- we should probably switch the tools to use liboption, which was discussed on the mailing list some time last year. Anyway, what was submitted lgtm.

llvm/trunk/test/tools/llvm-nm/X86/demangle.ll

 ; RUN: llc -filetype=obj -mtriple=x86_64-pc-linux -o %t.o %s
 ; RUN: llvm-nm %t.o | FileCheck --check-prefix="MANGLED" %s
+; RUN: llvm-nm %t.o --no-demangle | FileCheck --check-prefix="MANGLED" %s
 ; RUN: llvm-nm -C %t.o | FileCheck --check-prefix="DEMANGLED" %s
 ; RUN: llvm-nm --demangle %t.o | FileCheck --check-prefix="DEMANGLED" %s
 
 ; RUN: llc -filetype=obj -mtriple=x86_64-apple-darwin9 -o %t.macho %s
 ; RUN: llvm-nm %t.macho | FileCheck --check-prefix="MACHO-MANGLED" %s
 ; RUN: llvm-nm -C %t.macho | FileCheck --check-prefix="DEMANGLED" %s
 
 ; RUN: llc -filetype=obj -mtriple=x86_64-pc-win32 -o %t.coff %s
 ; RUN: llvm-nm %t.coff | FileCheck --check-prefix="COFF-MANGLED" %s
 ; RUN: llvm-nm -C %t.coff | FileCheck --check-prefix="COFF-DEMANGLED" %s
 
+; Show that the last of --no-demangle/--demangle wins:
+; RUN: llvm-nm --demangle --no-demangle %t.o | FileCheck --check-prefix="MANGLED" %s
+; RUN: llvm-nm --no-demangle --demangle %t.o | FileCheck --check-prefix="DEMANGLED" %s
+; RUN: llvm-nm --no-demangle --demangle --no-demangle %t.o | FileCheck --check-prefix="MANGLED" %s
+; RUN: llvm-nm --demangle --no-demangle --demangle %t.o | FileCheck --check-prefix="DEMANGLED" %s
+
 define i32 @_Z3fooi(i32) #0 {
 entry:
   ret i32 1
 }
 
 define float @_Z3barf(float) #0 {
 entry:
   ret float 0.000000e+00

llvm/trunk/tools/llvm-nm/llvm-nm.cpp

cl::alias NumericSortn("n", cl::desc("Alias for --numeric-sort"),
                        cl::aliasopt(NumericSort), cl::Grouping);
 cl::alias NumericSortv("v", cl::desc("Alias for --numeric-sort"),
                        cl::aliasopt(NumericSort), cl::Grouping);
 
 cl::opt<bool> NoSort("no-sort", cl::desc("Show symbols in order encountered"));
 cl::alias NoSortp("p", cl::desc("Alias for --no-sort"), cl::aliasopt(NoSort),
                   cl::Grouping);
 
-cl::opt<bool> Demangle("demangle", cl::desc("Demangle C++ symbol names"));
-cl::alias DemangleC("C", cl::desc("Alias for --demangle"), cl::aliasopt(Demangle),
-                    cl::Grouping);
+cl::opt<bool> Demangle("demangle", cl::ZeroOrMore,
+                       cl::desc("Demangle C++ symbol names"));
+cl::alias DemangleC("C", cl::desc("Alias for --demangle"),
+                    cl::aliasopt(Demangle), cl::Grouping);
+cl::opt<bool> NoDemangle("no-demangle", cl::init(false), cl::ZeroOrMore,
+                         cl::desc("Don't demangle symbol names"));
 
 cl::opt<bool> ReverseSort("reverse-sort", cl::desc("Sort in reverse order"));
 cl::alias ReverseSortr("r", cl::desc("Alias for --reverse-sort"),
                        cl::aliasopt(ReverseSort), cl::Grouping);
 
 cl::opt<bool> PrintSize("print-size",
                         cl::desc("Show symbol size instead of address"));
 cl::alias PrintSizeS("S", cl::desc("Alias for --print-size"),
if (SizeSort && !PrintSize)
     PrintAddress = false;
   if (OutputFormat == sysv || SizeSort)
     PrintSize = true;
   if (InputFilenames.empty())
     InputFilenames.push_back("a.out");
   if (InputFilenames.size() > 1)
     MultipleFiles = true;
 
+  // If both --demangle and --no-demangle are specified then pick the last one.
+  if (NoDemangle.getPosition() > Demangle.getPosition())
+    Demangle = !NoDemangle;
+
   for (unsigned i = 0; i < ArchFlags.size(); ++i) {
     if (ArchFlags[i] == "all") {
       ArchAll = true;
     } else {
       if (!MachOObjectFile::isValidArch(ArchFlags[i]))
         error("Unknown architecture named '" + ArchFlags[i] + "'",
               "for the -arch option");
     }