This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Make getCurFunction() return null outside function parsing
ClosedPublic

Authored by rnk on Mar 2 2018, 11:50 AM.

Details

Summary

Before this patch, Sema pre-allocated a FunctionScopeInfo and kept it in
the first, always present element of the FunctionScopes stack. This
meant that Sema::getCurFunction would return a pointer to this
pre-allocated object when parsing code outside a function body. This is
pretty much always a bug, so this patch moves the pre-allocated object
into a separate unique_ptr. This should make bugs like PR36536 a lot
more obvious.

As you can see from this patch, there were a number of places that
unconditionally assumed they were always called inside a function.
However, there are also many places that null checked the result of
getCurFunction(), so I think this is a reasonable direction.

Diff Detail

Repository
rC Clang

Event Timeline

rnk created this revision.Mar 2 2018, 11:50 AM
thakis accepted this revision.Mar 7 2018, 9:27 AM
thakis added a subscriber: thakis.

Good luck!

I think the forward-looking statement in https://reviews.llvm.org/D43980 about this change here is a bit off, see below.

The hasLocalStorage() comment below is probably the only interesting comment.

clang/include/clang/Sema/Sema.h
1320 ↗(On Diff #136823)

The other patch description said "This means the FunctionScopes stack will often be empty, which will make getCurFunction() assert" -- did you change your mind, or did you mean "which will make the caller of getCurFunction() crash when it uses the null pointer"?

clang/lib/Sema/AnalysisBasedWarnings.cpp
635 ↗(On Diff #136823)

nit: Just remove everything up to and including the - – we no longer repeat function names in comments.

clang/lib/Sema/SemaDecl.cpp
11328 ↗(On Diff #136823)

the hasLocalStorage() check addition here seems unrelated?

clang/lib/Sema/SemaExprCXX.cpp
1117 ↗(On Diff #136823)

Maybe add "// can be -1 if there's no current function scope and FunctionScopeIndexToStopAt is not set" since that seems pretty subtle.

This revision is now accepted and ready to land.Mar 7 2018, 9:27 AM
rnk added a comment.Mar 7 2018, 9:42 AM

Thanks!

clang/include/clang/Sema/Sema.h
1320 ↗(On Diff #136823)

I changed my mind after fixing the fallout caused by the assert in .back(). There are a lot of places that want to say "if we're in a function scope and we had a VLA or object with a destructor, then mark this as a branch protected scope". If the variable is in global or class scope, we don't need to mark the scope as branch protected, and a null check seems like the right way to express that.

clang/lib/Sema/SemaDecl.cpp
11328 ↗(On Diff #136823)

This is a different way of checking if the variable is in function scope. I think I made this change before making getCurFunction() return nullptr.

rnk updated this revision to Diff 137472.Mar 7 2018, 1:50 PM

Bring back Sema::setFunctionHas* methods which internally do nothing when
called outside function scope. This appears to happen in practice when parsing
invalid code involving things like statement expressions, VLAs at global scope,
etc.

rnk added a reviewer: rjmccall.Mar 7 2018, 1:50 PM

I agree that having those sites just no-op themselves is the cleanest approach.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.