[PATCH] Don't crash on [[clang::fallthrough]] on Decls.

[Aaron Ballman]

On Tue, Mar 31, 2015 at 5:55 PM, Nico Weber <thakis at chromium.org> wrote:
> Hi aaron.ballman,
>
> Fixes PR23089

Thank you for working on this!

>
> http://reviews.llvm.org/D8750
>
> Files:
>   include/clang/Basic/Attr.td
>   include/clang/Basic/DiagnosticSemaKinds.td
>   include/clang/Sema/AttributeList.h
>   test/SemaCXX/switch-implicit-fallthrough.cpp
>
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -106,6 +106,8 @@
>  def GlobalVar : SubsetSubject<Var,
>                               [{S->hasGlobalStorage()}]>;
>
> +def NotAllowedOnDecls : SubsetSubject<Var, [{false}]>;
> +
>  // FIXME: this hack is needed because DeclNodes.td defines the base Decl node
>  // type to be a class, not a definition. This makes it impossible to create an
>  // attribute subject which accepts a Decl. Normally, this is not a problem,
> @@ -690,7 +692,8 @@
>
>  def FallThrough : Attr {
>    let Spellings = [CXX11<"clang", "fallthrough">];
> -//  let Subjects = [NullStmt];
> +  let Subjects = SubjectList<[NotAllowedOnDecls], ErrorDiag,
> +                             "ExpectedEmptyStatement">;

This does not seem like the correct fix to me. For starters, not on
decls != allowed on statements (for instance, this suggests it could
be applied to types as well). Also, while I know we don't have a lot
of automation in SemaStmtAttr.cpp for this, we want to keep the
declarative nature of the td file accurate, and fix the source code
instead (as NullStmt is the only valid Stmt this attribute can attach
to, but the declaration in the td file appears like you could put this
on anything that's not a declaration).

I'm traveling for work this week, and don't have the chance to look
into the code too heavily, but I suspect this is a fix that would go
into handleCommonAttributeFeatures (there are no attributes which
apply to stmt & decl like there are for type & decl, which is why that
assert exists).

I would also guess that diagnoseAppertainsTo would already handle the
diagnostic for this case (but if not, that generated code may need
updating as well).

~Aaron

>    let Documentation = [FallthroughDocs];
>  }
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -2264,7 +2264,7 @@
>    "Objective-C instance methods|init methods of interface or class extension declarations|"
>    "variables, functions and classes|Objective-C protocols|"
>    "functions and global variables|structs, unions, and typedefs|structs and typedefs|"
> -  "interface or protocol declarations|kernel functions}1">,
> +  "interface or protocol declarations|kernel functions|empty statements}1">,
>    InGroup<IgnoredAttributes>;
>  def err_attribute_wrong_decl_type : Error<warn_attribute_wrong_decl_type.Text>;
>  def warn_type_attribute_wrong_type : Warning<
> Index: include/clang/Sema/AttributeList.h
> ===================================================================
> --- include/clang/Sema/AttributeList.h
> +++ include/clang/Sema/AttributeList.h
> @@ -846,7 +846,8 @@
>    ExpectedStructOrUnionOrTypedef,
>    ExpectedStructOrTypedef,
>    ExpectedObjectiveCInterfaceOrProtocol,
> -  ExpectedKernelFunction
> +  ExpectedKernelFunction,
> +  ExpectedEmptyStatement
>  };
>
>  }  // end namespace clang
> Index: test/SemaCXX/switch-implicit-fallthrough.cpp
> ===================================================================
> --- test/SemaCXX/switch-implicit-fallthrough.cpp
> +++ test/SemaCXX/switch-implicit-fallthrough.cpp
> @@ -1,6 +1,5 @@
>  // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wimplicit-fallthrough %s
>
> -
>  int fallthrough(int n) {
>    switch (n / 10) {
>        if (n - 1) {
> @@ -300,3 +299,8 @@
>    }
>    return n;
>  }
> +
> +[[clang::fallthrough]] int a; // expected-error {{'fallthrough' attribute only applies to empty statements}}
> +[[clang::fallthrough]] int f(); // expected-error {{'fallthrough' attribute only applies to empty statements}}
> +void g([[clang::fallthrough]] int p); // expected-error {{'fallthrough' attribute only applies to empty statements}}
> +struct [[clang::fallthrough]] S; // expected-error {{'fallthrough' attribute only applies to empty statements}}
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/