[Lldb-commits] [PATCH] D130524: Don't hold the StackFrame mutex while getting a ValueObject Dynamic value

Mon Jul 25 15:11:55 PDT 2022

jingham created this revision.
jingham added reviewers: JDevlieghere, clayborg, augusto2112.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

We've noticed occasional hangs in the test TestGuiExpandThreadsTree.py.  They are caused by a lock inversion between the StackFrameList and StackFrame mutexes.

One thread has the StackFrame mutex, and is trying to acquire the StackFrameList mutex for that thread:

- thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
  - frame #0: 0x000000018fffaafc libsystem_kernel.dylib`__psynch_mutexwait + 8 frame #1: 0x0000000190034358 libsystem_pthread.dylib`_pthread_mutex_firstfit_lock_wait + 84 frame #2: 0x0000000190031cbc libsystem_pthread.dylib`_pthread_mutex_firstfit_lock_slow + 248 frame #3: 0x000000018ff86b7c libc++.1.dylib`std::__1::recursive_mutex::lock() + 16 frame #4: 0x000000010dc85c0c liblldb.15.0.0git.dylib`lldb_private::StackFrameList::GetFrameWithStackID(lldb_private::StackID const&) + 72 frame #5: 0x000000010dcbad44 liblldb.15.0.0git.dylib`lldb_private::Thread::GetFrameWithStackID(lldb_private::StackID const&) + 64 frame #6: 0x000000010dc3cd94 liblldb.15.0.0git.dylib`lldb_private::ExecutionContextRef::GetFrameSP() const + 84 frame #7: 0x000000010dc3ca3c liblldb.15.0.0git.dylib`lldb_private::ExecutionContext::ExecutionContext(lldb_private::ExecutionContextRef const&) + 244 frame #8: 0x000000010db314a8 liblldb.15.0.0git.dylib`lldb_private::ValueObject::CalculateDynamicValue(lldb::DynamicValueType) + 84 frame #9: 0x000000010db31564 liblldb.15.0.0git.dylib`lldb_private::ValueObject::GetDynamicValue(lldb::DynamicValueType) + 108 frame #10: 0x000000010dc7f4ac liblldb.15.0.0git.dylib`lldb_private::StackFrame::GetValueObjectForFrameVariable(std::__1::shared_ptr<lldb_private::Variable> const&, lldb::DynamicValueType) + 204

    ^^^^^^  frame #10 is the frame that acquires the StackFrame mutex  ^^^^^^^^^^

    frame #11: 0x000000010daffdc0 liblldb.15.0.0git.dylib`FrameVariablesWindowDelegate::WindowDelegateDraw(curses::Window&, bool) + 216 frame #12: 0x000000010daeb208 liblldb.15.0.0git.dylib`curses::Window::Draw(bool) + 52 frame #13: 0x000000010daeb23c liblldb.15.0.0git.dylib`curses::Window::Draw(bool) + 104 frame #14: 0x000000010daea3f8 liblldb.15.0.0git.dylib`curses::Application::Run(lldb_private::Debugger&) + 172 frame #15: 0x000000010daea338 liblldb.15.0.0git.dylib`lldb_private::IOHandlerCursesGUI::Run() + 28 frame #16: 0x000000010dac8abc liblldb.15.0.0git.dylib`lldb_private::Debugger::RunIOHandlers() + 140 frame #17: 0x000000010dbbe210 liblldb.15.0.0git.dylib`lldb_private::CommandInterpreter::RunCommandInterpreter(lldb_private::CommandInterpreterRunOptions&) + 156 frame #18: 0x000000010d90f780 liblldb.15.0.0git.dylib`lldb::SBDebugger::RunCommandInterpreter(bool, bool) + 168 frame #19: 0x0000000104058938 lldb`Driver::MainLoop() + 2776 frame #20: 0x000000010405959c lldb`main + 2456 frame #21: 0x000000021b293c14 dyld`start + 2372

The other has the StackFrameList mutex and is trying to get the StackFrame mutex held above.

  thread #2, name = 'lldb.debugger.event-handler'
    frame #0: 0x000000018fffaafc libsystem_kernel.dylib`__psynch_mutexwait + 8
    frame #1: 0x0000000190034358 libsystem_pthread.dylib`_pthread_mutex_firstfit_lock_wait + 84
    frame #2: 0x0000000190031cbc libsystem_pthread.dylib`_pthread_mutex_firstfit_lock_slow + 248
    frame #3: 0x000000018ff86b7c libc++.1.dylib`std::__1::recursive_mutex::lock() + 16
    frame #4: 0x000000010dc7c630 liblldb.15.0.0git.dylib`lldb_private::StackFrame::GetStackID() + 32

