This is an archive of the discontinued LLVM Phabricator instance.

[llvm-symbolizer] Add -no-demangle as alias for -demangle=false
ClosedPublic

Authored by Quolyk on Jan 16 2019, 12:16 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Quolyk created this revision.Jan 16 2019, 12:16 AM

I'm not sure how do I handle situation when -demange and -no-demangle appear in one command. I mean what precedence to use.

I had a look at an option to disable demangling, as I worked on PR40054, but didn't come up with a clean way of handling both --demangle and --no-demangle, using the cl interface. There, I decided to abandon it, as demangling will be off by default, so we don't need to explicitly disable it. Here is different, because demangling is on by default (and the --demangle switch needs to exist for GNU compatibility). Using tablegen would make it possible to have both sensibly (see examples in e.g. LLD), but I don't think we should do that at this point.

Usually speaking in command-line tools, if there are both positive and negative versions of the option (e.g. --demangle and --no-demangle), the last one present on the command-line takes precedence, so I'd see if there's an API for that. If not, or it's not easy to use, you could probably just emit an error if both are specified.

test/DebugInfo/llvm-symbolizer.test
215 ↗(On Diff #181984)

Nit: use double dash to match other options in this file.

tools/llvm-symbolizer/llvm-symbolizer.cpp
66 ↗(On Diff #181984)

You don't need the cl::NotHidden here, as it is not a true alias option.

This should probably have an explicit cl::init(false), to "match" the ClDemangle option.

ruiu added a comment.Jan 16 2019, 8:30 AM

-demangle=false is a weird option. Has it been ever documented? If not, probably we should remove it and document only -no-demangle.

jhenderson added a comment.EditedJan 16 2019, 8:35 AM

-demangle=false is a weird option. Has it been ever documented? If not, probably we should remove it and document only -no-demangle.

I agree with you, hence why I filed the bug in the first place. The "=false" syntax is documented on the CommandLine library page (see https://llvm.org/docs/CommandLine.html#boolean-arguments), but I don't think it is documented explicit in the llvm-symbolizer page.

docs/CommandGuide/llvm-symbolizer.rst
92 ↗(On Diff #181984)

As noted by @ruiu, let's not document "demangle=false" explicitly, so remove the second sentence here.

Quolyk updated this revision to Diff 182267.EditedJan 17 2019, 6:06 AM

Update tests. Add logic. Honestly, I'm not good at writing llvm-symbolizer test, so I picked test that would fail if --demangle=true to test --demangle, --no-demangle order.
That doesn't mean I want to merge this patch as is. If more specific tests are needed I would be happy to dive into.

Quolyk marked 3 inline comments as done.Jan 17 2019, 6:08 AM
jhenderson added inline comments.Jan 17 2019, 6:37 AM
test/DebugInfo/llvm-symbolizer.test
42–46 ↗(On Diff #182267)

I think it would make more sense to extend the case below that you have already added --no-demangle to.

There you can test it something like the following (add the "DEMANGLED_FUNCTION_NAME" alongside the "SHORT_FUNCTION_NAME" check pattern):

RUN: llvm-symbolizer --functions=short --demangle --no-demangle < %t.input7 \
RUN:    | FileCheck %s --check-prefix=SHORT_FUNCTION_NAME
RUN: llvm-symbolizer --functions=short --no-demangle --demangle < %t.input7 \
RUN:    | FileCheck %s --check-prefix=DEMANGLED_FUNCTION_NAME

DEMANGLED_FUNCTION_NAME: c()
tools/llvm-symbolizer/llvm-symbolizer.cpp
211–212 ↗(On Diff #182267)

Nice! I didn't know about the getPosition function.

Quolyk updated this revision to Diff 182283.Jan 17 2019, 7:08 AM

Update tests.

jhenderson added inline comments.Jan 17 2019, 7:32 AM
test/DebugInfo/llvm-symbolizer.test
221 ↗(On Diff #182283)

Sorry, I realised that I misread the test. The existing --demangle=false tests are testing that the "short" name is NOT matched (that's what the -NOT suffix is for on the line above, so this isn't testing what you think it is testing.

Instead, you'll want some new tests below this and the corresponding checks. The --functions=short means that it is printing the short version of the function name, taken from the debug information, I think. In the new tests you write, you don't want the --functions switch at all, I believe.

Try experimenting with running llvm-symbolizer on %t.input7 and look at the output. You should see a function name c() appear somewhere, if I'm not mistaken. Assuming you do, double-check that --no-demangle turns it into _Z1cv, and then add something like the following tests at the end of the file:

; Check that the last of --demangle and --no-demangle wins.
RUN: llvm-symbolizer --demangle < %t.input7 \
RUN:    | FileCheck %s --check-prefix=DEMANGLED_FUNCTION_NAME
RUN: llvm-symbolizer --no-demangle < %t.input7 \
RUN:    | FileCheck %s --check-prefix=MANGLED_FUNCTION_NAME
RUN: llvm-symbolizer --demangle --no-demangle < %t.input7 \
RUN:    | FileCheck %s --check-prefix=MANGLED_FUNCTION_NAME
RUN: llvm-symbolizer --no-demangle --demangle < %t.input7 \
RUN:    | FileCheck %s --check-prefix=DEMANGLED_FUNCTION_NAME

MANGLED_FUNCTION_NAME: _Z1cv
DEMANGLED_FUNCTION_NAME: c()
tools/llvm-symbolizer/llvm-symbolizer.cpp
209 ↗(On Diff #182283)

Did you mean to remove the comment you had here before?

Quolyk updated this revision to Diff 182497.Jan 18 2019, 4:39 AM

Update tests.

Quolyk marked 4 inline comments as done.Jan 18 2019, 4:42 AM
Quolyk added inline comments.
test/DebugInfo/llvm-symbolizer.test
221 ↗(On Diff #182283)

Thanks for detailed explanation, now it becomes clear to me.

tools/llvm-symbolizer/llvm-symbolizer.cpp
209 ↗(On Diff #182283)

It's back. Wasn't sure it's necessary.

Quolyk marked 2 inline comments as done.Jan 18 2019, 4:42 AM
jhenderson added inline comments.Jan 18 2019, 4:49 AM
tools/llvm-symbolizer/llvm-symbolizer.cpp
210 ↗(On Diff #182497)

Nit: remove the "as demangle option".

Also, use clang-format to choose when to split the column onto multiple lines, rather than doing it by hand. I think most (maybe all) of this line will fit on the same line as the first line.

212 ↗(On Diff #182497)

What does getPosition do if the option is not on the command-line?

214–216 ↗(On Diff #182497)

Why has this changed?

Quolyk updated this revision to Diff 182747.Jan 20 2019, 11:27 PM

Used clang-format -style='{ReflowComments: true}'.

Quolyk marked 3 inline comments as done.Jan 20 2019, 11:31 PM
Quolyk added inline comments.
tools/llvm-symbolizer/llvm-symbolizer.cpp
212 ↗(On Diff #182497)

If option is not specified, cl::opt::getPosition() == 0.

This revision is now accepted and ready to land.Jan 21 2019, 1:42 AM
This revision was automatically updated to reflect the committed changes.

@jhenderson thanks a lot for review and remarks. That is very valuable.