Page MenuHomePhabricator

ValueObject: Fix a crash related to children address type computation
ClosedPublic

Authored by labath on Oct 21 2019, 10:55 AM.

Details

Summary

This patch fixes a crash encountered when debugging optimized code. If some
variable has been completely optimized out, but it's value is nonetheless known,
the compiler can replace it with a DWARF expression computing its value. The
evaluating these expressions results in a eValueTypeHostAddress Value object, as
it's contents are computed into an lldb buffer. However, any value that is
obtained by dereferencing pointers in this object should no longer have the
"host" address type.

Lldb had code to account for this, but it was only present in the
ValueObjectVariable class. This wasn't enough when the object being described
was a struct, as then the object holding the actual pointer was a
ValueObjectChild. This caused lldb to dereference the contained pointer in the
context of the host process and crash.

Though I am not an expert on ValueObjects, it seems to me that this children
address type logic should apply to all types of objects (and indeed, applying
applying the same logic to ValueObjectChild fixes the crash). Therefore, I move
this code to the base class, and arrange it to be run everytime the value is
updated.

The test case is a reduced and simplified version of the original debug info
triggering the crash. Originally we were dealing with a local variable, but as
these require a running process to display, I changed it to use a global one
instead.

Diff Detail

Event Timeline

labath created this revision.Oct 21 2019, 10:55 AM
clayborg accepted this revision.Oct 21 2019, 11:05 AM

Looks good!

This revision is now accepted and ready to land.Oct 21 2019, 11:05 AM

Except for the comment comment this looks fine.

I think the Host -> Load address code isn't quite right, though it looks like that's not your doing.

The main reason why you would copy a ValueObject into Host memory it is to freeze-dry it as a ConstResult. Since that is supposed to represent the object at that point in time, it should only be valid to you ask the ValueObject to produce its children if the process is at the same StopID as when the ValueObject was made. Then the ValueObject system should fetch that more data and include it in the freeze-dried object. But if the StopID has moved on, you should just give an error: "Can't travel back in time to fetch that extra data".

At the ValueObject level, however, we only know how the data is stored (in Host or Process) and not why. So it's harder to get this behavior right.

As I said, however, I don't think this planned design was ever carried out successfully, so I don't think this will have broken anything.

source/Core/ValueObject.cpp
162–163 ↗(On Diff #225921)

"remain a file address if it was a file address."?

davide added a subscriber: davide.Oct 24 2019, 12:30 PM
davide removed a subscriber: davide.
davide added a subscriber: davide.
This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2019, 10:53 AM

In Swift you have to ask the runtime for most of the layout details of objects so getting a const result object, stepping, then asking it to reevaluate itself would lead to passing the runtime incorrect data and potentially undoing a correct type decision that you had made when you fetched the result on the first stop. That's what it sounded like from Davide's description, but I haven't looked at it more deeply yet.

All the ValueObjects have a ModID, so you for const objects you should always check that before updating. I think the problem is this just never happened before because the code was not in the ValueObjectConst inheritance path. By hoisting that code into ValueObject, now it was, but it was not correct to do that without consulting the modID. Note that for cases like this we check the last user stop, not the actual last stop. We really can't avoid running expressions to get dynamic types and some other jobs, so we really want to consider had run expressions as not changing the essential state of the program. This hasn't caused problems in practice, and if it were to become a problem we could classify expressions as state changing or not state changing, and move the user stop ID forward for the former as well...

labath added a comment.Nov 4 2019, 9:36 AM

In Swift you have to ask the runtime for most of the layout details of objects so getting a const result object, stepping, then asking it to reevaluate itself would lead to passing the runtime incorrect data and potentially undoing a correct type decision that you had made when you fetched the result on the first stop. That's what it sounded like from Davide's description, but I haven't looked at it more deeply yet.

All the ValueObjects have a ModID, so you for const objects you should always check that before updating. I think the problem is this just never happened before because the code was not in the ValueObjectConst inheritance path. By hoisting that code into ValueObject, now it was, but it was not correct to do that without consulting the modID. Note that for cases like this we check the last user stop, not the actual last stop. We really can't avoid running expressions to get dynamic types and some other jobs, so we really want to consider had run expressions as not changing the essential state of the program. This hasn't caused problems in practice, and if it were to become a problem we could classify expressions as state changing or not state changing, and move the user stop ID forward for the former as well...

Hmm.. that's interesting. But what is the thing that's causing the layout to be "reevaluated"? Is it the GetCompilerType() call on line 141? Should we maybe just have ValueObjectConstResult override GetCompilerType so that it "freeze-dries" the type too ? From your description, it sounds like that's exactly the semantics we want here...

labath added a comment.Dec 2 2019, 7:24 AM

This seems to have fizzled out without any sort of a conclusion.. Did you guys do anything about the crashes you were seeing on the swift side?

davide added a comment.Dec 2 2019, 7:33 AM

We've been off all the past week. I'll circle back with Jim about this once I get to the office.

We've been off all the past week. I'll circle back with Jim about this once I get to the office.

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

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.

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. Hopefully, once I've figured that out I can get a fix and if I'm lucky a C/C++ based test case that shows the same error.

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

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.