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

Fri Jul 9 09:39:54 PDT 2021

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