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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
