Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb] Add frame recognizers for libc++ std::invoke #105695

Merged
merged 7 commits into from
Aug 27, 2024

Conversation

vogelsgesang
Copy link
Member

@vogelsgesang vogelsgesang commented Aug 22, 2024

With this commit, we also hide the implementation details of std::invoke. To do so, the LibCXXFrameRecognizer got a couple more regular expressions.

The regular expression passed into AddRecognizer became problematic, as it was evaluated on the demangled name. Those names also included result types for C++ symbols. For std::__invoke the return type is a huge decltype(...), making the regular expresison really hard to write.

Instead, I added support to AddRecognizer for matching on the demangled names without result type and argument types.

By hiding the implementation details of invoke, also the back traces for std::function become even nicer, because std::function is using __invoke internally.

Co-authored-by: Adrian Prantl [email protected]

@vogelsgesang vogelsgesang requested review from adrian-prantl and removed request for JDevlieghere August 22, 2024 16:58
@llvmbot llvmbot added the lldb label Aug 22, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 22, 2024

@llvm/pr-subscribers-lldb

Author: Adrian Vogelsgesang (vogelsgesang)

Changes

With this commit, we also hide the implementation details of std::invoke. To do so, the LibCXXFrameRecognizer got a couple more regular expressions.

The regular expression passed into the AddRecognizer became problematic, as it was evaluated on the demangled name. Those names also included result types for C++ symbols. For std::__invoke the return type is a huge decltype(...), making the regular expresison really hard to write.

Instead, I added support for AddRecognizer to match on the demangled names without result type and argument types.

By hiding the implementation details of invoke, also the back traces for std::function become even nicer, because std::function is using __invoke internally.


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

8 Files Affected:

  • (modified) lldb/include/lldb/Target/StackFrameRecognizer.h (+7-2)
  • (modified) lldb/source/Commands/Options.td (+1-1)
  • (modified) lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp (+36-12)
  • (modified) lldb/source/Target/StackFrameRecognizer.cpp (+37-4)
  • (modified) lldb/test/API/lang/cpp/std-function-recognizer/TestStdFunctionRecognizer.py (+19-2)
  • (added) lldb/test/API/lang/cpp/std-invoke-recognizer/Makefile (+5)
  • (added) lldb/test/API/lang/cpp/std-invoke-recognizer/TestStdInvokeRecognizer.py (+28)
  • (added) lldb/test/API/lang/cpp/std-invoke-recognizer/main.cpp (+40)
