Page MenuHomePhabricator

[llvm-symbolizer] Add -C as a short alias to -demangle
ClosedPublic

Authored by Quolyk on Fri, Jan 11, 4:41 AM.

Diff Detail

Event Timeline

Quolyk created this revision.Fri, Jan 11, 4:41 AM

I cannot have DIA SDK on my computer. As I see @jhenderson you have a Windows computer. Could you please write if tests are correct and in the right place. If there is a way to test it without DIA SDK any advice would be helpful.

Could you add an ELF test for this too, please, to show the route through the Itanium demangler (and when it doesn't go through it for -C=false)?

Unfortunately, it looks like I don't have a pre-requisite either, so the pdb test is unsupported for me too. The test change looks safe, but adding the ELF test as well would give the extra reassurance.

Quolyk updated this revision to Diff 181509.Mon, Jan 14, 1:59 AM

Add test

Could you add an ELF test for this too, please, to show the route through the Itanium demangler (and when it doesn't go through it for -C=false)?

Unfortunately, it looks like I don't have a pre-requisite either, so the pdb test is unsupported for me too. The test change looks safe, but adding the ELF test as well would give the extra reassurance.

How do i call itanium demangler? AFAIK llvm-symbolizer only accepts bool value of -demangle flag.

The Itanium demangler is used typically for ELF C++ symbols. If you take a look at test/tools/llvm-objdump/X86/demangle.s, you can see how the (de-)mangling happens. test/DebugInfo/llvm-symbolizer.test also uses the --demangle switch, and you could extend it to add -C alongside the demangle=false case or similar, if you want.

ruiu added inline comments.Mon, Jan 14, 11:19 AM
test/tools/llvm-symbolizer/pdb/pdb.test
8

This is not a new test because we have a test that exercises -demangle=false, but what is the "false" format style? addr2line reports an error if an argument given to --demangle or -C is not known.

jhenderson added inline comments.Tue, Jan 15, 2:15 AM
test/DebugInfo/llvm-symbolizer.test
218 ↗(On Diff #181509)

You don't need a new check-prefix. You can reuse the existing one. You also don't need to recreate %t.input7.

test/tools/llvm-symbolizer/pdb/pdb.test
8

Interesting catch. That wasn't something I noticed. llvm-symbolizer treats --demangle (and therefore -C) as a boolean switch, rather than one that takes a style, i.e. --demangle=false disables demangling.

It's worth noting that unlike addr2line, demangling is on by default too, so this switch only needs to exist for a) compatibility, and b) so that demangling can be disabled, unless we change the default (which could break people's workflows...).

Quolyk updated this revision to Diff 181769.Tue, Jan 15, 3:49 AM

Update tests

jhenderson accepted this revision.Tue, Jan 15, 4:20 AM

LGTM. I'm happy with this, but I'd like @ruiu to give a final thumbs up with relation to his comment.

This revision is now accepted and ready to land.Tue, Jan 15, 4:20 AM
ruiu accepted this revision.Tue, Jan 15, 10:41 AM

LGTM

This revision was automatically updated to reflect the committed changes.