[Lldb-commits] [PATCH] D83450: Delegate UpdateChildrenPointerType to the Root ValueObject

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83450/new/

https://reviews.llvm.org/D83450