[Lldb-commits] [PATCH] D98370: [lldb] Fix SBValue::Persist() for constant values

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D98370#2710058 <https://reviews.llvm.org/D98370#2710058>, @werat wrote:

> In D98370#2709828 <https://reviews.llvm.org/D98370#2709828>, @jingham wrote:
>
>> Be careful here.  There are really two kinds of persistent variables: "expression results" and variables created in the expression parser.  The former are by design constant.  The idea is that you can use them to checkpoint the state of a value and refer to it later.  You can use their values in expressions, but they aren't supposed to be modifiable.  Those are the ones called $NUM.
>>
>> The ones you make in an expression, like:
>>
>> (lldb) expr int $myVar = 20
>>
>> on the other hand are modifiable.
>
> What about the values created via `SBValue::Persist()` then? They have names like `$NUM`, but they can be either of the two you mentioned and more (values created via `CreateValueFromData` or values acquired via `FindVariable`):

I am guessing those names are chosen just because that was a handy routine for auto-generating a variable name.  It doesn't follow with our general naming conventions, we should probably distinguish them from result variables by giving them some other name (like $P0...) or something.

>   v = frame.EvaluateExpression("...")  # `v` is "$0" -- it's an "expression result" by your classification
>   vp = v.Persist()  # `vp` is "$1" -- I assume it should point the same value as "$0" and be non-modifiable
>   
>   --
>   
>   v = frame.EvaluateExpression("$myVar = 20")  # `v` is "$0" -- it's an "expression result" by your classification
>   vp = v.Persist()  # `vp` is "$1" -- I assume it should point the same value as "$0" and be non-modifiable
>   
>   myVar = frame.EvaluateExpression("$myVar")  # now `myVar` is "$myVar" -- it's a "variables created in the expression parser"
>   myVarP= myVar.Persist()  # `myVarP` is "$2" -- ?? Should it point the same value as `myVar` and be modifiable?
>   
>   --
>   
>   v = target.CreateValueFromData(...)
>   vp = v.Persist()  # `vp` is "$0" -- ?? Should it be modifiable? Should it point to the original `v`?
>
>
>
> ------
>
> I think at this point I should explain the problem I'm solving from the beginning.
>
> I have built an expression evaluator for LLDB (and based on LLDB) -- https://github.com/google/lldb-eval. It's much faster than LLDB's `EvaluateExpression`, but has some limitations -- it doesn't support user-defined function calls, doesn't allows to evaluate arbitrary C++ code, etc.
> It is used in Stadia for Visual Studio debugger (https://github.com/googlestadia/vsi-lldb) complementing LLDB. Expressions that are not supported by `lldb-eval` are evaluated by `LLDB`, thus from user perspective everything "just works". In some situations we need to maintain some "state", which can be read/written by multiple consecutive expressions. Simplified example:
>
>   EvaluateExpression("$index += 1");
>   v1 = EvaluateExpression("array[$index]")
>   
>   EvaluateExpression("$index += 1");
>   v2 = EvaluateExpression("array[$index]")
>
> In case of `LLDB` we create `$index` via something like `expr $index = 0` and then the expressions can modify it. In `lldb-eval`, however, we don't want to use LLDB's persistent variables, so the state is represented via regular map of `SBValue` objects (https://werat.dev/blog/blazing-fast-expression-evaluation-for-c-in-lldb/#maintaning-state). Basically the API is something like this:
>
>   state = {"index": some_sbvalue}
>   
>   EvaluateExpression("index += 1", state);
>   v1 = EvaluateExpression("array[index]", state)
>   
>   EvaluateExpression("index += 1", state);
>   v2 = EvaluateExpression("array[index]", state)
>   
>   some_sbvalue.GetValue()  # == 2
>
> But as I mentioned before, our debugger shell is "smart" and can fallback to `LLDB` if `lldb-eval` fails to evaluate the expression. If there was a state involved, it needs to be converted to LLDB's persistent variables. Right now we workaround this issues by always allocating "state" values in LLDB (https://github.com/googlestadia/vsi-lldb/blob/0fcbe30a498fac70230efdb815125ee5f6f087bb/YetiVSI/DebugEngine/NatvisEngine/NatvisExpressionEvaluator.cs#L315) -- this way we don't need to convert it back. However it would be better to convert only if needed and for that I was hoping to use `SBValue::Persist()`.
>
> The example below doesn't work, but that's what I would like to fix (code is simplified):
>
>   // We can assume here the result is created via something like `CreateValueFromData()`
>   // 
>   lldb::SBValue counter = lldb_eval::EvaluateExpression("10");
>   
>   lldb_eval::ContextVariableList vars = { {"counter", &counter } };
>   
>   while (counter.GetValueAsUnsigned() > 5) {
>     lldb_eval::EvaluateExpression("--counter", vars);
>   }
>   
>   lldb::SBValue p = counter.Persist();  // Assume `p.GetName()` is "$1"
>   lldb::SBValue v1 = lldb::EvaluateExpression("$1");    // v1.GetValue() == 5
>   lldb::SBValue v2 = lldb::EvaluateExpression("--$1");  // v2.GetValue() == 4
>
> Sorry for the long message, I hope it's more clear what I'm trying to do here :)

NP, good to know the context...

>From playing around with the current state of things, it looks like the Persisted version of an SBValue should behave exactly like it's underlying SBValue, the only thing Persist is supposed to do is give it a name that can be referred to in expressions.  For instance, if you get an SBValue from SBFrame.FindVariable, call Persist on it, and then call SetValueFromCString on the persisted version, or use it in an expression and it actually changes the value of the local variable, just as it would if you called that on the FrameVariable one.

So it looks to me like you can't just key off of whether the underlying m_frozen_sp is a ValueObjectConstResult.  That's just the object container that mostly always gets used to hold the frozen value.  Instead we should always set the copy back policies based on the kind of SBValue.  That happens automatically for expression & expression result variables because the system gets to make them the right way when it sets them up.  From what I can see, inheriting the original SBValue's behavior when calling Persist on a FrameVariable SBValue also sets it up to track the underlying value & be able to change it/change with it.

Have you implemented the part where instead of using an expression result for your extra values you make one with SBTarget.CreateValueFromData?  Do SBValues made that way also fail to change value like the expression result ones do?  It doesn't seem to me like you actually care whether expression results are changeable - you don't actually plan to use those right?  You just want to make sure that Persist-ed CreateValueFromData SBValues are changeable (and get materialized properly when used).  So I'd start with seeing if you can change SBValues made that way.  If they just work, then Party!

Otherwise, Persist is going to have to have some way to tell these SBValues apart from Expression Result Variables, so that it can fix them up appropriately.  Maybe we should be stricter about the rule should be that $<NN> variables are always considered expression result.  We'd probably want to add a parallel naming queue for Persisted variables to keep them distinct?  And then you could do pretty much what you are doing here, but only to non-result variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98370