diff --git a/lldb/include/lldb/Target/StackFrameRecognizer.h b/lldb/include/lldb/Target/StackFrameRecognizer.h
index 8acebc12c4b1dc..585f0b0bb59009 100644
--- a/lldb/include/lldb/Target/StackFrameRecognizer.h
+++ b/lldb/include/lldb/Target/StackFrameRecognizer.h
@@ -107,12 +107,14 @@ class StackFrameRecognizerManager {
 public:
   void AddRecognizer(lldb::StackFrameRecognizerSP recognizer,
                      ConstString module, llvm::ArrayRef<ConstString> symbols,
-                     bool first_instruction_only = true);
+                     bool first_instruction_only = true,
+                     Mangled::NamePreference mangling_preference = Mangled::ePreferDemangled);
 
   void AddRecognizer(lldb::StackFrameRecognizerSP recognizer,
                      lldb::RegularExpressionSP module,
                      lldb::RegularExpressionSP symbol,
-                     bool first_instruction_only = true);
+                     bool first_instruction_only = true,
+                     Mangled::NamePreference mangling_preference = Mangled::ePreferDemangled);
 
   void ForEach(std::function<
                void(uint32_t recognizer_id, std::string recognizer_name,
@@ -143,10 +145,13 @@ class StackFrameRecognizerManager {
     std::vector<ConstString> symbols;
     lldb::RegularExpressionSP symbol_regexp;
     bool first_instruction_only;
+    Mangled::NamePreference mangling_preference;
   };
 
   std::deque<RegisteredEntry> m_recognizers;
   uint16_t m_generation;
+  std::unordered_set<Mangled::NamePreference> m_used_manglings;
+
 };
 
 /// \class ValueObjectRecognizerSynthesizedValue
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index 9c4dbed6939ba9..df906e9d7c808f 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -1049,7 +1049,7 @@ let Command = "thread backtrace" in {
   def thread_backtrace_extended : Option<"extended", "e">, Group<1>,
   Arg<"Boolean">, Desc<"Show the extended backtrace, if available">;
   def thread_backtrace_unfiltered : Option<"unfiltered", "u">, Group<1>,
-  Desc<"Filter out frames according to installed frame recognizers">;
+  Desc<"Do not filter out frames according to installed frame recognizers">;
 }
 
 let Command = "thread step scope" in {
diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
index c60200ab186d09..3665e1a4c77e55 100644
--- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include <cstring>
+#include <iostream>
 
 #include <memory>
 
@@ -44,7 +45,7 @@ char CPPLanguageRuntime::ID = 0;
 /// A frame recognizer that is installed to hide libc++ implementation
 /// details from the backtrace.
 class LibCXXFrameRecognizer : public StackFrameRecognizer {
-  RegularExpression m_hidden_function_regex;
+  std::array<RegularExpression, 3> m_hidden_regex;
   RecognizedStackFrameSP m_hidden_frame;
 
   struct LibCXXHiddenFrame : public RecognizedStackFrame {
@@ -53,10 +54,32 @@ class LibCXXFrameRecognizer : public StackFrameRecognizer {
 
 public:
   LibCXXFrameRecognizer()
-      : m_hidden_function_regex(
-            R"(^std::__1::(__function.*::operator\(\)|__invoke))"
-            R"((\[.*\])?)"    // ABI tag.
-            R"(( const)?$)"), // const.
+      : m_hidden_regex{
+            // internal implementation details of std::function
+            //    std::__1::__function::__alloc_func<void (*)(), std::__1::allocator<void (*)()>, void ()>::operator()[abi:ne200000]
+            //    std::__1::__function::__func<void (*)(), std::__1::allocator<void (*)()>, void ()>::operator()
+            //    std::__1::__function::__value_func<void ()>::operator()[abi:ne200000]() const
+            RegularExpression{""
+              R"(^std::__[0-9]*::)" // Namespace.
+              R"(__function::.*::operator\(\))"
+              R"((\[.*\])?)"    // ABI tag.
+              R"(( const)?$)"}, // const.
+            // internal implementation details of std::invoke
+            //   std::__1::__invoke[abi:ne200000]<void (*&)()>
+            RegularExpression{
+              R"(^std::__[0-9]*::)" // Namespace.
+              R"(__invoke)"
+              R"((\[.*\])?)"  // ABI tag.
+              R"(<.*>)"},     // template argument.
+            // internal implementation details of std::invoke
+            //   std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:ne200000]<void (*&)()>
+            RegularExpression{
+              R"(^std::__[0-9]*::)" // Namespace.
+              R"(__invoke_void_return_wrapper<.*>::__call)"
+              R"((\[.*\])?)"  // ABI tag.
+              R"(<.*>)"}      // template argument.
+
+        },
         m_hidden_frame(new LibCXXHiddenFrame()) {}
 
   std::string GetName() override { return "libc++ frame recognizer"; }
@@ -69,8 +92,9 @@ class LibCXXFrameRecognizer : public StackFrameRecognizer {
     if (!sc.function)
       return {};
 
-    if (m_hidden_function_regex.Execute(sc.function->GetNameNoArguments()))
-      return m_hidden_frame;
+    for (RegularExpression &r : m_hidden_regex)
+      if (r.Execute(sc.function->GetNameNoArguments()))
+        return m_hidden_frame;
 
     return {};
   }
@@ -81,8 +105,9 @@ CPPLanguageRuntime::CPPLanguageRuntime(Process *process)
   if (process)
     process->GetTarget().GetFrameRecognizerManager().AddRecognizer(
         StackFrameRecognizerSP(new LibCXXFrameRecognizer()), {},
-        std::make_shared<RegularExpression>("^std::__1::"),
-        /*first_instruction_only*/ false);
+        std::make_shared<RegularExpression>("std::__[0-9]*::"),
+        /*first_instruction_only=*/ false,
+        /*mangling_preference=*/ Mangled::ePreferDemangledWithoutArguments);
 }
 
 bool CPPLanguageRuntime::IsAllowedRuntimeValue(ConstString name) {
@@ -108,8 +133,7 @@ bool contains_lambda_identifier(llvm::StringRef &str_ref) {
 
 CPPLanguageRuntime::LibCppStdFunctionCallableInfo
 line_entry_helper(Target &target, const SymbolContext &sc, Symbol *symbol,
-                  llvm::StringRef first_template_param_sref,
-                  bool has_invoke) {
+                  llvm::StringRef first_template_param_sref, bool has_invoke) {
 
   CPPLanguageRuntime::LibCppStdFunctionCallableInfo optional_info;
 
@@ -190,7 +214,7 @@ CPPLanguageRuntime::FindLibCppStdFunctionCallableInfo(
     ValueObjectSP sub_member_f_(member_f_->GetChildMemberWithName("__f_"));
 
     if (sub_member_f_)
-        member_f_ = sub_member_f_;
+      member_f_ = sub_member_f_;
   }
 
   if (!member_f_)
diff --git a/lldb/source/Target/StackFrameRecognizer.cpp b/lldb/source/Target/StackFrameRecognizer.cpp
index 44411afc65dda9..37901a2ea526a8 100644
--- a/lldb/source/Target/StackFrameRecognizer.cpp
+++ b/lldb/source/Target/StackFrameRecognizer.cpp
@@ -62,19 +62,24 @@ void StackFrameRecognizerManager::BumpGeneration() {
 
 void StackFrameRecognizerManager::AddRecognizer(
     StackFrameRecognizerSP recognizer, ConstString module,
-    llvm::ArrayRef<ConstString> symbols, bool first_instruction_only) {
+    llvm::ArrayRef<ConstString> symbols, bool first_instruction_only,
+    Mangled::NamePreference mangling_preference) {
   m_recognizers.push_front({(uint32_t)m_recognizers.size(), recognizer, false,
                             module, RegularExpressionSP(), symbols,
                             RegularExpressionSP(), first_instruction_only});
+  m_used_manglings.insert(mangling_preference);
   BumpGeneration();
 }
 
 void StackFrameRecognizerManager::AddRecognizer(
     StackFrameRecognizerSP recognizer, RegularExpressionSP module,
-    RegularExpressionSP symbol, bool first_instruction_only) {
+    RegularExpressionSP symbol, bool first_instruction_only,
+    Mangled::NamePreference mangling_preference) {
   m_recognizers.push_front({(uint32_t)m_recognizers.size(), recognizer, true,
                             ConstString(), module, std::vector<ConstString>(),
-                            symbol, first_instruction_only});
+                            symbol, first_instruction_only,
+                            mangling_preference});
+  m_used_manglings.insert(mangling_preference);
   BumpGeneration();
 }
 
@@ -119,13 +124,30 @@ bool StackFrameRecognizerManager::RemoveRecognizerWithID(
 void StackFrameRecognizerManager::RemoveAllRecognizers() {
   BumpGeneration();
   m_recognizers.clear();
+  m_used_manglings.clear();
 }
 
 StackFrameRecognizerSP
 StackFrameRecognizerManager::GetRecognizerForFrame(StackFrameSP frame) {
   const SymbolContext &symctx = frame->GetSymbolContext(
       eSymbolContextModule | eSymbolContextFunction | eSymbolContextSymbol);
-  ConstString function_name = symctx.GetFunctionName();
+  ConstString function_name_mangled;
+  ConstString function_name_demangled;
+  ConstString function_name_noargs;
+  for (Mangled::NamePreference m : m_used_manglings) {
+    switch (m) {
+    case Mangled::ePreferMangled:
+      function_name_mangled = symctx.GetFunctionName(m);
+      break;
+    case Mangled::ePreferDemangled:
+      function_name_demangled = symctx.GetFunctionName(m);
+      break;
+    case Mangled::ePreferDemangledWithoutArguments:
+      function_name_noargs = symctx.GetFunctionName(m);
+      break;
+    }
+  }
+
   ModuleSP module_sp = symctx.module_sp;
   if (!module_sp)
     return StackFrameRecognizerSP();
@@ -145,6 +167,17 @@ StackFrameRecognizerManager::GetRecognizerForFrame(StackFrameSP frame) {
       if (!entry.module_regexp->Execute(module_name.GetStringRef()))
         continue;
 
+    ConstString function_name = [&]() {
+      switch (entry.mangling_preference) {
+      case Mangled::ePreferMangled:
+        return function_name_mangled;
+      case Mangled::ePreferDemangled:
+        return function_name_demangled;
+      case Mangled::ePreferDemangledWithoutArguments:
+        return function_name_noargs;
+      }
+    }();
+
     if (!entry.symbols.empty())
       if (!llvm::is_contained(entry.symbols, function_name))
         continue;
diff --git a/lldb/test/API/lang/cpp/std-function-recognizer/TestStdFunctionRecognizer.py b/lldb/test/API/lang/cpp/std-function-recognizer/TestStdFunctionRecognizer.py
index 30fe3ecb1e4bf4..28004194dee07c 100644
--- a/lldb/test/API/lang/cpp/std-function-recognizer/TestStdFunctionRecognizer.py
+++ b/lldb/test/API/lang/cpp/std-function-recognizer/TestStdFunctionRecognizer.py
@@ -7,6 +7,23 @@
 class LibCxxStdFunctionRecognizerTestCase(TestBase):
     NO_DEBUG_INFO_TESTCASE = True
 
+    @add_test_categories(["libc++"])
+    def test_frame_recognizer(self):
+        """Test that std::function all implementation details are hidden in SBFrame"""
+        self.build()
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+            self, "// break here", lldb.SBFileSpec("main.cpp")
+        )
+        self.assertIn("foo", thread.GetFrameAtIndex(0).GetFunctionName())
+        # Skip all hidden frames
+        frame_id = 1
+        while frame_id < thread.GetNumFrames() and thread.GetFrameAtIndex(frame_id).IsHidden():
+            frame_id = frame_id + 1
+        # Expect `std::function<...>::operator()` to be the direct parent of `foo`
+        self.assertIn("::operator()", thread.GetFrameAtIndex(frame_id).GetFunctionName())
+        # And right above that, there should be the `main` frame
+        self.assertIn("main", thread.GetFrameAtIndex(frame_id + 1).GetFunctionName())
+
     @add_test_categories(["libc++"])
     def test_backtrace(self):
         """Test that std::function implementation details are hidden in bt"""
@@ -27,12 +44,12 @@ def test_backtrace(self):
         self.expect(
             "thread backtrace -u",
             ordered=True,
-            patterns=["frame.*foo", "frame.*std::__1::__function", "frame.*main"],
+            patterns=["frame.*foo", "frame.*std::__[0-9]*::__function", "frame.*main"],
         )
         self.expect(
             "thread backtrace --unfiltered",
             ordered=True,
-            patterns=["frame.*foo", "frame.*std::__1::__function", "frame.*main"],
+            patterns=["frame.*foo", "frame.*std::__[0-9]*::__function", "frame.*main"],
         )
 
     @add_test_categories(["libc++"])
diff --git a/lldb/test/API/lang/cpp/std-invoke-recognizer/Makefile b/lldb/test/API/lang/cpp/std-invoke-recognizer/Makefile
new file mode 100644
index 00000000000000..69014eb9c0f2eb
--- /dev/null
+++ b/lldb/test/API/lang/cpp/std-invoke-recognizer/Makefile
@@ -0,0 +1,5 @@
+CXX_SOURCES := main.cpp
+USE_LIBCPP := 1
+CXXFLAGS_EXTRAS := -std=c++17
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/std-invoke-recognizer/TestStdInvokeRecognizer.py b/lldb/test/API/lang/cpp/std-invoke-recognizer/TestStdInvokeRecognizer.py
new file mode 100644
index 00000000000000..f2f8c506630eac
--- /dev/null
+++ b/lldb/test/API/lang/cpp/std-invoke-recognizer/TestStdInvokeRecognizer.py
@@ -0,0 +1,28 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class LibCxxStdFunctionRecognizerTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    @add_test_categories(["libc++"])
+    def test_frame_recognizer(self):
+        """Test that implementation details details of `std::invoke`"""
+        self.build()
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+            self, "// break here", lldb.SBFileSpec("main.cpp")
+        )
+
+        while process.GetState() != lldb.eStateExited:
+            self.assertIn("print_num", thread.GetFrameAtIndex(0).GetFunctionName())
+            self.process.Continue()
+        # # Skip all hidden frames
+        # frame_id = 1
+        # while frame_id < thread.GetNumFrames() and thread.GetFrameAtIndex(frame_id).IsHidden():
+        #     frame_id = frame_id + 1
+        # # Expect `std::function<...>::operator()` to be the direct parent of `foo`
+        # self.assertIn("::operator()", thread.GetFrameAtIndex(frame_id).GetFunctionName())
+        # # And right above that, there should be the `main` frame
+        # self.assertIn("main", thread.GetFrameAtIndex(frame_id + 1).GetFunctionName())
diff --git a/lldb/test/API/lang/cpp/std-invoke-recognizer/main.cpp b/lldb/test/API/lang/cpp/std-invoke-recognizer/main.cpp
new file mode 100644
index 00000000000000..78497d2938fe8a
--- /dev/null
+++ b/lldb/test/API/lang/cpp/std-invoke-recognizer/main.cpp
@@ -0,0 +1,40 @@
+#include <functional>
+#include <iostream>
+
+void print_num(int i) {
+  // break here
+  std::cout << i << '\n';
+}
+
+int add(int i, int j) {
+  // break here
+  return i + j;
+}
+
+struct PrintAdder {
+  PrintAdder(int num) : num_(num) {}
+  void operator()(int i) const {
+    // break here
+    std::cout << i << '\n';
+  }
+  void print_add(int i) const {
+    // break here
+    std::cout << num_ + i << '\n';
+  }
+  int num_;
+};
+
+int main() {
+  // Invoke a void-returning function
+  std::invoke(print_num, -9);
+
+  // Invoke a non-void-returning function
+  std::cout << std::invoke(add, 1, 10) << '\n';
+
+  // Invoke a member function
+  const PrintAdder foo(314159);
+  std::invoke(&PrintAdder::print_add, foo, 1);
+
+  // Invoke a function object
+  std::invoke(PrintAdder(12), 18);
+}

@vogelsgesang vogelsgesang changed the title [lldb-dap] Add frame recognizers for libc++ std::invoke [lldb] Add frame recognizers for libc++ std::invoke Aug 22, 2024
@vogelsgesang
Copy link
Member Author

vogelsgesang commented Aug 22, 2024

@mordante @ldionne FYI. I would be interested which other functions come to mind that should be hidden.

See #105457 (comment) for screenshots of how this looks from a user's perspective

// std::__1::__invoke[abi:ne200000]<void (*&)()>
RegularExpression{
R"(^std::__[0-9]*::)" // Namespace.
R"(__invoke)"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should simply hide everything starting with ^std::__[0-9]*::__.*. Or are there any __ functions in libc++ which are not implementation details and should be shown in stacktraces by default?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Don't think we want to maintain this list.

(also, side-note, I think matching on const and abi_tag is probably redundant. Not a concern if we just filter out all the std::__*::__ stuff).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to defer switching to ^std::__[0-9]*::__.* in a follow-up commit, if that's fine by you.

I am pretty sure that this commit here works without unintended side effects, but I am not sure if ^std::__[0-9]*::__.* would regress debugability in unforeseen ways

Copy link
Member

@petrhosek petrhosek Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libc++ ABI namespace doesn't have to be just a number, it can be any string that starts with __. For example, in some of our Google projects we use __ktl and __pw; libFuzzer uses __Fuzzer in its internal libc++ build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. I posted this comment before I saw your fix on main. By now, the commit contains `^std::__[^:]::

@@ -1049,7 +1049,7 @@ let Command = "thread backtrace" in {
def thread_backtrace_extended : Option<"extended", "e">, Group<1>,
Arg<"Boolean">, Desc<"Show the extended backtrace, if available">;
def thread_backtrace_unfiltered : Option<"unfiltered", "u">, Group<1>,
Desc<"Filter out frames according to installed frame recognizers">;
Desc<"Do not filter out frames according to installed frame recognizers">;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just push this as an NFC separately

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have direct push-access and piggy-backing it in this PR is less effort for me

case Mangled::ePreferDemangledWithoutArguments:
return function_name_noargs;
}
}();
Copy link
Member

@Michael137 Michael137 Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just pass symctx.GetFunctionName(entry.mangling_preference) to the below is_contained right?

No need for the switch statement here and above and also no need to keep a map of m_used_manglings

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about performance here. The code previously computed the demanded name only once and then checked against all frame recognizers. I am not sure how expensive it is to demangle a name

Copy link
Member

@Michael137 Michael137 Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The demangling gets chached anyway (in the Mangled class). So wouldn't worry about it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way: I don't think I will be merging this part of my commit in the end. #105756 is very similar, and I think I will just rebase on it, after it got merged

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vogelsgesang I closed my PR, feel free to copy&paste what you find useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The demangling gets chached anyway (in the Mangled class). So wouldn't worry about it

ah, great! I didn't know this. I removed the additional caching from StackFrameRecognizer again

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vogelsgesang I closed my PR, feel free to copy&paste what you find useful.

I did copy over:

  • the improved comments in StackFrameRecognizer.h
  • the printing improvements to frame recognizer list (but I am using a slightly different format)

However, I did not change the VerboseTrapFrameRecognizer and AssertFrameRecognizer to use the mangled names. I will leave this for a follow-up commit.

Copy link

github-actions bot commented Aug 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link

github-actions bot commented Aug 25, 2024

✅ With the latest revision this PR passed the Python code formatter.

@vogelsgesang vogelsgesang force-pushed the lldb-frame-recognizer-invoke branch 5 times, most recently from e21f87c to cf62995 Compare August 26, 2024 04:28
With this commit, we also hide the implementation details of
`std::invoke`. To do so, the `LibCXXFrameRecognizer` got a couple more
regular expressions.

The regular expression passed into the `AddRecognizer` became
problematic, as it was evaluated on the demangled name. Those names also
included result types for C++ symbols. For `std::__invoke` the return
type is a huge `decltype(...)`, making the regular expresison really
hard to write.

Instead, I added support for `AddRecognizer` to match on the demangled
names without result type and argument types.

By hiding the implementation details of `invoke`, also the back traces
for `std::function` become even nicer, because `std::function` is using
`__invoke` internally.
They are already cached in `Mangled`
Copy link
Member

@Michael137 Michael137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (with some minor comments). I'd wait for @adrian-prantl to also review this

lldb/test/API/lang/cpp/std-invoke-recognizer/main.cpp Outdated Show resolved Hide resolved
substrs=["recognizer.MyFrameRecognizer, module a.out, symbol bar"],
substrs=[
"recognizer.MyFrameRecognizer, module a.out, demangled symbol bar"
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't seem to have tests for this with ePreferDemangledWithoutArguments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I was not able to write such a test case. Note that the frame recognizer add command does not yet expose a way to match on anything else than demangled names. I agree that we should be adding test cases here, as soon as frame recognizer add supports choosing the symbol mangling.

@adrian-prantl Are you planning to extend frame recognizer add accordingly?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I was not able to write such a test case. Note that the frame recognizer add command does not yet expose a way to match on anything else than demangled names. I agree that we should be adding test cases here, as soon as frame recognizer add supports choosing the symbol mangling.

@adrian-prantl Are you planning to extend frame recognizer add accordingly?

It's not on my immediate todo list, but it would be an obvious next step.

Copy link
Collaborator

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@petrhosek
Copy link
Member

@vogelsgesang Can we please merge this PR to unbreak the builders that were broken by #104523?

@vogelsgesang vogelsgesang merged commit dd060bd into llvm:main Aug 27, 2024
5 of 6 checks passed
@vogelsgesang vogelsgesang deleted the lldb-frame-recognizer-invoke branch August 27, 2024 17:18
@adrian-prantl
Copy link
Collaborator

Looks like this is failing on the Darwin bot: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/10463/

@vogelsgesang
Copy link
Member Author

fixed in #106281

5c4lar pushed a commit to 5c4lar/llvm-project that referenced this pull request Aug 29, 2024
With this commit, we also hide the implementation details of
`std::invoke`. To do so, the `LibCXXFrameRecognizer` got a couple more
regular expressions.

The regular expression passed into `AddRecognizer` became problematic,
as it was evaluated on the demangled name. Those names also included
result types for C++ symbols. For `std::__invoke` the return type is a
huge `decltype(...)`, making the regular expresison really hard to
write.

Instead, I added support to `AddRecognizer` for matching on the
demangled names without result type and argument types.

By hiding the implementation details of `invoke`, also the back traces
for `std::function` become even nicer, because `std::function` is using
`__invoke` internally.

Co-authored-by: Adrian Prantl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants