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

2024-02-13 Thread via lldb-commits 

jimingham wrote:

I thought I had some clever idea that made me think this would be sufficient.  
But on further thought, I think I was just delusional.  I'll have to think 
about this some more.

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

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

2024-02-13 Thread via lldb-commits 

https://github.com/jimingham closed 
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

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

2024-02-01 Thread Adrian Prantl via lldb-commits 


@@ -458,7 +458,12 @@ class ValueObject {
   virtual bool GetDeclaration(Declaration );
 
   // 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 ();
+  
+  // Check the current error state without updating the value:

adrian-prantl wrote:

`///`

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

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

2024-02-01 Thread Adrian Prantl via lldb-commits 


@@ -114,21 +114,26 @@ 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;  

adrian-prantl wrote:

Doesn't whatever call site this is returned to now also need to call the new 
non-updating API to surface it to the user?

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

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

2024-01-31 Thread Greg Clayton via lldb-commits 


@@ -458,7 +458,12 @@ class ValueObject {
   virtual bool GetDeclaration(Declaration );
 
   // 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 ();
+  
+  // Check the current error state without updating the value:
+  const Status () { return m_error; }

clayborg wrote:

`PeekError()`?

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

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

2024-01-31 Thread Greg Clayton via lldb-commits 

https://github.com/clayborg commented:

LGTM with a possible rename mentioned in inline comments. Anyone else have 
anything?

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

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

2024-01-31 Thread Greg Clayton via lldb-commits 

https://github.com/clayborg edited 
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

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

2024-01-31 Thread via lldb-commits 

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff a03a6e99647318a86ea398c42e241da43e3c550e 
c6d76783fce826f01fe4713e3cc58504d591aebf -- 
lldb/include/lldb/Core/ValueObject.h lldb/source/API/SBValue.cpp
``





View the diff from clang-format here.


``diff
diff --git a/lldb/include/lldb/Core/ValueObject.h 
b/lldb/include/lldb/Core/ValueObject.h
index db8023618b..e18ad3153a 100644
--- a/lldb/include/lldb/Core/ValueObject.h
+++ b/lldb/include/lldb/Core/ValueObject.h
@@ -461,7 +461,7 @@ public:
   // This gets the current error for this ValueObject, it may update the value
   // to ensure that the error is up to date.
   const Status ();
-  
+
   // Check the current error state without updating the value:
   const Status () { return m_error; }
 
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index 182ae669d8..1069ec0e42 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -117,7 +117,7 @@ public:
 if (!target) {
   // If we can't get the target, the error might still be useful:
   if (value_sp->CheckError().Fail())
-return value_sp;  
+return value_sp;
   return ValueObjectSP();
 }
 lock = std::unique_lock(target->GetAPIMutex());

``




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

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

2024-01-31 Thread via lldb-commits 

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: None (jimingham)


Changes

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 );
 
   // 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 ();
+  
+  // Check the current error state without updating the value:
+  const Status () { 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(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();
 }
 

``




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

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

2024-01-31 Thread via lldb-commits 

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 
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 );
 
   // 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 ();
+  
+  // Check the current error state without updating the value:
+  const Status () { 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(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