[PATCH] D43980: Push a function scope when parsing function bodies without a declaration
rnk added a comment. Thanks! John touched this code last in https://reviews.llvm.org/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 https://reviews.llvm.org/D44039. Comment at: clang/lib/Sema/SemaDecl.cpp:12412 +// anyway so we can try to parse the function body. +PushFunctionScope(); return D; thakis wrote: > 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? 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. Comment at: clang/test/SemaCXX/pr36536.cpp:19 + // this when they forget to close a namespace, and we'd generate far fewer + // errors if names in Foo were in scope. + // expected-error@+1 {{unknown type name 'NameInClass'}} thakis wrote: > 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. I rewrote this to clarify things. Repository: rC Clang https://reviews.llvm.org/D43980 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43980: Push a function scope when parsing function bodies without a declaration
This revision was automatically updated to reflect the committed changes. Closed by commit rC326926: Push a function scope when parsing function bodies without a declaration (authored by rnk, committed by ). Changed prior to commit: https://reviews.llvm.org/D43980?vs=136635&id=137437#toc Repository: rC Clang https://reviews.llvm.org/D43980 Files: lib/Sema/SemaDecl.cpp test/SemaCXX/pr36536.cpp Index: test/SemaCXX/pr36536.cpp === --- test/SemaCXX/pr36536.cpp +++ test/SemaCXX/pr36536.cpp @@ -0,0 +1,44 @@ +// RUN: %clang_cc1 -std=c++11 %s -verify -fno-spell-checking + +// These test cases are constructed to make clang call ActOnStartOfFunctionDef +// with nullptr. + +struct ImplicitDefaultCtor1 {}; +struct Foo { + typedef int NameInClass; + void f(); +}; +namespace bar { +// FIXME: Improved our recovery to make this a redeclaration of Foo::f, +// even though this is in the wrong namespace. That will allow name lookup to +// find NameInClass below. Users are likely to hit this when they forget to +// close namespaces. +// expected-error@+1 {{cannot define or redeclare 'f' here}} +void Foo::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } + // expected-error@+1 {{unknown type name 'NameInClass'}} + NameInClass var; +} +} // namespace bar + +struct ImplicitDefaultCtor2 {}; +template class TFoo { void f(); }; +// expected-error@+1 {{nested name specifier 'decltype(TFoo())::'}} +template void decltype(TFoo())::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } +} + +namespace tpl2 { +struct ImplicitDefaultCtor3 {}; +template class A { + template class B { +void mf2(); + }; +}; +template +template <> +// expected-error@+1 {{nested name specifier 'A::B::'}} +void A::B::mf2() { + switch (0) { case 0: ImplicitDefaultCtor3 o; } +} +} Index: lib/Sema/SemaDecl.cpp === --- lib/Sema/SemaDecl.cpp +++ lib/Sema/SemaDecl.cpp @@ -12406,8 +12406,13 @@ Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D, SkipBodyInfo *SkipBody) { - if (!D) + if (!D) { +// Parsing the function declaration failed in some way. Push on a fake scope +// anyway so we can try to parse the function body. +PushFunctionScope(); return D; + } + FunctionDecl *FD = nullptr; if (FunctionTemplateDecl *FunTmpl = dyn_cast(D)) @@ -12816,6 +12821,9 @@ getCurFunction()->ObjCWarnForNoInitDelegation = false; } } else { +// Parsing the function declaration failed in some way. Pop the fake scope +// we pushed on. +PopFunctionScopeInfo(ActivePolicy, dcl); return nullptr; } Index: test/SemaCXX/pr36536.cpp === --- test/SemaCXX/pr36536.cpp +++ test/SemaCXX/pr36536.cpp @@ -0,0 +1,44 @@ +// RUN: %clang_cc1 -std=c++11 %s -verify -fno-spell-checking + +// These test cases are constructed to make clang call ActOnStartOfFunctionDef +// with nullptr. + +struct ImplicitDefaultCtor1 {}; +struct Foo { + typedef int NameInClass; + void f(); +}; +namespace bar { +// FIXME: Improved our recovery to make this a redeclaration of Foo::f, +// even though this is in the wrong namespace. That will allow name lookup to +// find NameInClass below. Users are likely to hit this when they forget to +// close namespaces. +// expected-error@+1 {{cannot define or redeclare 'f' here}} +void Foo::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } + // expected-error@+1 {{unknown type name 'NameInClass'}} + NameInClass var; +} +} // namespace bar + +struct ImplicitDefaultCtor2 {}; +template class TFoo { void f(); }; +// expected-error@+1 {{nested name specifier 'decltype(TFoo())::'}} +template void decltype(TFoo())::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } +} + +namespace tpl2 { +struct ImplicitDefaultCtor3 {}; +template class A { + template class B { +void mf2(); + }; +}; +template +template <> +// expected-error@+1 {{nested name specifier 'A::B::'}} +void A::B::mf2() { + switch (0) { case 0: ImplicitDefaultCtor3 o; } +} +} Index: lib/Sema/SemaDecl.cpp === --- lib/Sema/SemaDecl.cpp +++ lib/Sema/SemaDecl.cpp @@ -12406,8 +12406,13 @@ Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D, SkipBodyInfo *SkipBody) { - if (!D) + if (!D) { +// Parsing the function declaration failed in some way. Push on a fake scope +// anyway so we can try to parse the function body. +PushFunctionScope(); return D; + } + FunctionDecl *FD = nullptr; if (FunctionTemplateDecl *FunTmpl = dyn_cast(D)) @@ -12816,6 +12821,9 @@ getCurFunction()->ObjCWarnForNoInitDelegation = false; } } else { +// Parsing the function declaration failed in some way. Pop the fake scope +// we pushed on. +
[PATCH] D43980: Push a function scope when parsing function bodies without a declaration
This revision was automatically updated to reflect the committed changes. Closed by commit rL326926: Push a function scope when parsing function bodies without a declaration (authored by rnk, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D43980?vs=136635&id=137436#toc Repository: rL LLVM https://reviews.llvm.org/D43980 Files: cfe/trunk/lib/Sema/SemaDecl.cpp cfe/trunk/test/SemaCXX/pr36536.cpp Index: cfe/trunk/test/SemaCXX/pr36536.cpp === --- cfe/trunk/test/SemaCXX/pr36536.cpp +++ cfe/trunk/test/SemaCXX/pr36536.cpp @@ -0,0 +1,44 @@ +// RUN: %clang_cc1 -std=c++11 %s -verify -fno-spell-checking + +// These test cases are constructed to make clang call ActOnStartOfFunctionDef +// with nullptr. + +struct ImplicitDefaultCtor1 {}; +struct Foo { + typedef int NameInClass; + void f(); +}; +namespace bar { +// FIXME: Improved our recovery to make this a redeclaration of Foo::f, +// even though this is in the wrong namespace. That will allow name lookup to +// find NameInClass below. Users are likely to hit this when they forget to +// close namespaces. +// expected-error@+1 {{cannot define or redeclare 'f' here}} +void Foo::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } + // expected-error@+1 {{unknown type name 'NameInClass'}} + NameInClass var; +} +} // namespace bar + +struct ImplicitDefaultCtor2 {}; +template class TFoo { void f(); }; +// expected-error@+1 {{nested name specifier 'decltype(TFoo())::'}} +template void decltype(TFoo())::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } +} + +namespace tpl2 { +struct ImplicitDefaultCtor3 {}; +template class A { + template class B { +void mf2(); + }; +}; +template +template <> +// expected-error@+1 {{nested name specifier 'A::B::'}} +void A::B::mf2() { + switch (0) { case 0: ImplicitDefaultCtor3 o; } +} +} Index: cfe/trunk/lib/Sema/SemaDecl.cpp === --- cfe/trunk/lib/Sema/SemaDecl.cpp +++ cfe/trunk/lib/Sema/SemaDecl.cpp @@ -12406,8 +12406,13 @@ Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D, SkipBodyInfo *SkipBody) { - if (!D) + if (!D) { +// Parsing the function declaration failed in some way. Push on a fake scope +// anyway so we can try to parse the function body. +PushFunctionScope(); return D; + } + FunctionDecl *FD = nullptr; if (FunctionTemplateDecl *FunTmpl = dyn_cast(D)) @@ -12816,6 +12821,9 @@ getCurFunction()->ObjCWarnForNoInitDelegation = false; } } else { +// Parsing the function declaration failed in some way. Pop the fake scope +// we pushed on. +PopFunctionScopeInfo(ActivePolicy, dcl); return nullptr; } Index: cfe/trunk/test/SemaCXX/pr36536.cpp === --- cfe/trunk/test/SemaCXX/pr36536.cpp +++ cfe/trunk/test/SemaCXX/pr36536.cpp @@ -0,0 +1,44 @@ +// RUN: %clang_cc1 -std=c++11 %s -verify -fno-spell-checking + +// These test cases are constructed to make clang call ActOnStartOfFunctionDef +// with nullptr. + +struct ImplicitDefaultCtor1 {}; +struct Foo { + typedef int NameInClass; + void f(); +}; +namespace bar { +// FIXME: Improved our recovery to make this a redeclaration of Foo::f, +// even though this is in the wrong namespace. That will allow name lookup to +// find NameInClass below. Users are likely to hit this when they forget to +// close namespaces. +// expected-error@+1 {{cannot define or redeclare 'f' here}} +void Foo::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } + // expected-error@+1 {{unknown type name 'NameInClass'}} + NameInClass var; +} +} // namespace bar + +struct ImplicitDefaultCtor2 {}; +template class TFoo { void f(); }; +// expected-error@+1 {{nested name specifier 'decltype(TFoo())::'}} +template void decltype(TFoo())::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } +} + +namespace tpl2 { +struct ImplicitDefaultCtor3 {}; +template class A { + template class B { +void mf2(); + }; +}; +template +template <> +// expected-error@+1 {{nested name specifier 'A::B::'}} +void A::B::mf2() { + switch (0) { case 0: ImplicitDefaultCtor3 o; } +} +} Index: cfe/trunk/lib/Sema/SemaDecl.cpp === --- cfe/trunk/lib/Sema/SemaDecl.cpp +++ cfe/trunk/lib/Sema/SemaDecl.cpp @@ -12406,8 +12406,13 @@ Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D, SkipBodyInfo *SkipBody) { - if (!D) + if (!D) { +// Parsing the function declaration failed in some way. Push on a fake scope +// anyway so we can try to parse the function body. +PushFunctionScope(); return D; + } + FunctionDecl *FD = nullptr; if (FunctionTemplateDecl *FunTmpl = dyn_cast(D)) @@ -12816,6 +12821,9 @@
[PATCH] D43980: Push a function scope when parsing function bodies without a declaration
thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Fix LGTM, one optional comment below. Comment at: clang/lib/Sema/SemaDecl.cpp:12412 +// anyway so we can try to parse the function body. +PushFunctionScope(); return D; 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? Comment at: clang/test/SemaCXX/pr36536.cpp:19 + // this when they forget to close a namespace, and we'd generate far fewer + // errors if names in Foo were in scope. + // expected-error@+1 {{unknown type name 'NameInClass'}} 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. https://reviews.llvm.org/D43980 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43980: Push a function scope when parsing function bodies without a declaration
rnk created this revision. rnk added a reviewer: rsmith. 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. https://reviews.llvm.org/D43980 Files: clang/lib/Sema/SemaDecl.cpp clang/test/SemaCXX/pr36536.cpp Index: clang/test/SemaCXX/pr36536.cpp === --- /dev/null +++ clang/test/SemaCXX/pr36536.cpp @@ -0,0 +1,45 @@ +// RUN: %clang_cc1 -std=c++11 %s -verify -fno-spell-checking + +// These test cases are constructed to make clang call ActOnStartOfFunctionDef +// with nullptr. + +struct ImplicitDefaultCtor1 {}; +struct Foo { + typedef int NameInClass; + void f(); +}; +namespace bar { +// expected-error@+1 {{cannot define or redeclare 'f' here}} +void Foo::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } + + // FIXME: If we improved our recovery to redeclare Foo::f in the wrong + // namespace, we could find the right type name here. Users probably run into + // this when they forget to close a namespace, and we'd generate far fewer + // errors if names in Foo were in scope. + // expected-error@+1 {{unknown type name 'NameInClass'}} + NameInClass var; +} +} // namespace bar + +struct ImplicitDefaultCtor2 {}; +template class TFoo { void f(); }; +// expected-error@+1 {{nested name specifier 'decltype(TFoo())::'}} +template void decltype(TFoo())::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } +} + +namespace tpl2 { +struct ImplicitDefaultCtor3 {}; +template class A { + template class B { +void mf2(); + }; +}; +template +template <> +// expected-error@+1 {{nested name specifier 'A::B::'}} +void A::B::mf2() { + switch (0) { case 0: ImplicitDefaultCtor3 o; } +} +} Index: clang/lib/Sema/SemaDecl.cpp === --- clang/lib/Sema/SemaDecl.cpp +++ clang/lib/Sema/SemaDecl.cpp @@ -12406,8 +12406,13 @@ Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D, SkipBodyInfo *SkipBody) { - if (!D) + if (!D) { +// Parsing the function declaration failed in some way. Push on a fake scope +// anyway so we can try to parse the function body. +PushFunctionScope(); return D; + } + FunctionDecl *FD = nullptr; if (FunctionTemplateDecl *FunTmpl = dyn_cast(D)) @@ -12816,6 +12821,9 @@ getCurFunction()->ObjCWarnForNoInitDelegation = false; } } else { +// Parsing the function declaration failed in some way. Pop the fake scope +// we pushed on. +PopFunctionScopeInfo(ActivePolicy, dcl); return nullptr; } Index: clang/test/SemaCXX/pr36536.cpp === --- /dev/null +++ clang/test/SemaCXX/pr36536.cpp @@ -0,0 +1,45 @@ +// RUN: %clang_cc1 -std=c++11 %s -verify -fno-spell-checking + +// These test cases are constructed to make clang call ActOnStartOfFunctionDef +// with nullptr. + +struct ImplicitDefaultCtor1 {}; +struct Foo { + typedef int NameInClass; + void f(); +}; +namespace bar { +// expected-error@+1 {{cannot define or redeclare 'f' here}} +void Foo::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } + + // FIXME: If we improved our recovery to redeclare Foo::f in the wrong + // namespace, we could find the right type name here. Users probably run into + // this when they forget to close a namespace, and we'd generate far fewer + // errors if names in Foo were in scope. + // expected-error@+1 {{unknown type name 'NameInClass'}} + NameInClass var; +} +} // namespace bar + +struct ImplicitDefaultCtor2 {}; +template class TFoo { void f(); }; +// expected-error@+1 {{nested name specifier 'decltype(TFoo())::'}} +template void decltype(TFoo())::f() { + switch (0) { case 0: ImplicitDefaultCtor1 o; } +} + +namespace tpl2 { +struct ImplicitDefaultCtor3 {}; +template class A { + template class B { +void mf2(); + }; +};