[Lldb-commits] [lldb] Don't cause a premature UpdateIfNeeded when checking in SBValue::GetSP (PR #80222)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: None (jimingham)

<details>
<summary>Changes</summary>

Don't cause a premature UpdateIfNeeded when checking whether an otherwise invalid SBValue with a useful error should be handed out from GetSP.

This is a follow-on to e8a2fd5e7be2.  In that change, I used GetError to check for an error at the beginning of GetSP, before checking for a valid Target or a stopped Process.  But GetError calls UpdateIfNeeded.  Normally that's not a problem as somewhere along UpdateIfNeeded the code will realize it can't update and return w/o doing any harm.

But when running lldb in a multithreaded environment (e.g. in Xcode) if you are very unlucky you can get the update to start before you proceed and then just stall waiting to get access to gdb-remote while the process is running.

The change is to add ValueObject::CheckError that returns the error w/o UpdatingIfNeeded, and use that in SBValue::GetSP in the places where we would normally return an invalid SP.

I tried to make a test for this but it's unobservable unless you are very unlucky, and I couldn't figure out a way to be consistently unlucky.

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


2 Files Affected:

- (modified) lldb/include/lldb/Core/ValueObject.h (+5) 
- (modified) lldb/source/API/SBValue.cpp (+11-6) 


``````````diff
diff --git a/lldb/include/lldb/Core/ValueObject.h b/lldb/include/lldb/Core/ValueObject.h
index 4c0b0b2dae6cd..db8023618b7f6 100644
--- a/lldb/include/lldb/Core/ValueObject.h
+++ b/lldb/include/lldb/Core/ValueObject.h
@@ -458,7 +458,12 @@ class ValueObject {
   virtual bool GetDeclaration(Declaration &decl);
 
   // The functions below should NOT be modified by subclasses
+  // This gets the current error for this ValueObject, it may update the value
+  // to ensure that the error is up to date.
   const Status &GetError();
+  
+  // Check the current error state without updating the value:
+  const Status &CheckError() { return m_error; }
 
   ConstString GetName() const { return m_name; }
 
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index 89d26a1fbe282..182ae669d880b 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -114,13 +114,12 @@ class ValueImpl {
     lldb::ValueObjectSP value_sp = m_valobj_sp;
 
     Target *target = value_sp->GetTargetSP().get();
-    // If this ValueObject holds an error, then it is valuable for that.
-    if (value_sp->GetError().Fail())
-      return value_sp;
-
-    if (!target)
+    if (!target) {
+      // If we can't get the target, the error might still be useful:
+      if (value_sp->CheckError().Fail())
+        return value_sp;      
       return ValueObjectSP();
-
+    }
     lock = std::unique_lock<std::recursive_mutex>(target->GetAPIMutex());
 
     ProcessSP process_sp(value_sp->GetProcessSP());
@@ -128,7 +127,13 @@ class ValueImpl {
       // We don't allow people to play around with ValueObject if the process
       // is running. If you want to look at values, pause the process, then
       // look.
+      // However, if this VO already had an error, then that is worth showing
+      // the user.  However, we can't update it so use CheckError not GetError.
       error.SetErrorString("process must be stopped.");
+
+      if (value_sp->CheckError().Fail())
+        return value_sp;
+
       return ValueObjectSP();
     }
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/80222