This is an archive of the discontinued LLVM Phabricator instance.

Push a function scope when parsing function bodies without a declaration
ClosedPublic

Authored by rnk on Mar 1 2018, 4:26 PM.

Details

Summary

This is PR36536.

There are a few ways to reach Sema::ActOnStartOfFunctionDef with a null
Decl. Currently, the parser continues on to attempt to parse the
statements in the function body without pushing a function scope or
declaration context. However, lots of statement parsing logic relies on
getCurFunction() returning something reasonable. It turns out that
getCurFunction() will never return null today because of an optimization
where Sema pre-allocates one FunctionScopeIfno and reuses it when
possible. This goes wrong when something inside the function body causes
us to push another function scope, such as requiring an implicit
definition of a special member function. Reusing the state clears it
out, which will lead to bugs. In PR36536, we found that the SwitchStack
gets unbalanced, because we push a switch, clear out the stack, and then
try to pop a switch that isn't there.

As a follow-up, I plan to move the pre-allocated FunctionScopeInfo out
of the FunctionScopes stack. This means the FunctionScopes stack will
often be empty, which will make getCurFunction() assert. That's a
high-risk change, so I want to separate it out.

Diff Detail

Repository
rC Clang

Event Timeline

rnk created this revision.Mar 1 2018, 4:26 PM
thakis accepted this revision.Mar 7 2018, 9:09 AM
thakis added a subscriber: thakis.

Fix LGTM, one optional comment below.

clang/lib/Sema/SemaDecl.cpp
12412 ↗(On Diff #136635)

Feels a bit long-term risky since ActOnStartOfFunctionDef() and ActOnFinishFunctionBody() both need to know about this special-case invariant. Maybe it's worth to add a FakeFunctionScopeCount member to sema in +assert builds, and to increment that here, assert it's > 0 in the other place and decrement it there, and then assert it's 0 at end of TU?

clang/test/SemaCXX/pr36536.cpp
19 ↗(On Diff #136635)

Not 100% clear to me what the FIXME is here. Maybe "FIXME: We should improve our recovery to redeclare...." if that's what's meant.

This revision is now accepted and ready to land.Mar 7 2018, 9:09 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Mar 7 2018, 11:37 AM

Thanks!

John touched this code last in rL112038 in 2010, so maybe he has some thoughts on how to clean this and the follow-up. I think I'll land this as is since it fixes the crash and we can discuss more improvements in D44039.

clang/lib/Sema/SemaDecl.cpp
12412 ↗(On Diff #136635)

Well, it's more like these early returns break the invariant that ActOnStartOfFunctionDef pushes a function scope. We could try to clean it up with an RAII helper or an layer of function call that ensures that we always push and pop on start and finish, but I'll leave that for the follow-up.

clang/test/SemaCXX/pr36536.cpp
19 ↗(On Diff #136635)

I rewrote this to clarify things.