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

[Jim Ingham via lldb-commits]

> On Jul 10, 2021, at 6:18 AM, Andy Yankovsky via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> werat added a comment.
> 
> 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 :)
> 
> If I understand correctly, there are four "types" of values from the user (API) perspective:
> 
> 1. `ExpressionResult` -- value returned by `SBFrame::EvaluateExpression()`
> 2. `ExpressionPersistentVariable` -- value created by the expression via `auto $name = ...` syntax. Can be obtained by `SBFrame::FindValue("$name", lldb::eValueTypeConstResult)`.
> 3. "Const value" -- value created by `SBTarget::CreateValueFromData()` or `SBTarget::CreateValueFromAddress`
> 4. "Variable reference" -- value returned by `SBFrame::FindVariable()`
> 
> 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`).
> If I understand your comment correctly, you're saying it should work only for `ExpressionPersistentVariable` values (#2). Is that right?

No, that is not what I meant to say.  

Of the 4 cases above (there's also Register SBValues BTW) I was only arguing that #1 should not be settable.  After all, you might have done:

expr 5 + 5
(int) $0 = 10

I'm not sure what $0 = 11 would mean.  But beyond that, they are useful for freezing values for later consultation, so you want to make sure that once the process has changed state
they won't try to update themselves, accidentally or on purpose.

ValueObjectVariable objects should definitely be settable.  That's how most GUI's would implement choosing a child from a Variables display and changing its value, so setting them is
actually a pretty common operation.

Persistent variables also need to be settable, since they are often used as scratch counters and so forth, and wouldn't be of nearly as much use if they weren't.

CreateValueFromAddress values relate to things in memory (and actually track changes in the memory underlying them when the process changes state).  Since they are the way
that you can do casts to get a structured access to raw memory at the SB API layer, it seems reasonable that they should also be settable.

And objects made from CreateValueFromData are mostly used for artificial tasks like making backing ValueObjects for pieces of complex data structures like dictionaries, maps, etc
then having them be settable is also appropriate.

The other concern I have is that all these types of values that are used for "local variable display" like the Variable, FromAddress & FromData ones need to support IsChanged.  
That's why it struck me as wrong that a general sounding method like SetNeedsUpdating was throwing away the already made child values.  To do a good job of presenting "is changed" 
results you need to know both what children had been fetched (in a GUI this corresponds to what values the user has seen, so those are the ones you're supposed to check) 
and their old values.  So discarding the children except in cases where you know you are going to reset them completely is likely to get in the way of that.

I do agree the current implementation has gotten a little confused, but I think what we want to have happen is fairly clear, as evinced by the fact that we actually agreed on
the correct behaviors...

Jim


> 
> 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?
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D105470/new/
> 
> https://reviews.llvm.org/D105470
>