^^^^ frame #4 is trying to get the StackFrame mutex held by thread 1

  frame #5: 0x000000010dc85c34 liblldb.15.0.0git.dylib`lldb_private::StackFrameList::GetFrameWithStackID(lldb_private::StackID const&) + 112

^^^^ frame #5 is the frame that acquires the StackFrameList mutex  ^^^^^^^^

  frame #6: 0x000000010dcbad44 liblldb.15.0.0git.dylib`lldb_private::Thread::GetFrameWithStackID(lldb_private::StackID const&) + 64
  frame #7: 0x000000010dc3cd94 liblldb.15.0.0git.dylib`lldb_private::ExecutionContextRef::GetFrameSP() const + 84
  frame #8: 0x000000010dc3d04c liblldb.15.0.0git.dylib`lldb_private::ExecutionContext::ExecutionContext(lldb_private::ExecutionContextRef const*, bool) + 528
  frame #9: 0x000000010db3435c liblldb.15.0.0git.dylib`lldb_private::ValueObject::EvaluationPoint::SyncWithProcessState(bool) + 52
  frame #10: 0x000000010db2ae28 liblldb.15.0.0git.dylib`lldb_private::ValueObject::UpdateValueIfNeeded(bool) + 120
  frame #11: 0x000000010db2e678 liblldb.15.0.0git.dylib`lldb_private::ValueObject::GetValueAsCString(lldb_private::TypeFormatImpl const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&) + 36
  frame #12: 0x000000010db2e840 liblldb.15.0.0git.dylib`lldb_private::ValueObject::GetValueAsCString() + 300
  frame #13: 0x000000010db2f1b8 liblldb.15.0.0git.dylib`lldb_private::ValueObject::DumpPrintableRepresentation(lldb_private::Stream&, lldb_private::ValueObject::ValueObjectRepresentationStyle, lldb::Format, lldb_private::ValueObject::PrintableRepresentationSpecialCases, bool) + 1092
  frame #14: 0x000000010dae0af0 liblldb.15.0.0git.dylib`lldb_private::FormatEntity::Format(lldb_private::FormatEntity::Entry const&, lldb_private::Stream&, lldb_private::SymbolContext const*, lldb_private::ExecutionContext const*, lldb_private::Address const*, lldb_private::ValueObject*, bool, bool) + 9724
  frame #15: 0x000000010daded2c liblldb.15.0.0git.dylib`lldb_private::FormatEntity::Format(lldb_private::FormatEntity::Entry const&, lldb_private::Stream&, lldb_private::SymbolContext const*, lldb_private::ExecutionContext const*, lldb_private::Address const*, lldb_private::ValueObject*, bool, bool) + 2104
  frame #16: 0x000000010daded2c liblldb.15.0.0git.dylib`lldb_private::FormatEntity::Format(lldb_private::FormatEntity::Entry const&, lldb_private::Stream&, lldb_private::SymbolContext const*, lldb_private::ExecutionContext const*, lldb_private::Address const*, lldb_private::ValueObject*, bool, bool) + 2104
  frame #17: 0x000000010dadecdc liblldb.15.0.0git.dylib`lldb_private::FormatEntity::Format(lldb_private::FormatEntity::Entry const&, lldb_private::Stream&, lldb_private::SymbolContext const*, lldb_private::ExecutionContext const*, lldb_private::Address const*, lldb_private::ValueObject*, bool, bool) + 2024
  frame #18: 0x000000010dc81d54 liblldb.15.0.0git.dylib`lldb_private::StackFrame::DumpUsingSettingsFormat(lldb_private::Stream*, bool, char const*) + 244
  frame #19: 0x000000010dc82418 liblldb.15.0.0git.dylib`lldb_private::StackFrame::GetStatus(lldb_private::Stream&, bool, bool, bool, char const*) + 92
  frame #20: 0x000000010dc86488 liblldb.15.0.0git.dylib`lldb_private::StackFrameList::GetStatus(lldb_private::Stream&, unsigned int, unsigned int, bool, unsigned int, bool, char const*) + 580
  frame #21: 0x000000010dcb8e24 liblldb.15.0.0git.dylib`lldb_private::Thread::GetStatus(lldb_private::Stream&, unsigned int, unsigned int, unsigned int, bool, bool) + 964
  frame #22: 0x000000010dacb6d4 liblldb.15.0.0git.dylib`lldb_private::Debugger::HandleThreadEvent(std::__1::shared_ptr<lldb_private::Event> const&) + 184
  frame #23: 0x000000010dacba48 liblldb.15.0.0git.dylib`lldb_private::Debugger::DefaultEventHandler() + 556
  frame #24: 0x000000010dace1f4 liblldb.15.0.0git.dylib`std::__1::__function::__func<lldb_private::Debugger::StartEventHandlerThread()::$_3, std::__1::allocator<lldb_private::Debugger::StartEventHandlerThread()::$_3>, void* ()>::operator()() + 16
  frame #25: 0x000000010db8cf60 liblldb.15.0.0git.dylib`lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(void*) + 100
  frame #26: 0x000000010e833e18 liblldb.15.0.0git.dylib`lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(void*) + 32
  frame #27: 0x0000000190037280 libsystem_pthread.dylib`_pthread_start + 148

