Page MenuHomePhabricator

[Demangle] Add a few more options to the microsoft demangler
ClosedPublic

Authored by mstorsjo on Oct 12 2019, 2:38 PM.

Details

Summary

This corresponds to commonly used options to UnDecorateSymbolName within llvm.

Add them as hidden options in llvm-undname. MS undname.exe takes numeric flags, corresponding to the UNDNAME_* constants, but instead of hardcoding in mappings for those numbers, just add textual options instead, as it the use of them here is primarily intended for testing.

This should allow replacing UnDecorateSymbolName from dbghelp with the llvm demangler mostly without changing the output.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 12 2019, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2019, 2:38 PM
rnk added inline comments.Oct 14 2019, 1:26 PM
llvm/include/llvm/Demangle/Demangle.h
39

Any reason not to say MSDF_NoCallingConvention instead? I see it's consistent with the UnDecorateSymbolName flag, but "allocation language" is pretty opaque. It's not like we're being compatible with undname here, we might as well use descriptive command line flag names.

40

"Function returns" could be "return types", but I don't feel as strongly about it.

llvm/lib/Demangle/MicrosoftDemangleNodes.cpp
602

Code golf suggestion:

StringRef AccessSpec;
bool IsStatic = true;
switch (SC) {
  case StorageClass::PrivateStatic:
    AccessSpec = "private";
    break;
  case StorageClass::ProtectedStatic:
    AccessSpec = "protected";
    break;
  case StorageClass::PublicStatic:
    AccessSpec = "public";
    break;
  default:
    IsStatic = false;
    break;
}
if (!(Flags & OF_NoAccessSpecifier) && !AccessSpec.empty())
  OS << AccessSpec << ": ";
if (!(Flags & OF_NoNoMemberType) && IsStatic)
  OS << "static";
llvm/tools/llvm-undname/llvm-undname.cpp
37

Ditto, we should consider a more descriptive name.

mstorsjo marked 3 inline comments as done.Oct 14 2019, 1:51 PM
mstorsjo added inline comments.
llvm/include/llvm/Demangle/Demangle.h
39

Sounds good, I'd change both this and the llvm-undname flag then.

40

If deviating from the undname naming, we can just as well make all of them more straightforward, so I'd change this one as well then.

llvm/lib/Demangle/MicrosoftDemangleNodes.cpp
602

Neat! StringRef isn't really allowed here, but using const char * isn't any more cumbersome here.

mstorsjo updated this revision to Diff 224900.Oct 14 2019, 1:54 PM

Applied Reid's suggestions

rnk accepted this revision.Oct 14 2019, 2:10 PM

lgtm

llvm/lib/Demangle/MicrosoftDemangleNodes.cpp
602

Right. Wow, I guess the demangler has it's whole own set of operator<< overloads. >_>

This revision is now accepted and ready to land.Oct 14 2019, 2:10 PM
This revision was automatically updated to reflect the committed changes.

This is cool, thanks!

Note that there are a 3 or so places where the demangler also hardcodes OF_Default. One patch that I have sitting around locally fixes one of them (https://gist.github.com/nico/8f219c64c9269c1f8c6657c46f3e8644 -- feel free to take that one over; I wrote it for something else that I couldn't yet prove is actually a problem; from the test script at the bottom you can probably figure out what it was :)). As-is, these options are ignored for these cases (cached types in templates, parent scopes of locally numbered scopes, and something else). It's possible that these contexts are unaffected by these options, but I figured I'd mention it, in case you want to check.

mstorsjo marked an inline comment as done.Oct 17 2019, 12:33 PM

This is cool, thanks!

Note that there are a 3 or so places where the demangler also hardcodes OF_Default. One patch that I have sitting around locally fixes one of them (https://gist.github.com/nico/8f219c64c9269c1f8c6657c46f3e8644 -- feel free to take that one over; I wrote it for something else that I couldn't yet prove is actually a problem; from the test script at the bottom you can probably figure out what it was :)). As-is, these options are ignored for these cases (cached types in templates, parent scopes of locally numbered scopes, and something else). It's possible that these contexts are unaffected by these options, but I figured I'd mention it, in case you want to check.

Oh, ok. TBH, unless there's a clear issue I'm running into, I don't think I'll be looking more into it right now. My original quest was to sift through lldb and enable as much as makes sense of #ifdef _MSC_VER blocks on mingw, and that's more or less comple now. :-) For the use of dbghelp for demangling I thought it'd be trivial to just convert to the internal demangler - instead of using more elaborate mechanisms than #pragma comment(lib) for linking to dbghelp. It turned out to be a bit bigger task than I had expected originally, but not too bad.