Page MenuHomePhabricator

Recognize MSVC style mangling in CPlusPlusLanguage::IsCPPMangledName

Authored by asmith on Feb 7 2018, 6:54 PM.

Diff Detail


Event Timeline

asmith created this revision.Feb 7 2018, 6:54 PM
asmith retitled this revision from Add implementation for MSCV in CPlusPlusLanguage::IsCPPMangledName to Add implementation for MSVC in CPlusPlusLanguage::IsCPPMangledName.Feb 7 2018, 9:42 PM
davide added a subscriber: davide.Feb 7 2018, 10:50 PM

Can you add a unittest for this? :)

labath requested changes to this revision.Feb 8 2018, 1:47 AM
labath added a subscriber: labath.

That seems extremely dodgy. Surely whether something is a mangled name does not depend on the compiler we built lldb with. I am aware that we have many places with _Z hardcoded, but this seems to be making the situation only worse.

I'm not sure how is this API being used, but maybe this should just recognize both mangling schemes unconditionally ?

This revision now requires changes to proceed.Feb 8 2018, 1:47 AM

IsCPPMangledName() is a public method to determine if a symbol has a mangled name.

This change is needed so that lldb can find symbols with MSVC style mangled names.

Open to suggestions on a better way to deal with this...

Here are all the places it is used:

source/Core/Module.cpp: if (CPlusPlusLanguage::IsCPPMangledName(name_cstr))
source/Core/Module.cpp: !CPlusPlusLanguage::IsCPPMangledName(name_cstr)) {
source/Core/Mangled.cpp: if (CPlusPlusLanguage::IsCPPMangledName(mangled_name))
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp: !CPlusPlusLanguage::IsCPPMangledName(name)) ||
source/Expression/IRExecutionUnit.cpp: if (CPlusPlusLanguage::IsCPPMangledName(name.GetCString())) {
source/Expression/IRExecutionUnit.cpp: if (CPlusPlusLanguage::IsCPPMangledName(name.GetCString())) {

asmith added a comment.EditedFeb 8 2018, 12:30 PM

How about this?

if (name == nullptr)
  return false;
return ( (name[0] == '?') || (name[0] == '_' && name[1] == 'Z') );
jingham added a subscriber: jingham.Feb 8 2018, 1:54 PM

We'd get into problems on a platform other than MSVC that uses ^? for symbol names that aren't C++ mangled ones - or equivalently if MSVC uses ^_Z for non-C++ mangled names.

It looks like everywhere we use this (or use GuessLanguage) we either turn around and mangle it, and we handle the case where we weren't actually right about that. If we had another language with a different mangling (swift for example) and on some platform that language uses ? for its initial prefix, then we might get into trouble.

We don't everywhere do IsCPPMangledName -> Try to demangle -> Try next possible language. So we would miss demangling some names.

The "correct" way to do this is to change all the places that use this API actually get the current C++ runtime and ask that if they recognize the name. But right now that's hard because we infer the C++ runtime from the Process (after all you could have both the FSF & LLVM libstdc++'s on the same platform, and you might not know which you will get when you run. Even worse, it used to be (on macOS in the early days of lldb) that you could have two different C++ standard libraries with different manglings. So you can't make a great guess about what mangling is going to be used from just a Target. If we suspected this confusion were a likely problem we'd have to make a TargetLanguageRuntime that you would fall back to if you didn't have a Process, and have that implement IsCPPMangledName. That wouldn't help with two manglings on one platform, but it would keep the platforms properly segregated.

OTOH, this seems like a fairly remote possibility, so I'm okay with leaving this as a possible danger, though it might be good to make the comment clearer about that possibility, since the danger it warns about has clearly arisen...

asmith updated this revision to Diff 133489.Feb 8 2018, 2:33 PM
asmith retitled this revision from Add implementation for MSVC in CPlusPlusLanguage::IsCPPMangledName to Recognize MSVC style mangling in CPlusPlusLanguage::IsCPPMangledName.
zturner accepted this revision.Feb 8 2018, 2:46 PM

Looks good. Wish this function took a StringRef so you could just say return name.startswith("?") || name.startswith("_Z")

This revision was not accepted when it landed; it landed in state Needs Review.Feb 8 2018, 3:16 PM
This revision was automatically updated to reflect the committed changes.