Page MenuHomePhabricator

[analyzer] Fix a crash in RVO from within blocks.
ClosedPublic

Authored by NoQ on May 3 2019, 6:40 PM.

Details

Summary

While blocks are an Apple extension to C, i'm adding everybody because my mistake that i'm fixing here may hurt us more when we add more kinds of location contexts, so we shouldn't be making it anywhere.

In the attached test case, getA() returns an object by value, and therefore requires a construction context. Its construction context is that of a value that's immediately* returned (by value, without any sort of conversion), so copy elision (namely, RVO) applies. This means that we unwind the stack of LocationContexts in order to see where is the current callee called from. This way we obtain the construction context for the call site, and from that we can figure out what object is constructed. In this case it's going to be the first-and-only argument of use(). This is all good.

In this case RVO is applied to a return value of an anonymous block that's declared only to be immediately called and discarded. The Static Analyzer models block calls by putting two location contexts on the stack: a BlockInvocationContext followed by the actual StackFrameContext. I don't really know why it does that :) but that's not important, because if we introduce more kinds of location contexts, it'll look similarly anyway.

Therefore the correct idiom for obtaining the parent stack frame is to first obtain the parent, and then don't forget to obtain its stack frame, which is not necessarily the parent itself. This is the mistake that i made in my RVO code that i'm fixing here.

The code was crashing because it was looking up the call site in a CFGStmtMap for the wrong CFG (obtained from a wrong location context). This was happening in CallEvent::getCalleeStackFrame().

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.May 3 2019, 6:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 6:40 PM
Charusso accepted this revision.May 4 2019, 12:04 AM

I think the entire LocationContext stuff is a huge design issue, and you used its functionality without any hack. If you would rename the getStackFrame to getNextStackFrame or something, it is clear it is not a getter and traversing every frame until the top-frame.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
203 ↗(On Diff #198121)

The smallest the scope between the definition and its usage, the better the code. Could you put the new code immediately before the return statement?

This revision is now accepted and ready to land.May 4 2019, 12:04 AM
a_sidorin accepted this revision.May 4 2019, 12:21 PM
NoQ updated this revision to Diff 198342.May 6 2019, 2:49 PM

No, this is all wrong. We shouldn't unwrap to the parent stack frame here. We should end up exactly in the location context in which the call has happened. But we should still unwrap the block invocation context, as an exception, because it's just a weird location context that goes before the stack frame but still has its corresponding AnalysisDeclContext tied to the *callee* stack frame.

These block invocation contexts are used, like, once, to help modeling captures. I should probably try to get rid of them entirely.

NoQ updated this revision to Diff 198535.May 7 2019, 2:45 PM

Remove unused declaration from the test.

dcoughlin accepted this revision.May 7 2019, 3:01 PM

Looks good to me.

This revision was automatically updated to reflect the committed changes.