https://reviews.llvm.org/D69273

It looks like this change is causing problems with swift. I was talking a little bit with Davide about this and it seems like it wasn't obvious how this was designed to work. So here's what this was intended to do (apologies if this is at too basic a level and the issue was something deeper I missed, but anyway this might get us started...)

The lldb ValueObject system supports two fairly different kinds of values, live and frozen.

The classic example of a live ValueObject is ValueObjectVariable. That ValueObject is backed by an entity in the target, and knows when that entity is valid and not. So it can always try to do "UpdateValueIfNeeded" and that will always return good values. However, there's on complication with this, which is that we also want ValueObjectVariable to be able to answer "IsChanged". That's so in a UI you can mark values that change over a step in red, which is very helpful for following along in a debugging session. So you have to copy the values into host memory, in order to have something to compare against when you stop again. That's why there's this slightly complex dance between host and target memory for the live ValueObjects.

The example of a frozen object is the ValueObjectConstResult that is returned from expression evaluation. That value is fixed to a StopID, so the backing entity is only known to be good at that stop id. This is implemented by copying the value into Host memory and fetching it from there when requested.

The use case for this is for people like me who have a bad memory. So I can stop somewhere and do:

(lldb) expr foo
struct baz $1 = {
  bar = 20
}

Then later on when I forget what foo.bar was at that time, I can do:

(lldb) expr $1.bar
bar = 20

At a first approximation, this leads to the statement that ConstValues should fetch what they fetch when made, and then not offer any information that wasn't gathered when the variable was fetched, and you certainly don't ever want these values to be updated.

A little complication arises because I might do:

(lldb) expr foo_which_has_a_pointer
$1 = ...
(lldb) expr *$1->the_pointer

If the StopID is the same between the first and second evaluation, then you should follow the pointer into target memory and fetch the value. But if the StopID has changed, then trying to dereference a pointer should be an error. After all, you are now accessing an incoherent object, and if you try to do anything fancier with it than just print some memory (like asking the Swift Language Runtime what this value happens to be) you are very likely to get into trouble.

So it's clear we need two different behaviors w.r.t. how we treat live or frozen values. Pavel's change was addressing a failure in ValueObjectChild, and the solution was to move the ValueObjectVariable behavior up to the ValueObject level. But then that means that ValueObjectConstResults are no longer obeying the ConstResult rules.

But it seems like the problem really is that we have only one ValueObjectChild class, but child value objects can either be live or frozen, depending on the nature of their Root ValueObject. And this is made a little more complicated by the fact that frozen values only freeze when the stop ID changes.

Jim

Thanks for the explanation. + Pavel & Adrian.

(Just writing to say that tomorrow is a public holiday in most of Europe, so I wont be able to meaningfully reply to this until monday/tuesday. But if, in the mean time, you want to revert this, or just limit the scope of that patch somehow, then that's fine with me.)

I don't see this as an urgent revert. We just cherry-picked the change
in our branches and realized that it was creating some problems, so we
raised the issue and started the discussion.
Definitely it can wait a day or two.

Thanks,

Thanks for the writeup Jim. I haven't managed to dive into the source code yet, but the thing that's not clear to me from this otherwise detailed an understandable explanation is what is the interaction between this ConstResult stuff and the above patch.

Superficially, it doesn't sound like that patch should do anything bad here. As the ValueObjectConstResult's data is located in host memory, the patch will compute that its pointer children will be of "load address" type, which sounds like precisely what's needed here.

Of course, under the surface, there are plenty of ways this can go wrong, but precisely because of that, it's hard to say what's the right thing to do. Is it that ValueObjectConstResult uses the "address type" field to implement the "are the children valid at this stop ID" logic, and so this patch interferes with that? What's exactly the nature of the crash/misbehavior you were witnessing?

pl

Sorry, my brain is not working this morning, I answered your question in the review comments…

Jim

NP, maybe let's continue the discussion there? I find it useful to have the actual code change around..