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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
