https://github.com/jimingham created https://github.com/llvm/llvm-project/pull/80222
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. >From c6d76783fce826f01fe4713e3cc58504d591aebf Mon Sep 17 00:00:00 2001 From: Jim Ingham <jing...@apple.com> Date: Fri, 8 Dec 2023 14:57:23 -0800 Subject: [PATCH] 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. --- lldb/include/lldb/Core/ValueObject.h | 5 +++++ lldb/source/API/SBValue.cpp | 17 +++++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) 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(); } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits