This is an archive of the discontinued LLVM Phabricator instance.

Fix "ninja check-lldb" crash in IRExecutionUnit.cpp
ClosedPublic

Authored by ted on Mar 3 2016, 10:58 AM.

Details

Summary

From Adrian McCarthy:

"Running ninja check-lldb now has one crash in a Python process, due to deferencing a null pointer in IRExecutionUnit.cpp: candidate_sc.symbol is null, which leads to a call with a null this pointer."

Diff Detail

Event Timeline

ted updated this revision to Diff 49759.Mar 3 2016, 10:58 AM
ted retitled this revision from to Fix "ninja check-lldb" crash in IRExecutionUnit.cpp.
ted updated this object.
ted added reviewers: spyffe, zturner, amccarth.
ted added a subscriber: lldb-commits.
amccarth accepted this revision.Mar 8 2016, 10:26 AM
amccarth edited edge metadata.

Thanks for the fix.

Right now I'm trying to figure out why the test framework didn't detect the crash and reported that all the tests passed.

source/Expression/IRExecutionUnit.cpp
801

I'm OK with this, but I believe the LLDB style is to reverse the logic and use an early-out (a continue in this case) rather than increasing the depth of the rest of the code.

This revision is now accepted and ready to land.Mar 8 2016, 10:26 AM
spyffe accepted this revision.Mar 8 2016, 10:36 AM
spyffe edited edge metadata.

Looks good to me. Thanks for the fix!

source/Expression/IRExecutionUnit.cpp
805–812

Ah yes, I see what's being fixed here. This looks fine. I like Adrian's recommended early-out policy but this isn't a hard-and-fast rule. It seems like doing the logic the way you're doing it is less invasive, so I'm fine with doing it this way for now.

I don't think this is right. It is possible to have a sc.symbol be nullptr, but sc.function be valid. So the check for sc.symbol will reject the valid information in the function. Note, the code in the function is also wrong, since it only gets the address from the symbol.

Greg suggests adding something to SymbolContext to get the start address of the function that handles this possibility. GetAddressRange sort of does this, but only if the SymbolContext doesn't have a block or line entry.

ted updated this revision to Diff 50074.Mar 8 2016, 2:46 PM
ted edited edge metadata.

Updated to use early-out as requested

ted added a comment.Mar 8 2016, 2:46 PM

The change is to guard against the case where candidate_sc.symbol is nullptr.

candidate_sc.function is only used when load_address != LLDB_INVALID_ADDRESS, but load_address is set on line 802:

load_address = candidate_sc.symbol->ResolveCallableAddress(*target);

so candidate_sc.symbol must be valid.

The purpose of the function is to get the address of a symbol, so I don't think we care about candidate_sc.function when candidate_sc.symbol is nullptr.

ted added a subscriber: ted.EditedMar 9 2016, 11:37 AM

So you'd like to see this function get the address of a function that it
finds in either symbols or debug info?

Which should it prioritize when we have both?

ted updated this revision to Diff 50189.Mar 9 2016, 1:01 PM

Updated to address Jim's comments.

That seems fine to me. Thanks for putting up with the niggling...

ted closed this revision.Mar 9 2016, 2:10 PM