[lldb] Correctly check and report error states in StackFrame.cpp

[PortalPete]

This commits fixes a few subtle bugs where the method:

1. Declares a local Status error which eclipses the method's parameter Status &error.

   • The method then sets the error state to the local error and returns without ever touching the parameter &error.
   • This effectively traps the error state and its message from ever reaching the caller.
   • I also threw in a null pointer check in case the callee doesn't set its Status parameter but returns 0/nullptr.

2. Declares a local Status deref_error (good), passes it to the Dereference method (also good), but then checks the status of the method's Status &error parameter (not good).

   • The fix checks deref_error instead and also checks for a nullptr return value.
   • There's a good opportunity here for a future PR that changes the Dereference method to fold an error state into the ValueObject return value's m_error instead of using a parameter.

3. Declares another local Status error, which it doesn't pass to a method (because there isn't a parameter for it), and then checks for an error condition that never happens.

   • The fix just checks the callee's return value, because that's all it has to go on.
   • This likely comes from a copy/paste from issue 1 above.

rdar://119155810

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pete Lawrence (PortalPete)

Changes

rdar://119155810

---

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

1 Files Affected:

• (modified) lldb/source/Target/StackFrame.cpp (+10-12)

diff --git a/lldb/source/Target/StackFrame.cpp b/lldb/source/Target/StackFrame.cpp
index f0d78d8aa5fef..50cf01e63cd49 100644
--- a/lldb/source/Target/StackFrame.cpp
+++ b/lldb/source/Target/StackFrame.cpp
@@ -652,7 +652,7 @@ ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
         Status deref_error;
         if (valobj_sp->GetCompilerType().IsReferenceType()) {
           valobj_sp = valobj_sp->GetSyntheticValue()->Dereference(deref_error);
-          if (error.Fail()) {
+          if (!valobj_sp || deref_error.Fail()) {
             error.SetErrorStringWithFormatv(
                 "Failed to dereference reference type: %s", deref_error);
             return ValueObjectSP();
@@ -660,7 +660,7 @@ ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
         }
 
         valobj_sp = valobj_sp->Dereference(deref_error);
-        if (error.Fail()) {
+        if (!valobj_sp || deref_error.Fail()) {
           error.SetErrorStringWithFormatv(
               "Failed to dereference sythetic value: {0}", deref_error);
           return ValueObjectSP();
@@ -795,9 +795,9 @@ ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
           // what we have is *ptr[low]. the most similar C++ syntax is to deref
           // ptr and extract bit low out of it. reading array item low would be
           // done by saying ptr[low], without a deref * sign
-          Status error;
-          ValueObjectSP temp(valobj_sp->Dereference(error));
-          if (error.Fail()) {
+          Status deref_error;
+          ValueObjectSP temp(valobj_sp->Dereference(deref_error));
+          if (!temp || deref_error.Fail()) {
             valobj_sp->GetExpressionPath(var_expr_path_strm);
             error.SetErrorStringWithFormat(
                 "could not dereference \"(%s) %s\"",
@@ -813,9 +813,8 @@ ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
           // arr[0] (an operation that is equivalent to deref-ing arr) and
           // extract bit low out of it. reading array item low would be done by
           // saying arr[low], without a deref * sign
-          Status error;
           ValueObjectSP temp(valobj_sp->GetChildAtIndex(0));
-          if (error.Fail()) {
+          if (!temp) {
             valobj_sp->GetExpressionPath(var_expr_path_strm);
             error.SetErrorStringWithFormat(
                 "could not get item 0 for \"(%s) %s\"",
@@ -994,9 +993,9 @@ ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
         // deref ptr and extract bits low thru high out of it. reading array
         // items low thru high would be done by saying ptr[low-high], without a
         // deref * sign
-        Status error;
-        ValueObjectSP temp(valobj_sp->Dereference(error));
-        if (error.Fail()) {
+        Status deref_error;
+        ValueObjectSP temp(valobj_sp->Dereference(deref_error));
+        if (!temp || deref_error.Fail()) {
           valobj_sp->GetExpressionPath(var_expr_path_strm);
           error.SetErrorStringWithFormat(
               "could not dereference \"(%s) %s\"",
@@ -1011,9 +1010,8 @@ ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
         // get arr[0] (an operation that is equivalent to deref-ing arr) and
         // extract bits low thru high out of it. reading array items low thru
         // high would be done by saying arr[low-high], without a deref * sign
-        Status error;
         ValueObjectSP temp(valobj_sp->GetChildAtIndex(0));
-        if (error.Fail()) {
+        if (!temp) {
           valobj_sp->GetExpressionPath(var_expr_path_strm);
           error.SetErrorStringWithFormat(
               "could not get item 0 for \"(%s) %s\"",

[DavidSpickett]

Is testing this possible?

[felipepiovezan]

It is wild that all of these have gone unnoticed for such a long time. I guess it makes sense, as these are more of sanity checks that we never expect to fire?

[adrian-prantl]

Is testing this possible?

If this weren't inside of StackFrame we could write a reliable test based on LLVM IR, but it might be difficult to reliably produce debug info with optimized out variables from source code on all platforms. If you have any suggestions, let me know!

[DavidSpickett]

Testing sounds like more effort than it's worth, let's not block this on that.

[felipepiovezan]

Clicking merge on @PortalPete 's behalf

[PortalPete]

Clicking merge on @PortalPete 's behalf

Thank you, sir!