jingham added a comment.
In D105470#2866731 <https://reviews.llvm.org/D105470#2866731>, @werat wrote:
> Thanks for the prompt review!
>
>> This change is wrong for ValueObjectConstResult's. The original point of
>> ValueObjectConstResult's was to store the results of expressions so that,
>> even if the process is not at the original stop point, you could still check
>> the value. It should have the value it had at the time the expression was
>> evaluated. Updating the value is exactly what you don't want to do. So I
>> think you need to make sure that we can't change their value after they are
>> created.
>
> Looking at the current state of things and the available APIs, I don't think
> this principle holds anymore. `ValueObjectConstResult` is also used for
> values created via `SBTarget::CreateValueFromData`. As a user I don't see any
> reason why I shouldn't be allowed to modify the value of an object I created
> myself earlier. I would argue the same for the results of the expression
> evaluation. Why are some values different from others?
Expression results store the results of an expression evaluation. As such, it
doesn't really make sense to change their values. Moreover, I'd argue it is
confusing to update them. If I get the value of an object at time A and the
try to fetch a child of it at time B, the value I would get is incoherent with
the state of the object when I fetched it. So I don't think those operations
make much sense for ExpressionResults, and allowing them to change when you
look at them again defeats their purpose, which is to store the results of the
expression at the time it was evaluated.
Convenience variables made in the expression parser have a whole different
meaning. They are made by and their values controlled by the user of the
expression parser. You should be able to change them at will. And since you
might have handed them off to the program (for instance storing a convenience
variable held object in some collection managed by the program, they need to be
updated when program state changes.
>> Note, ExpressionResult variables are different from
>> ExpressionPersistentVariables. The former should not be updated, and after
>> the process moves on any children that weren't gathered become "unknown".
>> But ExpressionPersistentVariables should be able to be assigned to, and when
>> the reference target object, it should update them live. .
>
> Please, correct me if I'm wrong. In my example `ExpressionResult` is a value
> returned by `EvaluateExpression` (i.e. `b`) and
> `ExpressionPersistentVariable` is variable created by the expression
> evaluator (i.e. `$b_0`), right? As far as I can tell, they're both
> `ValueObjectConstResult` and both have the problem outlined in this patch:
>
> frame0.EvaluateExpression("auto $b_0 = b1")
> b = frame0.FindValue("$b_0", lldb.eValueTypeConstResult)
> ...
> # Same problem, updating the value of `b` doesn't invalidate children.
You are right in everything bug that ExpressionResult variables should not be
updated when program state changes.
> > The other thing to check about this patch is whether it defeats detecting
> changed values in child elements.
>
> This patch invalidates children only in `ValueObject::SetNeedsUpdate()`
> which is called only from `SetData/SetValueFromCString` as far as I
> understand. If you have a `ValueObjectVariable` which tracks some variable
> that can be modified by the process, then it will continue to work fine.
> `TestValueVarUpdate.py` passes successfully, but I didn't look to0 closely
> yet whether it actually tests the scenario you described.
The fact that SetNeedsUpdate only gets called from SetData/SetValueFromCString
seems to me to be just an accident, nothing enforces not using these when the
program state changes, for instance.
> ---
>
> Looking at this from the user perspective, I would prefer to be able to
> update any value, regardless of which API it came from. In my use case I rely
> on this heavily --
> https://werat.dev/blog/blazing-fast-expression-evaluation-for-c-in-lldb/#maintaning-state.
> I could potentially live with the results of `EvaluateExpression` being
> immutable, but being able to modify values created by `CreateValueFromData`
> or persistent values like `$b_0` is necessary for me.
I'm less certain about updating "CreateValueFromData" variables. Those tend to
be used by Synthetic Child Providers, for instance when you've found an element
of an NSDictionary, you use CreateValueFromData to produce the ValueObject that
represents it. But in these cases, the data formatters generally recreate
these objects rather than update them. If they were going to update them using
SetValueFromCString and the like, you would then need to preserve "IsChanged"
information in that process.
But for certain expression result variables should be changeable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105470/new/
https://reviews.llvm.org/D105470
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
