When a ValueObjectDynamicValue fails to update, return a not valid ValueObject so the static one is used instead

[jingham]

Summary

When we can't find a valid type for a dynamic value (for instance we just have a void * and can't compute anything for it) we used to try to hand out a dynamic value that "behaves like the static value". That mostly works, but getting something to behave like ValueObjectConstResult without being it didn't work - for instance AddressOf fails for this DynamicValue.

The comment before this section of code mentions that we might should return an invalid dynamic value instead, so that the static one would get used. I switched over to that, and I just needed to fix up a few cases where we were handing out a dynamic value w/o making sure it was valid first. That ends up being much simpler than trying to get this ValueObjectDynamicValue to be "just like" ValueObjectConstResult.

[mib]

LGTM with a comment.

lldb/source/Core/ValueObject.cpp

1862

It would be nice if ValueObjectDynamicValue had a IsValid method that returned the result of its Status attribute. Without reading the commit message, GetError doesn't tell us much about what we're trying to achieve here.

lldb/source/Core/ValueObjectConstResult.cpp

290

ditto

[jingham]

lldb/source/Core/ValueObject.cpp

1862

IsValid is less restrictive than having an error. IsValid is true if the value has a variable in it, even if, for instance, we couldn't currently read the memory underlying the value. I want "was there any error at all", so checking the Error is more appropriate here.

lldb/source/Core/ValueObject.cpp

 
 ValueObjectSP ValueObject::GetDynamicValue(DynamicValueType use_dynamic) {
   if (use_dynamic == eNoDynamicValues)
     return ValueObjectSP();
 
   if (!IsDynamic() && m_dynamic_value == nullptr) {
     CalculateDynamicValue(use_dynamic);
   }
-  if (m_dynamic_value)
+  if (m_dynamic_value && m_dynamic_value->GetError().Success())
     return m_dynamic_value->GetSP();
   else
     return ValueObjectSP();
 }
 
 ValueObjectSP ValueObject::GetSyntheticValue() {
   CalculateSyntheticValue();
 

lldb/source/Core/ValueObjectConstResult.cpp

ValueObjectConstResult::GetDynamicValue(lldb::DynamicValueType use_dynamic) {
   // they might point to might have changed at any time.
   if (use_dynamic != eNoDynamicValues) {
     if (!IsDynamic()) {
       ExecutionContext exe_ctx(GetExecutionContextRef());
       Process *process = exe_ctx.GetProcessPtr();
       if (process && process->IsPossibleDynamicValue(*this))
         m_dynamic_value = new ValueObjectDynamicValue(*this, use_dynamic);
     }
-    if (m_dynamic_value)
+    if (m_dynamic_value && m_dynamic_value->GetError().Success())
       return m_dynamic_value->GetSP();
   }
   return ValueObjectSP();
 }
 
 lldb::ValueObjectSP
 ValueObjectConstResult::Cast(const CompilerType &compiler_type) {
   return m_impl.Cast(compiler_type);
 }
 
 lldb::LanguageType ValueObjectConstResult::GetPreferredDisplayLanguage() {
   if (m_preferred_display_language != lldb::eLanguageTypeUnknown)
     return m_preferred_display_language;
   return GetCompilerTypeImpl().GetMinimumLanguage();
 }

lldb/source/Core/ValueObjectDynamicValue.cpp

if (class_type_or_name.HasType()) {
                        .GetCompilerType());
     } else {
       m_type_impl.Clear();
     }
   } else {
     m_type_impl.Clear();
   }
 
-  // If we don't have a dynamic type, then make ourselves just a echo of our
-  // parent. Or we could return false, and make ourselves an echo of our
-  // parent?
+  // If we don't have a dynamic type, set ourselves to be invalid and return
+  // false.  We used to try to produce a dynamic ValueObject that behaved "like"
+  // its parent, but that failed for ValueObjectConstResult, which is too 
+  // complex a beast to try to emulate.  If we return an invalid ValueObject,
+  // clients will end up getting the static value instead, which behaves
+  // correctly.
   if (!found_dynamic_type) {
     if (m_dynamic_type_info)
       SetValueDidChange(true);
     ClearDynamicTypeInformation();
     m_dynamic_type_info.Clear();
-    m_value = m_parent->GetValue();
-    m_error = m_value.GetValueAsData(&exe_ctx, m_data, GetModule().get());
-    return m_error.Success();
+    m_error.SetErrorString("no dynamic type found");
+    return false;
   }
 
   Value old_value(m_value);
 
   Log *log = GetLog(LLDBLog::Types);
 
   bool has_changed_type = false;
 

lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp

if (!error.Success()) {
     return ValueObjectSP();
   }
 
   lldb_private::formatters::InferiorSizedWord exception_isw(exception_addr,
                                                             *m_process);
   ValueObjectSP exception = ValueObject::CreateValueObjectFromData(
       "exception", exception_isw.GetAsData(m_process->GetByteOrder()), exe_ctx,
       voidstar);
-  exception = exception->GetDynamicValue(eDynamicDontRunTarget);
+  ValueObjectSP dyn_exception 
+      = exception->GetDynamicValue(eDynamicDontRunTarget);
+  // If we succeed in making a dynamic value, return that:
+  if (dyn_exception) 
+     return dyn_exception;
 
   return exception;
 }
 
 TypeAndOrName ItaniumABILanguageRuntime::GetDynamicTypeInfo(
     const lldb_private::Address &vtable_addr) {
   std::lock_guard<std::mutex> locker(m_dynamic_type_map_mutex);
   DynamicTypeCache::const_iterator pos = m_dynamic_type_map.find(vtable_addr);

lldb/test/API/python_api/value/addr_of_void_star/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/python_api/value/addr_of_void_star/TestValueAPIAddressOfVoidStar.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class ValueAPIVoidStarTestCase(TestBase):
+
+    def test(self):
+        self.build()
+        
+        target, process, thread, _ = lldbutil.run_to_source_breakpoint(self,
+                                                                       "Break at this line",
+                                                                       lldb.SBFileSpec("main.c"))
+        frame = thread.GetFrameAtIndex(0)
+
+        # Verify that the expression result for a void * behaves the same way as the
+        # variable value.
+
+        var_val = frame.FindVariable("void_ptr")
+        self.assertSuccess(var_val.GetError(), "Var version made correctly")
+        
+        expr_val = frame.EvaluateExpression("void_ptr")
+        self.assertSuccess(expr_val.GetError(), "Expr version succeeds")
+
+        # The pointer values should be equal:
+        self.assertEqual(var_val.unsigned, expr_val.unsigned, "Values are equal")
+
+        # Both versions should have valid AddressOf, and they should be the same.
+
+        val_addr_of = var_val.AddressOf()
+        self.assertNotEqual(val_addr_of, lldb.LLDB_INVALID_ADDRESS, "Var addr of right")
+
+        expr_addr_of = expr_val.AddressOf()
+        self.assertNotEqual(expr_addr_of, lldb.LLDB_INVALID_ADDRESS, "Expr addr of right")
+
+        # The AddressOf values should also be equal.
+        self.assertEqual(expr_addr_of.unsigned, val_addr_of.unsigned, "Addr of equal")
+

lldb/test/API/python_api/value/addr_of_void_star/main.c

+int main (int argc, char const *argv[]) {
+  char *char_ptr = "Some pointer here";
+  void *void_ptr = &char_ptr;
+  
+  return 0; // Break at this line
+}