[Lldb-commits] [lldb] ValueImpl::GetSP was calling GetError before acquiring the StopLocker. (PR #225976)

[via lldb-commits]

================
@@ -3909,9 +3912,20 @@ lldb::ValueObjectSP ValueImpl::GetSP(Process::StopLocker &stop_locker,
     // is running. If you want to look at values, pause the process, then
     // look.
     error = Status::FromErrorString("process must be stopped.");
+    // We still want to return a value object if it was in an error state, but
+    // we can't call GetError here, since that would call UpdateValueIfNeeded
+    // which isn't safe to do without holding the stop locker.
+    if (value_sp->PeekError().Fail())
+      return value_sp;
+
     return ValueObjectSP();
   }
 
+  // Now we can safely get the ValueObject to update itself and if that results
----------------
jimingham wrote:

I think we need to return some error here, but it isn't the ValueObject's error.  

Because we couldn't actually update the value, the error is likely stale.  But we actually have no way of telling how stale it is.  If you had continued 5 times w/o ever checking this ValueObject, then it would be 5 continues stale, for instance.  OTOH it might be a ValueObjectConstResult in which case updates aren't relevant and the error is always accurate.  

So the side-channel error we need to return says "we couldn't update the VO because the process is running returning cached error value instead".  I can make that a separate error from "returning an empty VO since we couldn't update the value" which we emit in the case where the previous error state was "Successful".



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