Page MenuHomePhabricator

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

Authored by jhenderson on Apr 2 2019, 8:58 AM.

Details

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).

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Apr 2 2019, 8:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 8:58 AM
rupprecht accepted this revision.Apr 2 2019, 11:43 AM
rupprecht added inline comments.
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)

This revision is now accepted and ready to land.Apr 2 2019, 11:43 AM
mattd added a comment.Apr 2 2019, 8:23 PM

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

mattd accepted this revision.Apr 2 2019, 8:23 PM
grimar added a comment.Apr 3 2019, 1:50 AM

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 marked 2 inline comments as done.Apr 3 2019, 2:05 AM

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.

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 updated this revision to Diff 193466.Apr 3 2019, 4:29 AM
jhenderson edited the summary of this revision. (Show Details)

Address review comments:

  • Update test
  • Add cl::ZeroOrMore to switches
  • Move logic a bit later
jhenderson requested review of this revision.Apr 3 2019, 4:30 AM

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

grimar accepted this revision.Apr 3 2019, 4:47 AM

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.

This revision is now accepted and ready to land.Apr 3 2019, 4:47 AM
This revision was automatically updated to reflect the committed changes.

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.