[Lldb-commits] [PATCH] D69273: ValueObject: Fix a crash related to children address type computation

Mon Jan 27 02:27:23 PST 2020

labath added a comment.

Thanks for the update Jim. I'm putting some of my thoughts inline.

In D69273#1840029 <https://reviews.llvm.org/D69273#1840029>, @jingham wrote:

> Sorry, I've been busy with other things.
>
> In answer to Pavel's direct question "What did we do about the swift crashes" the current answer is "we backed out the patch from the swift-lldb sources".  But that's clearly not the good answer...
>
> The thing that's surprising about the crash in the swift testsuite is that it happens because, in the process of building the dynamic ValueObject of the synthetic child for the expression result ValueObject for a simple swift expression, with this code in place one of the Values we are using that is in fact a load address type gets mislabeled as a host address, and then we crash accessing it in our address space.  IIUC, that's of the opposite of what this patch was trying to do...

Yes, that is the opposite of what this is trying to do, though I can see how this could trigger something like that when synthetic children come into the game. The logic `children_type = am_i_pointer ? load : host` is only correct for real children, and if this somehow fires on synthetic children of a non-pointer ValueObject, but the children are actually obtained via dereferencing some pointers, then they might get mislabelled as host pointers.

However, I don't think things could be this simple, as otherwise this would also reproduce on the synthetic children of std::map for instance. So there must be something more here...

> BTW, I also think this patch is formally wrong for const result objects, because once the stop ID has moved on, const result object values should never be converted back to load address types.  They are supposed to represent the state at the time of the capture, so checking the state of the process after that time can't be right.  But that wasn't the failure we were seeing in the Swift testsuite.

I get the "state at the time of the capture" argument, but it's not clear to me what would be the behavior without this patch(?) IIUC, we currently don't have any logic which would "freeze-dry" the pointed-to values (and it's tricky to do so, because those values can contain other pointers, etc.). So, this seems correct at least in the sense that those pointer values will get the correct address class (assuming the synthetic children issue above is sorted out) and we won't crash while trying to dereference them.

I also think the "state at the time of capture" thing needs to be more nuanced. For instance, if I had a variable like `int *ptr = &some_int;`, I wouldn't expect that `p ptr` followed by a `p *$1` would show me the old/stale value of `some_int`. That would certainly be the case if the pointer is part of an "implementation detail" of something (like the `const char *` inside `std::string`), but it seems like there are at least some cases where it would be reasonable to follow the pointer values into live memory.

> ConstResult objects are still implicated in this crash, because "expr var" crashes but "frame var" for this swift variable works.  The difference between the two cases is that in the succeeding case the root ValueObject is a ValueObjectVariable, and in the crashing case a ValueObjectConstResult.   But it doesn't have to do with the const result getting into an inconsistent state because it updates itself when it shouldn't.  It seems like there's just something in the logic of UpdateChildrenAddressType that is wrong for ValueObjectConstResult.  But it seems to take a fairly complex chain of values - ConstResult->ConstResultChild->SyntheticValue->DynamicValue to trigger the crash, and the crash is actually in getting the backing data for a ValueObjectDynamicValue...
> 
> It will take me some more head scratching to figure out why this is going wrong but I'm currently in the process of hastening my eventual balding.

Lets hope it doesn't come to that. :) Let me know if there's anything I can do to help.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69273