[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

Wed Oct 27 17:33:40 PDT 2021

jingham added a comment.

In D105470#2868858 <https://reviews.llvm.org/D105470#2868858>, @werat wrote:

> Thanks for the explanation! But at this point I feel I'm a bit confused about how it all _supposed_ to work in the current design :)

Apparently I lost track of this review...

> If I understand correctly, there are four "types" of values from the user (API) perspective:
>
> 1. `ExpressionResult` -- value returned by `SBFrame::EvaluateExpression()`

This is the one that tis expected to be "constant".  After all, expression results aren't necessarily anything in the target, they could be an int returned by a function call that is only in the expression result.
Moreover, this ValueObject represents the "results of an expression".  It really doesn't make sense to change that.  And even if the expression result is just some local variable, we want users to be able to refer back to the results of expressions even when they have left the frame where the expression was evaluated.  So these ones need to be "frozen" values.

Note there is a bunch of code in the ValueObjectConstResult that tracks the "live address" if the expression result was something in memory.  That's so that you can do things like *$1 and have then do the right thing.

And, there's another complexity which is that the ValueObjectConstResult class got reused for a bunch of things that don't have this restriction, so the internal policy is pretty confusing.  I'm trying to find some time to clean this up a bit in the near term.

> 2. `ExpressionPersistentVariable` -- value created by the expression via `auto $name = ...` syntax. Can be obtained by `SBFrame::FindValue("$name", lldb::eValueTypeConstResult)`.

These ones are supposed to act as though the user had declared an exported global variable of this name and type.  So it should be modifiable.  As I said above, it's really confusing that this is a ConstResult, which it clearly isn't...

> 3. "Const value" -- value created by `SBTarget::CreateValueFromData()` or `SBTarget::CreateValueFromAddress`

Creating a value from Data is making a ValueObject that only makes sense to lldb.  This is data in lldb's memory.  But these entities are really for use by other ValueObject entities, and they would know what the right policy for recalculation should be.

CreateValueFromAddress is like a the Expression Persistent Variables.  It refers to an address in the target, and isn't associated with a frame, so it should just reflect what's in that memory location, and writing makes sense as well.

> 4. "Variable reference" -- value returned by `SBFrame::FindVariable()`

These ones are pretty self evident.  We do allow you to modify the state of locals, so that should be hooked up.  If the underlying variable is a stack local, you it doesn't make sense to update or change them once their frame has been pushed off the stack.  But if the variable is a global, then it should be valid till the library it lives in gets unloaded.  These all have update points saying what the criteria for their validity are.

> For which of these value the following test is supposed to work?
>
>   struct Foo { int x; };
>   Foo* f1 = { .x = 1}
>   Foo* f2 = { .x = 2}  # pseudo-C for simplicity
>   
>   f1_ref = ...  # Get a value that holds the value of `f1` using one of the four methods described above
>   print(f1_ref.GetChild(0))  # '1'
>   f1_ref.SetValueFromCString(frame.FindVariable('f2').value)
>   print(f1_ref.GetChild(0))  # '2'
>
> My experiments show that it works for "variable references" and "const values" created by 
> `CreateValueFromAddress` (but _not_ `CreateValueFromData`).
> UPD: it seems values created `CreateValueFromAddress` actually behave like "variable references". Modifying their value will modify the underlying data directly.
>
> If I understand your comment correctly, you're saying it should work only for `ExpressionPersistentVariable` values (#2). Is that right?

Sorry if I wasn't clear.  From the comments above, you should be able to update everything but expression result variables.   Updating the From Data ones is a little less clear, but they are mostly for internal uses - like for handing out Synthetic Children.

> I don't have the full picture about the internal implementation and all the use cases, but as a user I would expect it to work for at least #2, #3 and #4. Afaik there's no API to fully distinguish between these kinds of values, so I find it confusing why `SBValue::SetData()` would be allowed for some values and not allowed for others. If I can create a value using `CreateValueFromData` and then there's a method `SetValueFromCString`, then I don't see why it should not be allowed (apart from implementation complexity/consistency reasons).
>
> What do you think? How should we proceed with this?

Note, however, that there is one more detail of importance in this system.  For variables that represent valid entities in the target, every time you stop we should be able to ask whether the value has changed.  We don't try to record every value in full depth, so if on stop A you looked at foo, foo->bar & foo->bar.baz, but not foo->other_struct, then we only report "IsChanged" for foo, foo->bar & foo->bar.baz.  But that means you can't willy-nilly throw away the children when you stop or you won't be able to reconstruct the previous value.

Anyway, except for the Data version, which is more an implementation for some other presentation, we seem to agree on what should happen:

ValueObjectVariables should refresh themselves on every stop until they are no longer valid.  And they need to be able to report "IsChanged".

ExpressionPersistentVariables and FromAddress ValueObjects should work like ValueObjectVariables that reflect globals, except we made up the globals.  The former get memory allocations we made, so they won't get externally changed unless you hand a pointer to them to the target somehow.  The address ones are just some random address, so there's no guarantee about their behavior when the target is running.  I don't actually remember whether IsChanged is hooked up for them, I don't think that gets shown anywhere.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470