Delegate UpdateChildrenPointerType to the Root ValueObject

[jingham]

Summary

This is a refinement on 96601ec28b7efe5abf3479a1aa91bcedb235bbbd. The intent of that change was to do the same work for the computation of the locations of the children of ValueObjectVariable as was done for the root ValueObjectVariable. This original patch did that by moving the computation from ValueObjectVariable to ValueObject. That fixed the problem but caused a handful of swift-lldb testsuite failures and a crash or two.

The problem is that synthetic value objects can sometimes represent objects in target memory, and other times they might be made up wholly in lldb memory, with pointers from one synthetic object to another, and so the ValueObjectVariable computation was not appropriate.

This patch delegates the computation to the root of the ValueObject in question. That solves the problem for ValueObjectVariable while not messing up the computation for ValueObjectConstResult or ValueObjectSynthetic.

This patch resolves all the swift-lldb failures and also still passes the non-swift testsuite.

The representation through ValueObjects of Swift Types is quite byzantine, and the ValueObject system is frustratingly self-healing, and I haven't yet been able to cons up a non-swift test case that will trigger the same failures we were seeing with swift data types. I'm still working on that, but I thought I'd get the code changes out for review while working on that, since we actually DO have tests for the change, just not in the non-swift tree...

[labath]

Assuming the goal is to make this code ValueObjectVariable-only, the code is fine. I can't say I understand why should it be ValueObjectVariable-only (*), but given how different the ValueObjectConstResult hierarchy is, I think that may be for the best. It would still be good to have a test case though, as the ValueObject hierarchy is in dire need of cleanup, and knowing all the constraints would definitely help.

(*) For example, I would have thought that removing this code from ValueObjectConstResult would make the test case added in D69273 fail if "target variable" was replaced by the "expression" command. It turns out that is not the case -- expr works just fine. My guess is that this is because the ValueObjectConstResult classes are peppered with explicit SetAddressTypeOfChildren(eAddressTypeLoad) calls, which means the address never ends up being interpreted as a host address. It also explains why it was easy for that patch to flip an address type from "load" to "host", but it does not explain (to me, at least) why "host" would not be the right address type.

(**) E.g. the fact that we have both ValueObject{Cast,Child} and ValueObjectConstResult{Cast,Child} tells me that there is probably something wrong with the separation of concerns.

[jingham]

I am not selling this as the correct long-term structure for ValueObjects. So far as I can see, in all cases it should be possible to tell a ValueObject where it's going to find both its own store and the storage for pointer children when the ValueObject is created. It's not like that's a feature we discover as we go through the process of creating the ValueObject. That isn't how it is currently done. Now we almost always start off an incorrect setting, and often goes through several rounds of resetting before it settles on the final value. That's what I meant by saying it is "frustratingly self-healing".

I also agree that the ValueObject classes and the jobs they do aren't terribly coherent.

For instance, ValueObjectConstResult gets used both for expression results and to make the child value for Synthetic children. When you make a child const-result ValueObject for a pointer value, you always look for the value in target memory. They represent a value we've produced from the target, not something we've made up in lldb. So setting a ConstResult child location to "load" when you go to fetch a Child is correct. But then once the Child's child value is retrieved from target memory, it is copied into host memory, and should not be refetched. Then if the target has been continued, any unfetched pointer child values become unrecoverable.

That's very different from Synthetic values. Synthetic values can be anything. In particular I could want to make a synthetic value that holds a pointer to another synthetic value, both of which I've made up by doing some computations but which neither of them are backed by anything in target memory. But synthetic values can also be just one of the children of the parent value. I suspect that the const result classes got weird in part by trying to serve those two masters.

I'm planning on continuing to dig into this stuff, and hopefully come up with something more coherent. But your fix is needed for ValueObjectVariables to avoid crashes when debugging optimized code, and so I need a short-term way to get it to work but not cause problems for Synthetic values. This fix is mostly correct. TTTT, I think if you were being more rigorous about this direction of fix, you would have UpdateChildrenAddressType walk back up the parent hierarchy asking at each level whether the parent knows where its pointer children should be found. But that's adding complexity that doesn't solve any actual problem, and we'd probably end up ripping it out again in a better solution.

On testing, do you mind if I check this in while I continue to work on getting a test. I'd like to get this into the next swift lldb release, and it would be much cleaner to fix it here and get our sources back sync'ed with the non-swift ones. If that really bugs you I can do it just on the swift side, but...

[labath]

Right. It does bug me a little that this has no test, but I don't think it's the worst that could happen. And you're the one who's going to have to debug this all over again if the next value object change breaks swift again.

In D83450#2142114, @jingham wrote:

I'm planning on continuing to dig into this stuff, and hopefully come up with something more coherent. But your fix is needed for ValueObjectVariables to avoid crashes when debugging optimized code, and so I need a short-term way to get it to work but not cause problems for Synthetic values. This fix is mostly correct. TTTT, I think if you were being more rigorous about this direction of fix, you would have UpdateChildrenAddressType walk back up the parent hierarchy asking at each level whether the parent knows where its pointer children should be found. But that's adding complexity that doesn't solve any actual problem, and we'd probably end up ripping it out again in a better solution.

Yes, I've been wondering if something like that wasn't needed, but I wasn't sure how all the parent-child relationships work...

[jingham]

In D83450#2143631, @labath wrote:

Right. It does bug me a little that this has no test, but I don't think it's the worst that could happen. And you're the one who's going to have to debug this all over again if the next value object change breaks swift again.

Thanks!

In D83450#2142114, @jingham wrote:

I'm planning on continuing to dig into this stuff, and hopefully come up with something more coherent. But your fix is needed for ValueObjectVariables to avoid crashes when debugging optimized code, and so I need a short-term way to get it to work but not cause problems for Synthetic values. This fix is mostly correct. TTTT, I think if you were being more rigorous about this direction of fix, you would have UpdateChildrenAddressType walk back up the parent hierarchy asking at each level whether the parent knows where its pointer children should be found. But that's adding complexity that doesn't solve any actual problem, and we'd probably end up ripping it out again in a better solution.

Yes, I've been wondering if something like that wasn't needed, but I wasn't sure how all the parent-child relationships work...

The reason for not doing it that way immediately is that I see no reason why the entity creating a subordinate ValueObject can't know where the ValueObject it is creating will find its data and the data for anything that reaches outside the immediate Value, when it creates it. So adding more complexity to UpdateChildrenAddressType just makes the whole system more Ptolemaic, and it's got way too many epicycles as it stands.

lldb/include/lldb/Core/ValueObject.h

protected:
 
   const char *GetLocationAsCStringImpl(const Value &value,
                                        const DataExtractor &data);
 
   bool IsChecksumEmpty();
 
   void SetPreferredDisplayLanguageIfNeeded(lldb::LanguageType);
 
+protected:
+  virtual void DoUpdateChildrenAddressType(ValueObject &valobj) { return; };
+
 private:
   virtual CompilerType MaybeCalculateCompleteType();
-  void UpdateChildrenAddressType();
+  void UpdateChildrenAddressType() {
+    GetRoot()->DoUpdateChildrenAddressType(*this);
+  }
 
   lldb::ValueObjectSP GetValueForExpressionPath_Impl(
       llvm::StringRef expression_cstr,
       ExpressionPathScanEndReason *reason_to_stop,
       ExpressionPathEndResultType *final_value_type,
       const GetValueForExpressionPathOptions &options,
       ExpressionPathAftermath *final_task_on_target);
 

lldb/include/lldb/Core/ValueObjectVariable.h

public:
   bool SetValueFromCString(const char *value_str, Status &error) override;
 
   bool SetData(DataExtractor &data, Status &error) override;
 
   lldb::VariableSP GetVariable() override { return m_variable_sp; }
 
 protected:
   bool UpdateValue() override;
   
+  void DoUpdateChildrenAddressType(ValueObject &valobj) override;
+
   CompilerType GetCompilerTypeImpl() override;
 
   lldb::VariableSP
       m_variable_sp;      ///< The variable that this value object is based upon
   Value m_resolved_value; ///< The value that DWARFExpression resolves this
                           ///variable to before we patch it up
 
 private:

lldb/source/Core/ValueObject.cpp

if (exe_scope) {
     }
   }
   m_manager->ManageObject(this);
 }
 
 // Destructor
 ValueObject::~ValueObject() {}
 
