Page MenuHomePhabricator

[lldb] Add method Language::IsMangledName
Needs RevisionPublic

Authored by xiaobai on Feb 7 2020, 1:27 AM.

Details

Reviewers
kwk
Group Reviewers
Restricted Project
Summary

Instead of asking each of the language plugins directly if a symbol is a
possible manghled name for that language, we can generalize this
pattern. By adding a method IsMangledName and iterating over each
loaded language plugin, we can avoid referencing any language plugin
directly.

Event Timeline

xiaobai created this revision.Feb 7 2020, 1:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2020, 1:27 AM
xiaobai marked an inline comment as done.Feb 7 2020, 1:33 AM

I uploaded this patch primarily to get some feedback on possible alternatives because I'm not happy creating a dependency from Core to Target here. Suggestions welcome!

lldb/source/Core/Mangled.cpp
12

Not happy about introducing this dependency. Maybe I can remove the GuessLanguage method from Mangled and add one to Language... something like Language::GuessMangledSymbolLanguage(Mangled mangled)?

friss added a subscriber: friss.Feb 7 2020, 1:36 AM
friss added inline comments.
lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
96–98

The original code was calling IsPossibleObjCMethodName and it looks like you completely lose this codepath with this rewrite. Unclear to me if it's the right thing to return here, but that's definitely a change in behavior.

xiaobai added inline comments.Feb 7 2020, 7:00 AM
lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
96–98

If I understand correctly, Objective-C names aren't mangled in general, and are usually stored in the m_demangled name of Mangled, so that code path should have been bogus to begin with. Maybe I'm missing a detail though?

I can add a comment to this to clarify that Objective-C names aren't mangled (if I am correct in my understanding).

shafik added inline comments.
lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
96–98

I believe this is true, I have not verified it myself. I believe what happens is that m_mangled is just set directly in the Objective-C but it probably would be nice to verify this via a test in MangledTest.cpp I actually though @mib had added a test here for Objective-C the other day.

friss added inline comments.Feb 8 2020, 1:52 AM
lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
96–98

If I understand correctly, Objective-C names aren't mangled in general, and are usually stored in the m_demangled name of Mangled, so that code path should have been bogus to begin with. Maybe I'm missing a detail though?

I'm just pointing out a change in behavior. If a Mangled object had m_mangled set to an Obj-C method name, GuessLanguage() would have returned eLanguageTypeObjC before your patch. It won't do it after. I don't actually know whether we ever put a method name in m_mangled, but I bet we do.

Whether Obj-C symbols are actually mangled or not is debatable too. The name of the method symbol tells you whether it is a class or instance method and the number and names of arguments. Looks pretty close to the information the C++ mangling gives you for a symbol.

mib added inline comments.Feb 8 2020, 11:18 AM
lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
96–98

In D71237, I only added a test for the function.mangled-name frame-format entity with a C++ example.

kwk requested changes to this revision.Mar 25 2020, 8:06 AM
kwk added a subscriber: kwk.

I agree with @friss that there is a change in behavior. I'm not sure it is fixable with what I wrote but I think so.

lldb/include/lldb/Target/Language.h
191

In Mangled::GuessLanguage() you call this function with if (lang->IsMangledName(mangled.GetCString())). mangled in that case is a ConstString. I wonder why you don't pass that to IsMangledName? Is a StringRef faster? I'm asking because I see GetMethodNameVariants below and that also must be called with a ConstString.

lldb/source/Core/Mangled.cpp
416

In the ObjCLanguage's implementation of IsMangledName() I hope to see a call to sPossibleObjCMethodName. I think that is what @friss is aiming for with D74187#1865342.

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
97

Essentially, just return ObjCLanguage::IsPossibleObjCMethodName(name), no? It is defined only a few lines further down below.

@friss is that what you mean in order to restore the original behavior?

This revision now requires changes to proceed.Mar 25 2020, 8:06 AM