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

Fri Jul 9 03:51:09 PDT 2021

werat added a comment.

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?

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

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

---

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470