This is happening because StackFrame::GetValueObjectForFrameVariable holds the StackFrame mutex over the call to ValueObject::GetDynamicValue, which is not right.  It's not necessary because the ValueObject we've made doesn't rely directly on its StackFrame, it depends on the StackID instead, and isn't modifying the StackFrame either.  And because it can cause lock inversion against the common pattern of getting the StackFrameList, and then asking it for some StackFrame, doing so is dangerous as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130524

Files:
  lldb/source/Target/StackFrame.cpp


Index: lldb/source/Target/StackFrame.cpp
===================================================================

--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -1145,26 +1145,34 @@
 ValueObjectSP
 StackFrame::GetValueObjectForFrameVariable(const VariableSP &variable_sp,
                                            DynamicValueType use_dynamic) {
-  std::lock_guard<std::recursive_mutex> guard(m_mutex);
   ValueObjectSP valobj_sp;
-  if (IsHistorical()) {
-    return valobj_sp;
-  }
-  VariableList *var_list = GetVariableList(true);
-  if (var_list) {
-    // Make sure the variable is a frame variable
-    const uint32_t var_idx = var_list->FindIndexForVariable(variable_sp.get());
-    const uint32_t num_variables = var_list->GetSize();
-    if (var_idx < num_variables) {
-      valobj_sp = m_variable_list_value_objects.GetValueObjectAtIndex(var_idx);
-      if (!valobj_sp) {
-        if (m_variable_list_value_objects.GetSize() < num_variables)
-          m_variable_list_value_objects.Resize(num_variables);
-        valobj_sp = ValueObjectVariable::Create(this, variable_sp);
-        m_variable_list_value_objects.SetValueObjectAtIndex(var_idx, valobj_sp);
+  { // Scope for stack frame mutex.  We need to drop this mutex before we figure
+    // out the dynamic value.  That will require converting the StackID in the 
+    // VO back to a StackFrame, which will in turn require locking the 
+    // StackFrameList.  If we still hold the StackFrame mutex, we could suffer 
+    // lock inversion against the pattern of getting the StackFrameList and 
+    // then the stack frame, which is fairly common.
+    std::lock_guard<std::recursive_mutex> guard(m_mutex);
+    if (IsHistorical()) {
+      return valobj_sp;
+    }
+    VariableList *var_list = GetVariableList(true);
+    if (var_list) {
+      // Make sure the variable is a frame variable
+      const uint32_t var_idx = var_list->FindIndexForVariable(variable_sp.get());
+      const uint32_t num_variables = var_list->GetSize();
+      if (var_idx < num_variables) {
+        valobj_sp = m_variable_list_value_objects.GetValueObjectAtIndex(var_idx);
+        if (!valobj_sp) {
+          if (m_variable_list_value_objects.GetSize() < num_variables)
+            m_variable_list_value_objects.Resize(num_variables);
+          valobj_sp = ValueObjectVariable::Create(this, variable_sp);
+          m_variable_list_value_objects.SetValueObjectAtIndex(var_idx,
+                                                              valobj_sp);
+        }
       }
     }
-  }
+  } // End of StackFrame mutex scope.
   if (use_dynamic != eNoDynamicValues && valobj_sp) {
     ValueObjectSP dynamic_sp = valobj_sp->GetDynamicValue(use_dynamic);
     if (dynamic_sp)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D130524.447485.patch
Type: text/x-patch
Size: 2759 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220725/179a2ec2/attachment-0001.bin>
