[LLDB] Fix symbol breakpoint lookups for non-C-like languages

[walter-erquinigo]

I'm developing the Mojo language, and some symbol breakpoints don't work because of two reasons that this PR fixes:

• There's a prune step that operates on the assuption that the symbol is C-like. That was discarding lots of the Mojo breakpoints
• When finding functions for symbol breakpoints with eFunctionNameTypeFull, LLDB was only matching die.getMangledName() and not die.GetName(). The latter is the one useful for Mojo, because the former has some unreadable format in Mojo that is not exposed to the user. This shouldn't cause any regression for C-like languages, as the prune step would sanitize them anyway, but it's useful for languages like Mojo.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Walter Erquinigo (walter-erquinigo)

Changes

I'm developing the Mojo language, and some symbol breakpoints don't work because of two reasons that this PR fixes:

• There's a prune step that operates on the assuption that the symbol is C-like. That was discarding lots of the Mojo breakpoints
• When finding functions for symbol breakpoints with eFunctionNameTypeFull, LLDB was only matching die.getMangledName() and not die.GetName(). The latter is the one useful for Mojo, because the former has some unreadable format in Mojo that is not exposed to the user. This shouldn't cause any regression for C-like languages, as the prune step would sanitize them anyway, but it's useful for languages like Mojo.

---

Full diff: https://github.com/llvm/llvm-project/pull/110543.diff

3 Files Affected:

• (modified) lldb/source/Breakpoint/BreakpointResolverName.cpp (+7-7)
• (modified) lldb/source/Core/Module.cpp (+4)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp (+2-1)

diff --git a/lldb/source/Breakpoint/BreakpointResolverName.cpp b/lldb/source/Breakpoint/BreakpointResolverName.cpp
index 264638eb836dc6..77ddd86302cd1d 100644
--- a/lldb/source/Breakpoint/BreakpointResolverName.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolverName.cpp
@@ -24,10 +24,10 @@
 using namespace lldb;
 using namespace lldb_private;
 
-BreakpointResolverName::BreakpointResolverName(const BreakpointSP &bkpt,
-    const char *name_cstr, FunctionNameType name_type_mask,
-    LanguageType language, Breakpoint::MatchType type, lldb::addr_t offset,
-    bool skip_prologue)
+BreakpointResolverName::BreakpointResolverName(
+    const BreakpointSP &bkpt, const char *name_cstr,
+    FunctionNameType name_type_mask, LanguageType language,
+    Breakpoint::MatchType type, lldb::addr_t offset, bool skip_prologue)
     : BreakpointResolver(bkpt, BreakpointResolver::NameResolver, offset),
       m_match_type(type), m_language(language), m_skip_prologue(skip_prologue) {
   if (m_match_type == Breakpoint::Regexp) {
@@ -237,9 +237,9 @@ void BreakpointResolverName::AddNameLookup(ConstString name,
   if (Language *lang = Language::FindPlugin(m_language)) {
     add_variant_funcs(lang);
   } else {
-    // Most likely m_language is eLanguageTypeUnknown. We check each language for
-    // possible variants or more qualified names and create lookups for those as
-    // well.
+    // Most likely m_language is eLanguageTypeUnknown. We check each language
+    // for possible variants or more qualified names and create lookups for
+    // those as well.
     Language::ForEach(add_variant_funcs);
   }
 }
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 88cc957e91fac4..29b419e9969743 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -797,6 +797,10 @@ void Module::LookupInfo::Prune(SymbolContextList &sc_list,
     while (i < sc_list.GetSize()) {
       if (!sc_list.GetContextAtIndex(i, sc))
         break;
+      if (!lldb_private::Language::LanguageIsCFamily(sc.GetLanguage())) {
+        ++i;
+        continue;
+      }
       // Make sure the mangled and demangled names don't match before we try to
       // pull anything out
       ConstString mangled_name(sc.GetFunctionName(Mangled::ePreferMangled));
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
index eafddbad03f57b..7bf53dce1b1b07 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
@@ -59,7 +59,8 @@ bool DWARFIndex::ProcessFunctionDIE(
     return true;
 
   // In case of a full match, we just insert everything we find.
-  if (name_type_mask & eFunctionNameTypeFull && die.GetMangledName() == name)
+  if (name_type_mask & eFunctionNameTypeFull &&
+      (die.GetMangledName() == name || die.GetName() == name))
     return callback(die);
 
   // If looking for ObjC selectors, we need to also check if the name is a

[jimingham]

This sort of code in generic parts of lldb is formally wrong, you shouldn't pretend to know that only C does something, you should ask the language plugin for the current context to do the job for you, and if it's a no-op, it should be that in the language plugin.

Of course, the code below this which you are circumventing ALSO doesn't belong in Module.cpp since it explicitly mentions CPlusPlusLanguage.

OTOH, maybe this should get a pass because you didn't cause the problem. OTOH, given you're adding support for a new and enough different language that it trips up on these sorts of things, for your purposes, making sure these little tasks are properly factored out is of more importance, perhaps...

[walter-erquinigo]

What you say makes sense.
@jimingham , what about me moving the implementation of this second part of the Prune method (line 790 onwards) to Language plugins? I could then create the method
SymbolLookupPruneHook(SymbolContextList &sc_list, size_t start_idx, Module::LookupInfo &lookup_info, Module &module)

That would be very convenient because I could implement this in my own language plugin for further refinements.

What do you think?

[jimingham]

There are only reformattings in this file, unless I'm missing something. Good to leave this sort of change out of patches that actually do something.

[walter-erquinigo]

Good catch, I didn't realize the formatter changes this file.

[jimingham]

That sounds entirely appropriate. Jim

…

On Sep 30, 2024, at 1:49 PM, Walter Erquinigo ***@***.***> wrote: @walter-erquinigo commented on this pull request. In lldb/source/Core/Module.cpp <#110543 (comment)>: > @@ -797,6 +797,10 @@ void Module::LookupInfo::Prune(SymbolContextList &sc_list, while (i < sc_list.GetSize()) { if (!sc_list.GetContextAtIndex(i, sc)) break; + if (!lldb_private::Language::LanguageIsCFamily(sc.GetLanguage())) { What you say makes sense. @jimingham <https://github.com/jimingham> , what about me moving the implementation of this second part of the Prune method (line 790 onwards) to Language plugins? I could then create the method SymbolLookupPruneHook(SymbolContextList &sc_list, size_t start_idx, Module::LookupInfo &lookup_info, Module &module) That would be very convenient because I could implement this in my own language plugin for further refinements. What do you think? — Reply to this email directly, view it on GitHub <#110543 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4FUUNGIJVLEB23B6LZZG2O5AVCNFSM6AAAAABPD35ZI2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZYGUZTONRVGU>. You are receiving this because you were mentioned.

[jimingham]

I don't think "Hook" is right here. You can tell this is a language hook by the fact that we're calling some GetLanguagePlugin->ThisMethod so the "Hook" is redundant. We don't call any of the other language hooks hooks.

[jimingham]

This LGTM except for a quibble over the name.

I have a suspicion that Rust might have used this bit of functionality - it pretends that it's C++ for much of the support. It would be good to make sure that:

Language::FindPlugin(sc.GetLanguage())

for a Rust frame really does return the C++ Language Runtime so we don't change that behavior.

[walter-erquinigo]

I'm closing this because I figured out a way for my language to be compliant with what LLDB expects, following the same path of Rust. I know we should make LLDB more generic, but I ended up finding so many little broken things if I don't follow C++'s style, and that's too much just for me.