[lldb/API] Mark SBValue with error as invalid

[medismailben]

This patch fixes the return value of SBValue::IsValid if GetError returns a failing SBError.

That alignes better the expectation that an SBValue is invalid if it has an error.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Med Ismail Bennani (medismailben)

Changes

This patch fixes the return value of SBValue::IsValid if GetError returns a failing SBError.

That alignes better the expectation that an SBValue is invalid if it has an error.

---

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

3 Files Affected:

• (modified) lldb/source/API/SBValue.cpp (+2-1)
• (added) lldb/test/API/python_api/sbvalue_is_valid/TestSBValueIsValid.py (+3)
• (added) lldb/test/API/python_api/sbvalue_is_valid/main.cpp (+6)

diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index e300ecee3f8ac..77cc7d1681829 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -97,7 +97,8 @@ class ValueImpl {
       // they depend on.  So I have no good way to make that check without
       // tracking that in all the ValueObject subclasses.
       TargetSP target_sp = m_valobj_sp->GetTargetSP();
-      return target_sp && target_sp->IsValid();
+      return target_sp && target_sp->IsValid() &&
+             m_valobj_sp->GetError().Success();
     }
   }
 
diff --git a/lldb/test/API/python_api/sbvalue_is_valid/TestSBValueIsValid.py b/lldb/test/API/python_api/sbvalue_is_valid/TestSBValueIsValid.py
new file mode 100644
index 0000000000000..a3d43c1bdeeb2
--- /dev/null
+++ b/lldb/test/API/python_api/sbvalue_is_valid/TestSBValueIsValid.py
@@ -0,0 +1,3 @@
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
diff --git a/lldb/test/API/python_api/sbvalue_is_valid/main.cpp b/lldb/test/API/python_api/sbvalue_is_valid/main.cpp
new file mode 100644
index 0000000000000..2a1dfe06faa77
--- /dev/null
+++ b/lldb/test/API/python_api/sbvalue_is_valid/main.cpp
@@ -0,0 +1,6 @@
+int main () {
+  int i = 42;
+  return 0; //% v1 = self.frame().EvaluateExpression("test"); v2 = self.frame().EvaluateExpression("i");
+  //% self.assertFalse(v1.IsValid())
+  //% self.assertTrue(v2.IsValid())
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[medismailben]

I fixed the formatting and most of the failures. I only need to find the right way to return the errors through DAP: prior to this change, errors were returned as part of the value object string, since the object was still considered valid but now that it's marked invalid, the return string is empty. @JDevlieghere what would be the right approach here ?

[JDevlieghere]

I was under the impression that these two (IsValid and GetError) were intentionally decoupled, although I don't remember the reasoning. I vaguely remember this came up when @PortalPete was working on this.

@adrian-prantl / @jimingham Do I misremember?

[jimingham]

This doesn't sound like a good idea to me. IsValid means "will I get meaningful information back if I ask a question of this object". You certainly want to know whether the Error state of the SBValue is in fact valid. So an SBValue with valid error information should always be in the IsValid = true state.

[jimingham]

Note, another way to do this would be to have default constructed SBValues have a Failed error state of "Nothing in me now". Then you could dispense with SBValie.IsValid altogether, since there wouldn't be any SBValues that don't at least have valid error states.

[medismailben]

This behavior is counterintuitive. Once you have an SBValue, there isn’t really a meaningful "question" the user can ask of it besides calling GetError. Checking whether the underlying pointer is non-null is just an implementation detail.

In C++, std::make_shared can only fail in two cases: out-of-memory (std::bad_alloc) or if the constructor of the managed object throws. But since LLVM/LLDB are built with exceptions disabled, either case would immediately terminate the LLDB. So if LLDB is still running, the pointer you got is guaranteed to be valid.

That makes IsValid() in its current form effectively useless: it never provides actionable information. Since we cannot remove IsValid from the API, I think we should make the it more natural and practical to use, without burdening user with redundant validity checks. That’s the reasoning behind my patch.

[JDevlieghere]

This behavior is counterintuitive. Once you have an SBValue, there isn’t really a meaningful "question" the user can ask of it besides calling GetError. Checking whether the underlying pointer is non-null is just an implementation detail.

In C++, std::make_shared can only fail in two cases: out-of-memory (std::bad_alloc) or if the constructor of the managed object throws. But since LLVM/LLDB are built with exceptions disabled, either case would immediately terminate the LLDB. So if LLDB is still running, the pointer you got is guaranteed to be valid.

That makes IsValid() in its current form effectively useless: it never provides actionable information. Since we cannot remove IsValid from the API, I think we should make the it more natural and practical to use, without burdening user with redundant validity checks. That’s the reasoning behind my patch.

I'm not sure I follow this argument. You talk about make_shared failing but that's not what this check is about. Like most of the SB API, the default constructor doesn't initialize the PIMPL pointer which is what IsValid is telling you when it returns false.

[medismailben]

This behavior is counterintuitive. Once you have an SBValue, there isn’t really a meaningful "question" the user can ask of it besides calling GetError. Checking whether the underlying pointer is non-null is just an implementation detail.
In C++, std::make_shared can only fail in two cases: out-of-memory (std::bad_alloc) or if the constructor of the managed object throws. But since LLVM/LLDB are built with exceptions disabled, either case would immediately terminate the LLDB. So if LLDB is still running, the pointer you got is guaranteed to be valid.
That makes IsValid() in its current form effectively useless: it never provides actionable information. Since we cannot remove IsValid from the API, I think we should make the it more natural and practical to use, without burdening user with redundant validity checks. That’s the reasoning behind my patch.

I'm not sure I follow this argument. You talk about make_shared failing but that's not what this check is about. Like most of the SB API, the default constructor doesn't initialize the PIMPL pointer which is what IsValid is telling you when it returns false.

My argument is that given that SBValue::IsValid ONLY checks the pointer validity, it's basically a no-op. I think that instead of only checking the pointer validity, it should also check the object validity like we do for other SB types:

bool SBTarget::IsValid() const {
  LLDB_INSTRUMENT_VA(this);
  return this->operator bool();
}
SBTarget::operator bool() const {
  LLDB_INSTRUMENT_VA(this);

  return m_opaque_sp.get() != nullptr && m_opaque_sp->IsValid();
}

...

bool SBProcess::IsValid() const {
  LLDB_INSTRUMENT_VA(this);
  return this->operator bool();
}
SBProcess::operator bool() const {
  LLDB_INSTRUMENT_VA(this);

  ProcessSP process_sp(m_opaque_wp.lock());
  return ((bool)process_sp && process_sp->IsValid());
}

... 

bool SBLineEntry::IsValid() const {
  LLDB_INSTRUMENT_VA(this);
  return this->operator bool();
}
SBLineEntry::operator bool() const {
  LLDB_INSTRUMENT_VA(this);

  return m_opaque_up.get() && m_opaque_up->IsValid();
}

...