[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

[Levon Ter-Grigoryan via Phabricator via lldb-commits]

PatriosTheGreat updated this revision to Diff 350596.
PatriosTheGreat added a comment.

Thanks for your suggestions.
I tried both of them, however the solution to patch assert recognizer doesn’t solve the original performance issue. Since the performance degradation doesn’t come from the recognizer. It appears since the LLDB needs to unwind at least one stack frame for each thread in order to determine does the LLDB even need to run recognizers (see this line: https://github.com/llvm/llvm-project/blob/main/lldb/source/Target/Thread.cpp#L587)

The solution to select the most relevant frame at the moment when the user actually needs a stack frame seems to fork fine. If I understand correctly it can be done at the Thread::GetStackFrameAtIndex method, since to provide stack trace view to the user all clients need to call it for all stack frames.

One problem I see in this solution is that recognizers should be aware to not call Thread::GetStackFrameAtIndex for the zeroth frame (which they actually got as a parameter), since otherwise it will bring to the infinity recursion. See changes to AssertFrameRecognizer.cpp

I’ve attached this solution to the current revision for further discussion.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103271/new/

https://reviews.llvm.org/D103271

Files:
  lldb/include/lldb/Target/Thread.h
  lldb/source/Target/AssertFrameRecognizer.cpp
  lldb/source/Target/Thread.cpp


Index: lldb/source/Target/Thread.cpp
===================================================================
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -578,14 +578,8 @@
   return raw_stop_description;
 }
 
-void Thread::SelectMostRelevantFrame() {
+void Thread::SelectMostRelevantFrame(lldb::StackFrameSP &frame_sp) {
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD);
-
-  auto frames_list_sp = GetStackFrameList();
-
-  // Only the top frame should be recognized.
-  auto frame_sp = frames_list_sp->GetFrameAtIndex(0);
-
   auto recognized_frame_sp = frame_sp->GetRecognizedFrame();
 
   if (!recognized_frame_sp) {
@@ -606,8 +600,6 @@
 void Thread::WillStop() {
   ThreadPlan *current_plan = GetCurrentPlan();
 
-  SelectMostRelevantFrame();
-
   // FIXME: I may decide to disallow threads with no plans.  In which
   // case this should go to an assert.
 
@@ -1405,6 +1397,15 @@
   exe_ctx.SetContext(shared_from_this());
 }
 
+lldb::StackFrameSP Thread::GetStackFrameAtIndex(uint32_t idx) {
+  auto frame = GetStackFrameList()->GetFrameAtIndex(idx);
+
+  if (frame && idx == 0)
+    SelectMostRelevantFrame(frame);
+
+  return frame;
+}
+
 StackFrameListSP Thread::GetStackFrameList() {
   std::lock_guard<std::recursive_mutex> guard(m_frame_mutex);
 
Index: lldb/source/Target/AssertFrameRecognizer.cpp
===================================================================
--- lldb/source/Target/AssertFrameRecognizer.cpp
+++ lldb/source/Target/AssertFrameRecognizer.cpp
@@ -118,8 +118,9 @@
 
   // Fetch most relevant frame
   for (uint32_t frame_index = 0; frame_index < frames_to_fetch; frame_index++) {
-    prev_frame_sp = thread_sp->GetStackFrameAtIndex(frame_index);
-
+    prev_frame_sp = frame_index == 0
+                        ? frame_sp
+                        : thread_sp->GetStackFrameAtIndex(frame_index);
     if (!prev_frame_sp) {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
       LLDB_LOG(log, "Abort Recognizer: Hit unwinding bound ({1} frames)!",
Index: lldb/include/lldb/Target/Thread.h
===================================================================
--- lldb/include/lldb/Target/Thread.h
+++ lldb/include/lldb/Target/Thread.h
@@ -218,7 +218,7 @@
 
   virtual void RefreshStateAfterStop() = 0;
 
-  void SelectMostRelevantFrame();
+  void SelectMostRelevantFrame(lldb::StackFrameSP &frame_sp);
 
   std::string GetStopDescription();
 
@@ -396,9 +396,7 @@
     return GetStackFrameList()->GetNumFrames();
   }
 
-  virtual lldb::StackFrameSP GetStackFrameAtIndex(uint32_t idx) {
-    return GetStackFrameList()->GetFrameAtIndex(idx);
-  }
+  virtual lldb::StackFrameSP GetStackFrameAtIndex(uint32_t idx);
 
   virtual lldb::StackFrameSP
   GetFrameWithConcreteFrameIndex(uint32_t unwind_idx);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D103271.350596.patch
Type: text/x-patch
Size: 2805 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20210608/8bebc24d/attachment-0001.bin>