-void ValueObject::UpdateChildrenAddressType() {
-  Value::ValueType value_type = m_value.GetValueType();
-  ExecutionContext exe_ctx(GetExecutionContextRef());
-  Process *process = exe_ctx.GetProcessPtr();
-  const bool process_is_alive = process && process->IsAlive();
-  const uint32_t type_info = GetCompilerType().GetTypeInfo();
-  const bool is_pointer_or_ref =
-      (type_info & (lldb::eTypeIsPointer | lldb::eTypeIsReference)) != 0;
-
-  switch (value_type) {
-  case Value::eValueTypeFileAddress:
-    // If this type is a pointer, then its children will be considered load
-    // addresses if the pointer or reference is dereferenced, but only if
-    // the process is alive.
-    //
-    // There could be global variables like in the following code:
-    // struct LinkedListNode { Foo* foo; LinkedListNode* next; };
-    // Foo g_foo1;
-    // Foo g_foo2;
-    // LinkedListNode g_second_node = { &g_foo2, NULL };
-    // LinkedListNode g_first_node = { &g_foo1, &g_second_node };
-    //
-    // When we aren't running, we should be able to look at these variables
-    // using the "target variable" command. Children of the "g_first_node"
-    // always will be of the same address type as the parent. But children
-    // of the "next" member of LinkedListNode will become load addresses if
-    // we have a live process, or remain a file address if it was a file
-    // address.
-    if (process_is_alive && is_pointer_or_ref)
-      SetAddressTypeOfChildren(eAddressTypeLoad);
-    else
-      SetAddressTypeOfChildren(eAddressTypeFile);
-    break;
-  case Value::eValueTypeHostAddress:
-    // Same as above for load addresses, except children of pointer or refs
-    // are always load addresses. Host addresses are used to store freeze
-    // dried variables. If this type is a struct, the entire struct
-    // contents will be copied into the heap of the
-    // LLDB process, but we do not currently follow any pointers.
-    if (is_pointer_or_ref)
-      SetAddressTypeOfChildren(eAddressTypeLoad);
-    else
-      SetAddressTypeOfChildren(eAddressTypeHost);
-    break;
-  case Value::eValueTypeLoadAddress:
-  case Value::eValueTypeScalar:
-  case Value::eValueTypeVector:
-    SetAddressTypeOfChildren(eAddressTypeLoad);
-    break;
-  }
-}
-
 bool ValueObject::UpdateValueIfNeeded(bool update_format) {
 
   bool did_change_formats = false;
 
   if (update_format)
     did_change_formats = UpdateFormatsIfNeeded();
 
   // If this is a constant value, then our success is predicated on whether we

lldb/source/Core/ValueObjectVariable.cpp

if (expr.Evaluate(&exe_ctx, nullptr, loclist_base_load_addr, nullptr,
       }
 
       SetValueIsValid(m_error.Success());
     } else {
       // could not find location, won't allow editing
       m_resolved_value.SetContext(Value::eContextTypeInvalid, nullptr);
     }
   }
+  
   return m_error.Success();
 }
 
+void ValueObjectVariable::DoUpdateChildrenAddressType(ValueObject &valobj) {
+  Value::ValueType value_type = valobj.GetValue().GetValueType();
+  ExecutionContext exe_ctx(GetExecutionContextRef());
+  Process *process = exe_ctx.GetProcessPtr();
+  const bool process_is_alive = process && process->IsAlive();
+  const uint32_t type_info = valobj.GetCompilerType().GetTypeInfo();
+  const bool is_pointer_or_ref =
+      (type_info & (lldb::eTypeIsPointer | lldb::eTypeIsReference)) != 0;
+
+  switch (value_type) {
+  case Value::eValueTypeFileAddress:
+    // If this type is a pointer, then its children will be considered load
+    // addresses if the pointer or reference is dereferenced, but only if
+    // the process is alive.
+    //
+    // There could be global variables like in the following code:
+    // struct LinkedListNode { Foo* foo; LinkedListNode* next; };
+    // Foo g_foo1;
+    // Foo g_foo2;
+    // LinkedListNode g_second_node = { &g_foo2, NULL };
+    // LinkedListNode g_first_node = { &g_foo1, &g_second_node };
+    //
+    // When we aren't running, we should be able to look at these variables
+    // using the "target variable" command. Children of the "g_first_node"
+    // always will be of the same address type as the parent. But children
+    // of the "next" member of LinkedListNode will become load addresses if
+    // we have a live process, or remain a file address if it was a file
+    // address.
+    if (process_is_alive && is_pointer_or_ref)
+      valobj.SetAddressTypeOfChildren(eAddressTypeLoad);
+    else
+      valobj.SetAddressTypeOfChildren(eAddressTypeFile);
+    break;
+  case Value::eValueTypeHostAddress:
+    // Same as above for load addresses, except children of pointer or refs
+    // are always load addresses. Host addresses are used to store freeze
+    // dried variables. If this type is a struct, the entire struct
+    // contents will be copied into the heap of the
+    // LLDB process, but we do not currently follow any pointers.
+    if (is_pointer_or_ref)
+      valobj.SetAddressTypeOfChildren(eAddressTypeLoad);
+    else
+      valobj.SetAddressTypeOfChildren(eAddressTypeHost);
+    break;
+  case Value::eValueTypeLoadAddress:
+  case Value::eValueTypeScalar:
+  case Value::eValueTypeVector:
+    valobj.SetAddressTypeOfChildren(eAddressTypeLoad);
+    break;
+  }
+}
+
+
+
 bool ValueObjectVariable::IsInScope() {
   const ExecutionContextRef &exe_ctx_ref = GetExecutionContextRef();
   if (exe_ctx_ref.HasFrameRef()) {
     ExecutionContext exe_ctx(exe_ctx_ref);
     StackFrame *frame = exe_ctx.GetFramePtr();
     if (frame) {
       return m_variable_sp->IsInScope(frame);
     } else {