[Attr] Support _attribute__ ((fallthrough))

[Nathan-Huckleberry]

Summary

Fixed extraneous matches of non-NullStmt

[Nathan-Huckleberry]

Revival of https://reviews.llvm.org/D63260 and https://reviews.llvm.org/D63299.

[Nathan-Huckleberry]

• Fixed formatting

[xbolva00]

clang/lib/Parse/ParseTentative.cpp

2131 ↗

(On Diff #210215)

Negative size() ?

Did you mean “== 0”? Not sure if “empty()” exists there..

clang/lib/Sema/AnalysisBasedWarnings.cpp

1278–1279

Make it unconditional (see my patch)

[xbolva00]

Thanks, I think this is fine solution for now.

Probably not ideal (@aaron.ballman mentioned the ideal solution - rewrite the parser), but “suboptimal” parser should not stop any progress in this area.

[xbolva00]

clang/lib/Parse/ParseTentative.cpp

2146 ↗

(On Diff #210215)

Is this “cheap” in terms of compile time?

[efriedma]

clang/lib/Parse/ParseStmt.cpp

104

If we're going to generally support statement attributes, it should be possible to apply them to non-null statements. (Even if there aren't any valid attributes right now, that's likely to change in the future.) Or is that not possible to implement for some reason?

[rsmith]

clang/lib/Parse/ParseTentative.cpp

2130 ↗

(On Diff #210215)

It's not correct in general to call arbitrary parsing actions in a tentatively-parsed region; they might annotate or otherwise mess with the token stream in undesirable ways, or produce diagnostics, etc. If we keep this tentative parsing formulation, you'll need to instead recognize the __attribute__ token then manually skip its attribute list.

2146 ↗

(On Diff #210215)

No; this is not a reasonable thing to do for every block-scope statement or declaration that we parse.

We should do the same thing that we do for all other kinds of attribute: parse them unconditionally then reject them in the contexts where they should not be permitted.

[Nathan-Huckleberry]

The main problem that we have is that the __attribute__ token always causes the parser to read the line as a declaration. Then the declaration parser handles reading the attributes list.

This case demonstrates the problem:

void foo() {
  __attribute__((address_space(0))) *x;
}

If the attribute token is read beforehand this code just becomes *x and is parsed as a dereference of an undefined variable when it should actually be a declaration.

Maybe the best solution is to pull the attributes parsing out to ParseStatementOrDeclaration then pass those attributes through to following functions.
When the determination is made whether a line is a declaration or a statement the attributes list can be looked at to determine if the attributes are statement or declaration attributes and continue accordingly. Maybe throwing a warning if mixing of declaration and statement attributes occur.

Does that sound like a good solution?

[efriedma]

Yes, that seems reasonable.

[aaron.ballman]

In D64838#1589770, @Nathan-Huckleberry wrote:

The main problem that we have is that the __attribute__ token always causes the parser to read the line as a declaration. Then the declaration parser handles reading the attributes list.

This case demonstrates the problem:

void foo() {
  __attribute__((address_space(0))) *x;
}

If the attribute token is read beforehand this code just becomes *x and is parsed as a dereference of an undefined variable when it should actually be a declaration.

Maybe the best solution is to pull the attributes parsing out to ParseStatementOrDeclaration then pass those attributes through to following functions.
When the determination is made whether a line is a declaration or a statement the attributes list can be looked at to determine if the attributes are statement or declaration attributes and continue accordingly. Maybe throwing a warning if mixing of declaration and statement attributes occur.

Does that sound like a good solution?

Please see the discussion in https://reviews.llvm.org/D63299#inline-564887 -- if I understand your approach properly, this approach leads to spooky action at a distance, where the name of the attribute dictates whether something is a statement or a declaration. I'm not comfortable with that. There's no reason we could not have an attribute that is both a statement attribute and a declaration attribute, and how would *that* parse?

I think this requires some deeper surgery in the parsing logic so that attributes do not impact whether something is treated as a declaration or a statement. The parser should parse attributes first, keep them around for a while, and attach them to either the decl or the statement at a later point.

clang/lib/Parse/ParseStmt.cpp

104

We already generally support statement attributes. See ProcessStmtAttribute() in SemaStmtAttr.cpp. They definitely need to apply to non-null statements.

[Nathan-Huckleberry]

In D64838#1592111, @aaron.ballman wrote:

In D64838#1589770, @Nathan-Huckleberry wrote:

The main problem that we have is that the __attribute__ token always causes the parser to read the line as a declaration. Then the declaration parser handles reading the attributes list.

This case demonstrates the problem:

void foo() {
  __attribute__((address_space(0))) *x;
}

If the attribute token is read beforehand this code just becomes *x and is parsed as a dereference of an undefined variable when it should actually be a declaration.

Maybe the best solution is to pull the attributes parsing out to ParseStatementOrDeclaration then pass those attributes through to following functions.
When the determination is made whether a line is a declaration or a statement the attributes list can be looked at to determine if the attributes are statement or declaration attributes and continue accordingly. Maybe throwing a warning if mixing of declaration and statement attributes occur.

Does that sound like a good solution?

Please see the discussion in https://reviews.llvm.org/D63299#inline-564887 -- if I understand your approach properly, this approach leads to spooky action at a distance, where the name of the attribute dictates whether something is a statement or a declaration. I'm not comfortable with that. There's no reason we could not have an attribute that is both a statement attribute and a declaration attribute, and how would *that* parse?

I think this requires some deeper surgery in the parsing logic so that attributes do not impact whether something is treated as a declaration or a statement. The parser should parse attributes first, keep them around for a while, and attach them to either the decl or the statement at a later point.

Any idea how to fix the problem in the code sample I gave? The addition of the attributes token causes code to be read as a declaration and without the token it is read as a unary operator.

[aaron.ballman]

In D64838#1592118, @Nathan-Huckleberry wrote:

In D64838#1592111, @aaron.ballman wrote:

In D64838#1589770, @Nathan-Huckleberry wrote:

The main problem that we have is that the __attribute__ token always causes the parser to read the line as a declaration. Then the declaration parser handles reading the attributes list.

This case demonstrates the problem:

void foo() {
  __attribute__((address_space(0))) *x;
}

If the attribute token is read beforehand this code just becomes *x and is parsed as a dereference of an undefined variable when it should actually be a declaration.

Maybe the best solution is to pull the attributes parsing out to ParseStatementOrDeclaration then pass those attributes through to following functions.
When the determination is made whether a line is a declaration or a statement the attributes list can be looked at to determine if the attributes are statement or declaration attributes and continue accordingly. Maybe throwing a warning if mixing of declaration and statement attributes occur.

Does that sound like a good solution?

Please see the discussion in https://reviews.llvm.org/D63299#inline-564887 -- if I understand your approach properly, this approach leads to spooky action at a distance, where the name of the attribute dictates whether something is a statement or a declaration. I'm not comfortable with that. There's no reason we could not have an attribute that is both a statement attribute and a declaration attribute, and how would *that* parse?

I think this requires some deeper surgery in the parsing logic so that attributes do not impact whether something is treated as a declaration or a statement. The parser should parse attributes first, keep them around for a while, and attach them to either the decl or the statement at a later point.

Any idea how to fix the problem in the code sample I gave? The addition of the attributes token causes code to be read as a declaration and without the token it is read as a unary operator.

I would start by trying to have Parser::ParseStatementOrDeclarationAfterAttributes() parse the GNU-style attributes if the __attribute__ token is encountered, adding to the Attrs, and then retrying the loop.

[Nathan-Huckleberry]

void foo() {
  __attribute__((address_space(0))) *x;
  *y;
}

If the attributes are parsed then function body looks like this to the parser:

{
  *x; //this one has attributes now
  *y;
{

The first line should be a valid declaration and the second like should be a dereference of an uninitialized variable. If the attributes token is discarded before parsing the rest of the line the only way to differentiate these is by looking at the attributes added to them.

An alternative may be parse the attributes list and immediately try to parse as a declaration then if that parsing fails attempt to parse as something else. Although this approach also has the scary implication of things that are supposed to be declarations getting reparsed as something entirely different.

[aaron.ballman]

In D64838#1592520, @Nathan-Huckleberry wrote:

void foo() {
  __attribute__((address_space(0))) *x;
  *y;
}

If the attributes are parsed then function body looks like this to the parser:

{
  *x; //this one has attributes now
  *y;
{

The first line should be a valid declaration and the second like should be a dereference of an uninitialized variable. If the attributes token is discarded before parsing the rest of the line the only way to differentiate these is by looking at the attributes added to them.

An alternative may be parse the attributes list and immediately try to parse as a declaration then if that parsing fails attempt to parse as something else. Although this approach also has the scary implication of things that are supposed to be declarations getting reparsed as something entirely different.

The issue is that a leading GNU-style attribute is not sufficient information to determine whether we're parsing a declaration or a statement; it shouldn't always be treated as a decl-specifier. I spoke with a GCC dev about how they handle this, and effectively, they parse the attributes first then attempt to parse a declaration; if that fails, they fall back to parsing a statement. I think the way forward for us that should be similar is to parse the attributes first and then wait until we see a decl-specifier before determining whether we want to parse a declaration or a statement, and attach the attributes after we've figured out which production we have. @rsmith may have even better approaches in mind, but we're definitely agreed that we should not parse statement/decl based on attribute identity. I would hope we could find a way to avoid lots of re-parsing work if we can (even to the point of perhaps breaking the address_space case because implicit int is pretty horrible to rely on in the first place; it depends on whether breaking that will break a lot of code or not).

[Nathan-Huckleberry]

In D64838#1593516, @aaron.ballman wrote:

In D64838#1592520, @Nathan-Huckleberry wrote:

void foo() {
  __attribute__((address_space(0))) *x;
  *y;
}

If the attributes are parsed then function body looks like this to the parser:

{
  *x; //this one has attributes now
  *y;
{

The first line should be a valid declaration and the second like should be a dereference of an uninitialized variable. If the attributes token is discarded before parsing the rest of the line the only way to differentiate these is by looking at the attributes added to them.

An alternative may be parse the attributes list and immediately try to parse as a declaration then if that parsing fails attempt to parse as something else. Although this approach also has the scary implication of things that are supposed to be declarations getting reparsed as something entirely different.

The issue is that a leading GNU-style attribute is not sufficient information to determine whether we're parsing a declaration or a statement; it shouldn't always be treated as a decl-specifier. I spoke with a GCC dev about how they handle this, and effectively, they parse the attributes first then attempt to parse a declaration; if that fails, they fall back to parsing a statement. I think the way forward for us that should be similar is to parse the attributes first and then wait until we see a decl-specifier before determining whether we want to parse a declaration or a statement, and attach the attributes after we've figured out which production we have. @rsmith may have even better approaches in mind, but we're definitely agreed that we should not parse statement/decl based on attribute identity. I would hope we could find a way to avoid lots of re-parsing work if we can (even to the point of perhaps breaking the address_space case because implicit int is pretty horrible to rely on in the first place; it depends on whether breaking that will break a lot of code or not).

@xbolva00's patch https://reviews.llvm.org/D63260?id=204583 essentially does that already. I'm not sure how to continue on this patch, several solutions have been suggested, but I'm not sure which to implement.

[xbolva00]

they parse the attributes first then attempt to parse a declaration; if that fails, they fall back to parsing a statement

Well, I don’t think this reparsing is ideal in terms of compile time either.

If we really care about attributes on implicit ints: I don’t think that parsing according to attribute name is so bad solution - if only “possible” issue is same attr. name for stmt and decl for some future attribute - let’s talk with GCC devs and make a deal about it.

[aaron.ballman]

In D64838#1597771, @xbolva00 wrote:

they parse the attributes first then attempt to parse a declaration; if that fails, they fall back to parsing a statement

Well, I don’t think this reparsing is ideal in terms of compile time either.

If we really care about attributes on implicit ints: I don’t think that parsing according to attribute name is so bad solution - if only “possible” issue is same attr. name for stmt and decl for some future attribute - let’s talk with GCC devs and make a deal about it.

Richard and I spoke about that offline last week and both agree that changing parsing behavior according to the attribute name is not acceptable behavior for the parser.

[Nathan-Huckleberry]

I agree that parsing according to attribute name/type is not a good solution.

It sounds like we have narrowed it down to two choices:
Do we want to follow the gcc method of parsing once and falling back if parsing fails?
Do we want to parse attributes first and then wait until we see a decl-specifier (breaking the implicit int case)?

[rsmith]

In D64838#1602840, @Nathan-Huckleberry wrote:

I agree that parsing according to attribute name/type is not a good solution.

It sounds like we have narrowed it down to two choices:
Do we want to follow the gcc method of parsing once and falling back if parsing fails?
Do we want to parse attributes first and then wait until we see a decl-specifier (breaking the implicit int case)?

I don't think so. A GCC attribute is a decl-specifier, so should trigger implicit-int in the languages that have it.

Option 1: teach the statement/declaration disambiguation code that an initial GNU attribute does not resolve the ambiguity and that it needs to disambiguate past one.

Option 2: parse the attributes and then call the disambiguation code and tell it that we've already consumed a decl-specifier.

[Nathan-Huckleberry]

• Rework attribute parsing

[Nathan-Huckleberry]

• Formatting fixes

[aaron.ballman]

clang/lib/Parse/ParseStmt.cpp

103

Spurious newline?

217

Elide braces.

clang/lib/Sema/AnalysisBasedWarnings.cpp

1278–1279

What is special about C99 here?

[xbolva00]

clang/lib/Sema/AnalysisBasedWarnings.cpp

1278–1279

I think we talked about it in previous patch and decided to drop ‘if’ and always run this code.

I had already noted it here, probably it was just missed yet.

[xbolva00]

clang/lib/Sema/AnalysisBasedWarnings.cpp

1228

This code is not tested?

I think you can use tests from my older patch.

[Nathan-Huckleberry]

• Fix test, formatting and conditional check

[xbolva00]

Thanks! Better and better:)

Change in clang/test/Index/blocks.c seems not ideal, or is it okay? @aaron.ballman

[xbolva00]

clang/lib/Parse/ParseStmt.cpp

233

Since you know that tok is kw_attr, I think you can use 'ParseGNUAttributes'.

[Nathan-Huckleberry]

• Fix test case spacing

[Nathan-Huckleberry]

• Allow decl-specifier source location to propagate to decl parsing

[Nathan-Huckleberry]

• Remove changes from accidentally formatted files

[xbolva00]

+1, looks good

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

1767 ↗

(On Diff #213407)

Should this also be passed DeclSpecStart?

clang/lib/Parse/ParseStmt.cpp

156

I think you can use GNUAttributeLoc.isValid() instead of using the extra local variable.

233

Agreed, you don't need to use the Maybe check here.

clang/test/SemaCXX/warn-unused-label-error.cpp

23

This change in diagnostics makes me very happy!

[Nathan-Huckleberry]

• Remove 'maybe', remove boolean and fix other call to ParseSimpleDeclaration

[aaron.ballman]

This LGTM, but you should hold off a bit to commit -- @rsmith, do you have any concerns with this approach?

[vitalybuka]

Compilation fails with this patch
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/22936/steps/bootstrap%20clang/logs/stdio

[  8%] Built target LLVMMC
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:541:2: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
        default:
        ^
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:541:2: note: insert '__attribute__((fallthrough));' to silence this warning
        default:
        ^
        __attribute__((fallthrough)); 
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:541:2: note: insert 'break;' to avoid fall-through
        default:
        ^
        break; 
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:737:2: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
        default:
        ^
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:737:2: note: insert '__attribute__((fallthrough));' to silence this warning
        default:
        ^
        __attribute__((fallthrough)); 
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:737:2: note: insert 'break;' to avoid fall-through
        default:
        ^
        break; 
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:1639:3: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
                default:                /* things that break a sequence */
                ^
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:1639:3: note: insert '__attribute__((fallthrough));' to silence this warning
                default:                /* things that break a sequence */
                ^
                __attribute__((fallthrough)); 
/b/sanitizer-x86_64-linux/build/llvm/lib/Support/regcomp.c:1639:3: note: insert 'break;' to avoid fall-through
                default:                /* things that break a sequence */
                ^
                break; 
3 errors generated.
lib/Support/CMakeFiles/LLVMSupport.dir/build.make:2558: recipe for target 'lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o' failed
make[3]: *** [lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o] Error 1
CMakeFiles/Makefile2:1206: recipe for target 'lib/Support/CMakeFiles/LLVMSupport.dir/all' failed
make[2]: *** [lib/Support/CMakeFiles/LLVMSupport.dir/all] Error 2
CMakeFiles/Makefile2:22547: recipe for target 'tools/gold/CMakeFiles/LLVMgold.dir/rule' failed
make[1]: *** [tools/gold/CMakeFiles/LLVMgold.dir/rule] Error 2
Makefile:6905: recipe for target 'LLVMgold' failed
make: *** [LLVMgold] Error 2
+ echo @@@STEP_FAILURE@@@
+ check_64bit 1 sanitizer
+ CONDITION=1
+ SANITIZER=sanitizer
@@@STEP_FAILURE@@@
+ '[' 1 == 1 ']'
+ echo @@@BUILD_STEP 64-bit check-sanitizer@@@


--------------------------------------------------------------------------------
started: Wed Aug 21 01:01:43 2019
ended: Wed Aug 21 01:17:17 2019
duration: 15 mins, 34 secs

[xbolva00]

This needs to be commited https://reviews.llvm.org/D66487

clang/include/clang/Basic/Attr.td

def ExtVectorType : Attr {
   let Documentation = [Undocumented];
   // This is a type attribute with an incorrect subject list, so should not be
   // permitted by #pragma clang attribute.
   let PragmaAttributeSupport = 0;
 }
 
 def FallThrough : StmtAttr {
   let Spellings = [CXX11<"", "fallthrough", 201603>, C2x<"", "fallthrough">,
-                   CXX11<"clang", "fallthrough">];
+                   CXX11<"clang", "fallthrough">, GCC<"fallthrough">];
 //  let Subjects = [NullStmt];
   let Documentation = [FallthroughDocs];
 }
 
 def FastCall : DeclOrTypeAttr {
   let Spellings = [GCC<"fastcall">, Keyword<"__fastcall">,
                    Keyword<"_fastcall">];
 //  let Subjects = [Function, ObjCMethod];

clang/lib/Parse/ParseStmt.cpp

 Parser::ParseStatementOrDeclaration(StmtVector &Stmts,
                                     ParsedStmtContext StmtCtx,
                                     SourceLocation *TrailingElseLoc) {
 
   ParenBraceBracketBalancer BalancerRAIIObj(*this);
 
   ParsedAttributesWithRange Attrs(AttrFactory);
   MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
   if (!MaybeParseOpenCLUnrollHintAttribute(Attrs))
     return StmtError();
 
   StmtResult Res = ParseStatementOrDeclarationAfterAttributes(
       Stmts, StmtCtx, TrailingElseLoc, Attrs);
 
   assert((Attrs.empty() || Res.isInvalid() || Res.isUsable()) &&
          "attributes on empty statement");
 
   if (Attrs.empty() || Res.isInvalid())

 };
 }
 
 StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
     StmtVector &Stmts, ParsedStmtContext StmtCtx,
     SourceLocation *TrailingElseLoc, ParsedAttributesWithRange &Attrs) {
   const char *SemiError = nullptr;
   StmtResult Res;
+  bool SeenGNUAttributes = false;
+  SourceLocation GNUAttributeLoc;
 
   // Cases in this switch statement should fall through if the parser expects
   // the token to end in a semicolon (in which case SemiError should be set),
   // or they directly 'return;' if not.
 Retry:
   tok::TokenKind Kind  = Tok.getKind();
   SourceLocation AtLoc;
   switch (Kind) {
case tok::identifier: {
     // Fall through
     LLVM_FALLTHROUGH;
   }
 
   default: {
     if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
          (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
              ParsedStmtContext()) &&
-        isDeclarationStatement()) {
+        (SeenGNUAttributes || isDeclarationStatement())) {
       SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
       DeclGroupPtrTy Decl = ParseDeclaration(DeclaratorContext::BlockContext,
                                              DeclEnd, Attrs);
+      if (SeenGNUAttributes)
+        DeclStart = GNUAttributeLoc;
       return Actions.ActOnDeclStmt(Decl, DeclStart, DeclEnd);
     }
 
     if (Tok.is(tok::r_brace)) {
       Diag(Tok, diag::err_expected_statement);
       return StmtError();
     }
 
     return ParseExprStatement(StmtCtx);
   }
 
+  case tok::kw___attribute: {
+    SeenGNUAttributes = true;
+    GNUAttributeLoc = Tok.getLocation();
+    MaybeParseGNUAttributes(Attrs);
+    goto Retry;
+  }
+
   case tok::kw_case:                // C99 6.8.1: labeled-statement
     return ParseCaseStatement(StmtCtx);
   case tok::kw_default:             // C99 6.8.1: labeled-statement
     return ParseDefaultStatement(StmtCtx);
 
   case tok::l_brace:                // C99 6.8.2: compound-statement
     return ParseCompoundStatement();
   case tok::semi: {                 // C99 6.8.3p3: expression[opt] ';'

clang/lib/Sema/AnalysisBasedWarnings.cpp

static StringRef getFallthroughAttrSpelling(Preprocessor &PP,
   };
 
   TokenValue ClangFallthroughTokens[] = {
     tok::l_square, tok::l_square, PP.getIdentifierInfo("clang"),
     tok::coloncolon, PP.getIdentifierInfo("fallthrough"),
     tok::r_square, tok::r_square
   };
 
-  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17;
+  bool PreferClangAttr = !PP.getLangOpts().CPlusPlus17 && !PP.getLangOpts().C2x;
 
   StringRef MacroName;
   if (PreferClangAttr)
     MacroName = PP.getLastMacroWithSpelling(Loc, ClangFallthroughTokens);
   if (MacroName.empty())
     MacroName = PP.getLastMacroWithSpelling(Loc, FallthroughTokens);
   if (MacroName.empty() && !PreferClangAttr)
     MacroName = PP.getLastMacroWithSpelling(Loc, ClangFallthroughTokens);
-  if (MacroName.empty())
-    MacroName = PreferClangAttr ? "[[clang::fallthrough]]" : "[[fallthrough]]";
+  if (MacroName.empty()) {
+    if (!PreferClangAttr)
+      MacroName = "[[fallthrough]]";
+    else if (PP.getLangOpts().CPlusPlus)
+      MacroName = "[[clang::fallthrough]]";
+    else
+      MacroName = "__attribute__((fallthrough))";
+  }
   return MacroName;
 }
 
 static void DiagnoseSwitchLabelsFallthrough(Sema &S, AnalysisDeclContext &AC,
                                             bool PerFunction) {
-  // Only perform this analysis when using [[]] attributes. There is no good
-  // workflow for this warning when not using C++11. There is no good way to
-  // silence the warning (no attribute is available) unless we are using
-  // [[]] attributes. One could use pragmas to silence the warning, but as a
-  // general solution that is gross and not in the spirit of this warning.
-  //
-  // NOTE: This an intermediate solution. There are on-going discussions on
-  // how to properly support this warning outside of C++11 with an annotation.
-  if (!AC.getASTContext().getLangOpts().DoubleSquareBracketAttributes)
-    return;
-
   FallthroughMapper FM(S);
   FM.TraverseStmt(AC.getBody());
 
   if (!FM.foundSwitchStatements())
     return;
 
   if (PerFunction && FM.getFallthroughStmts().empty())
     return;
for (const CFGBlock *B : llvm::reverse(*Cfg)) {
 
     S.Diag(Label->getBeginLoc(),
            PerFunction ? diag::warn_unannotated_fallthrough_per_function
                        : diag::warn_unannotated_fallthrough);
 
     if (!AnnotatedCnt) {
       SourceLocation L = Label->getBeginLoc();
       if (L.isMacroID())
         continue;
-      if (S.getLangOpts().CPlusPlus11) {
-        const Stmt *Term = B->getTerminatorStmt();
-        // Skip empty cases.
-        while (B->empty() && !Term && B->succ_size() == 1) {
-          B = *B->succ_begin();
-          Term = B->getTerminatorStmt();
-        }
-        if (!(B->empty() && Term && isa<BreakStmt>(Term))) {
-          Preprocessor &PP = S.getPreprocessor();
-          StringRef AnnotationSpelling = getFallthroughAttrSpelling(PP, L);
-          SmallString<64> TextToInsert(AnnotationSpelling);
-          TextToInsert += "; ";
-          S.Diag(L, diag::note_insert_fallthrough_fixit) <<
-              AnnotationSpelling <<
-              FixItHint::CreateInsertion(L, TextToInsert);
-        }
+
+        S.Diag(L, diag::note_insert_fallthrough_fixit)
+            << AnnotationSpelling
+            << FixItHint::CreateInsertion(L, TextToInsert);
       }
-      S.Diag(L, diag::note_insert_break_fixit) <<
-        FixItHint::CreateInsertion(L, "break; ");
     }
+    S.Diag(L, diag::note_insert_break_fixit)
+        << FixItHint::CreateInsertion(L, "break; ");
   }
 
   for (const auto *F : FM.getFallthroughStmts())
     S.Diag(F->getBeginLoc(), diag::err_fallthrough_attr_invalid_placement);
 }
 
 static bool isInLoop(const ASTContext &Ctx, const ParentMap &PM,
                      const Stmt *S) {

clang/test/Index/blocks.c

 // RUN: c-index-test -test-load-source local -fblocks %s | FileCheck %s
 
 typedef int int_t;
 struct foo { long x; };
 
 void test() {
   static struct foo _foo;
   __block i = 0;
   ^ int_t(struct foo *foo) { return (int_t) foo->x + i; }(&_foo);
 }
 
 // CHECK: blocks.c:6:6: FunctionDecl=test:6:6 (Definition) Extent=[6:1 - 10:2]
 // CHECK: blocks.c:6:13: CompoundStmt= Extent=[6:13 - 10:2]
 // CHECK: blocks.c:7:3: DeclStmt= Extent=[7:3 - 7:26]
 // CHECK: blocks.c:7:21: VarDecl=_foo:7:21 (Definition) Extent=[7:3 - 7:25]
 // CHECK: blocks.c:7:17: TypeRef=struct foo:4:8 Extent=[7:17 - 7:20]
-// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:3 - 8:16]
+// CHECK: blocks.c:8:11: VarDecl=i:8:11 (Definition) Extent=[8:11 - 8:16]
 // CHECK: blocks.c:8:15: IntegerLiteral= Extent=[8:15 - 8:16]
 // CHECK: blocks.c:9:3: CallExpr= Extent=[9:3 - 9:65]
 // CHECK: blocks.c:9:3: BlockExpr= Extent=[9:3 - 9:58]
 // CHECK: blocks.c:9:5: TypeRef=int_t:3:13 Extent=[9:5 - 9:10]
 // CHECK: blocks.c:9:23: ParmDecl=foo:9:23 (Definition) Extent=[9:11 - 9:26]
 // CHECK: blocks.c:9:18: TypeRef=struct foo:4:8 Extent=[9:18 - 9:21]
 // CHECK: blocks.c:9:28: CompoundStmt= Extent=[9:28 - 9:58]
 // CHECK: blocks.c:9:30: ReturnStmt= Extent=[9:30 - 9:55]
 // CHECK: blocks.c:9:37: BinaryOperator= Extent=[9:37 - 9:55]
 // CHECK: blocks.c:9:37: CStyleCastExpr= Extent=[9:37 - 9:51]
 // CHECK: blocks.c:9:38: TypeRef=int_t:3:13 Extent=[9:38 - 9:43]
 // CHECK: blocks.c:9:50: MemberRefExpr=x:4:19 SingleRefName=[9:50 - 9:51] RefName=[9:50 - 9:51] Extent=[9:45 - 9:51]
 // CHECK: blocks.c:9:45: DeclRefExpr=foo:9:23 Extent=[9:45 - 9:48]
 // CHECK: blocks.c:9:54: DeclRefExpr=i:8:11 Extent=[9:54 - 9:55]
 // CHECK: blocks.c:9:59: UnaryOperator= Extent=[9:59 - 9:64]
 // CHECK: blocks.c:9:60: DeclRefExpr=_foo:7:21 Extent=[9:60 - 9:64]
-

clang/test/Index/load-exprs.c

 // CHECK: load-exprs.c:6:12: VarDecl=x:6:12 (Definition) Extent=[6:3 - 6:32]
 // CHECK: load-exprs.c:6:10: TypeRef=struct X:2:8 Extent=[6:10 - 6:11]
 // CHECK: load-exprs.c:6:24: TypeRef=struct X:2:8 Extent=[6:24 - 6:25]
 // CHECK: load-exprs.c:7:9: VarDecl=xx:7:9 (Definition) Extent=[7:3 - 7:24]
 // CHECK: load-exprs.c:7:14: DeclRefExpr=ptr:3:14 Extent=[7:14 - 7:17]
 // CHECK: load-exprs.c:7:23: DeclRefExpr=x:6:12 Extent=[7:23 - 7:24]
 // CHECK: load-exprs.c:10:5: FunctionDecl=test_blocks:10:5 (Definition) Extent=[10:1 - 21:2]
 // CHECK: load-exprs.c:10:21: ParmDecl=x:10:21 (Definition) Extent=[10:17 - 10:22]
-// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:3 - 11:20]
+// CHECK: load-exprs.c:11:15: VarDecl=y:11:15 (Definition) Extent=[11:11 - 11:20]
 // CHECK: load-exprs.c:11:19: DeclRefExpr=x:10:21 Extent=[11:19 - 11:20]
 // CHECK: load-exprs.c:12:3: CallExpr= Extent=[12:3 - 19:7]
 // CHECK: load-exprs.c:13:17: VarDecl=z:13:17 (Definition) Extent=[13:6 - 13:22]
 // CHECK: load-exprs.c:14:6: DeclRefExpr=y:11:15 Extent=[14:6 - 14:7]
 // CHECK: load-exprs.c:14:13: DeclRefExpr=z:13:17 Extent=[14:13 - 14:14]
 // CHECK: load-exprs.c:14:18: DeclRefExpr=x:10:21 Extent=[14:18 - 14:19]
 // CHECK: load-exprs.c:15:6: CallExpr= Extent=[15:6 - 18:9]
 // CHECK: load-exprs.c:16:10: DeclRefExpr=z:13:17 Extent=[16:10 - 16:11]

 // CHECK: load-exprs.c:30:49: MemberRef=array:24:12 Extent=[30:49 - 30:54]
 // CHECK: load-exprs.c:30:55: DeclRefExpr=StartIndex:27:8 Extent=[30:55 - 30:65]
 // CHECK: load-exprs.c:30:67: MemberRef=a:2:16 Extent=[30:67 - 30:68]
 // CHECK: load-exprs.c:30:71: DeclRefExpr=aval:29:23 Extent=[30:71 - 30:75]
 // CHECK: load-exprs.c:31:29: TypeRef=struct Y:23:8 Extent=[31:29 - 31:30]
 // CHECK: load-exprs.c:31:32: MemberRef=array:24:12 Extent=[31:32 - 31:37]
 // CHECK: load-exprs.c:31:38: DeclRefExpr=StartIndex:27:8 Extent=[31:38 - 31:48]
 // CHECK: load-exprs.c:31:50: MemberRef=b:2:19 Extent=[31:50 - 31:51]
-

clang/test/Sema/fallthrough-attr.c

+// RUN: %clang_cc1 -fsyntax-only -std=gnu89 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=gnu99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DC2X -verify -Wimplicit-fallthrough %s
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+    n++;
+  case 1:
+#if defined(C2X)
+// expected-warning@-2{{unannotated fall-through between switch labels}} expected-note@-2{{insert '[[fallthrough]];' to silence this warning}} expected-note@-2{{insert 'break;' to avoid fall-through}}
+#else
+// expected-warning@-4{{unannotated fall-through between switch labels}} expected-note@-4{{insert '__attribute__ ((fallthrough));' to silence this warning}} expected-note@-4{{insert 'break;' to avoid fall-through}}
+#endif
+    n++;
+    __attribute__((fallthrough));
+  case 2:
+    n++;
+    break;
+  }
+  return n;
+}

clang/test/SemaCXX/switch-implicit-fallthrough.cpp

case 1:
     n++;
     [[clang::__fallthrough__]];
   case 2:
     n++;
     break;
   }
   return n;
 }
+
+int fallthrough_attribute_spelling(int n) {
+  switch (n) {
+  case 0:
+    n++;
+    __attribute__((fallthrough));
+  case 1:
+    n++;
+    break;
+  }
+  return n;
+}

clang/test/SemaCXX/warn-unused-label-error.cpp

namespace PR8455 {
   void g() {
     C: // unused label 'C' will not appear here because an error has occurred
       __attribute__((unused))
       #pragma weak unused_local_static  // expected-error {{expected ';' after __attribute__}}
       ;
   }
 
   void h() {
-    D: // expected-warning {{unused label 'D'}}
-      #pragma weak unused_local_static
-      __attribute__((unused))  // expected-warning {{declaration does not declare anything}}
-      ;
+  D:
+    __attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
   }
 }