[Attr] Support _attribute__ ((fallthrough))

[xbolva00]

Summary

Solves bug:
https://bugs.llvm.org/show_bug.cgi?id=4546

ClangBuiltLinux is interested to have it in Clang too:
https://github.com/ClangBuiltLinux/linux/issues/235

[xbolva00]

test/Sema/block-literal.c

44 ↗

(On Diff #204509)

I tried look at this, but I have no idea how my change to parse GNU affects __block.

[nickdesaulniers]

Thanks for the patch, I look forward to this feature!

I think the changes in test/SemaCXX/warn-unused-label-error.cpp, test/Sema/block-literal.c, and test/Sema/address_spaces.c should not be committed (2 look like unrelated cleanups?).

lib/Parse/ParseStmt.cpp

110–111 ↗

(On Diff #204509)

Wouldn't this statement have to change? __attribute__((fallthrough)) is an "attribute on [an] empty statement." Maybe this is not the correct place to try to parse a GNU attr?

lib/Sema/AnalysisBasedWarnings.cpp

1278–1279

Probably should additionally/instead check S.getLangOpts().GNUMode (since these are GNU C style attributes)? I guess we want these attributes to be supported in C regardless of -std=?

test/Sema/block-literal.c

44 ↗

(On Diff #204509)

If you remove your change to lib/Parse/ParseStmt.cpp, does this test still fail in this way? Did you test this on an assertion enabled build (-DLLVM_ENABLE_ASSERTIONS=ON)? (I would have expected the assertion I commented on above to fail.)

test/Sema/fallthrough-attr.c

2

would you mind adding a run for -std=gnu89? In particular, I'm worried about you're above change checking c99.

[xbolva00]

lib/Parse/ParseStmt.cpp

110–111 ↗

(On Diff #204509)

Since we parse [[ ]] parse here too, I expect it could be correct place.

@aaron.ballman ?

lib/Sema/AnalysisBasedWarnings.cpp

1278–1279

Good point.

test/Sema/block-literal.c

44 ↗

(On Diff #204509)

Yes, added line "MaybeParseGNUAttributes(Attrs);" cause this changes in block-literal.c and address_spaces.c.

44 ↗

(On Diff #204509)

Yes, LLVM_ENABLE_ASSERTIONS is ticked in cmake-gui.. I have no assert failure in that place..

[xbolva00]

Well, I am not familiar with __block, but this is quite weird :)

__block y = 7;

if (Tok.is(tok::kw___attribute))

printf("__attr\n");

Output:
__attr

[xbolva00]

test/Sema/block-literal.c

44 ↗

(On Diff #204509)

See my comment.. if __block is internally as "kw _ _ attribute", now I know why this was changed.

[xbolva00]

Addressed some review notes. Thanks.

[xbolva00]

test/Sema/address_spaces.c

12 ↗

(On Diff #204584)

I think this is an acceptable change..

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

23 ↗

(On Diff #204584)

I think this is an improvement.

[nickdesaulniers]

lib/Sema/AnalysisBasedWarnings.cpp

1278–1279

IIUC, we allow GNU C attributes for C regardless of ISO vs GNU C. What we want to express is "if c++11 and newer, or c".

I think this might be better expressed as:

if (S.getLangOpts().CPlusPlus11 || (!S.getLangOpts().CPlusPlus && !S.getLangOpts().ObjC) {

But maybe there's a more canonical way to express "if the language is C." (If not, maybe we can consider such a change to LangOpt to make this easier, but as part of this commit).

[nickdesaulniers]

but as part of this commit

but *not* as part of this commit

[efriedma]

test/Sema/address_spaces.c

12 ↗

(On Diff #204584)

This is scary. gcc and clang both parse void f() { __attribute((aligned)) *x; } etc. as a declaration; I don't think we want to change that, even if that usage is a bit dubious in modern C. And it's not clear to me if there are other implications here; does this affect the handling of statement/declaration ambiguity in C++?

[nickdesaulniers]

test/Sema/address_spaces.c

12 ↗

(On Diff #204584)

It's a pointer to implicit int. Either way, I think the change and the comments are polluting this code review, hence the suggestion to submit as a separate individual patch.

[xbolva00]

test/Sema/address_spaces.c

12 ↗

(On Diff #204584)

But this patch causes changes in those files. So I dont know what to split here...

[xbolva00]

lib/Sema/AnalysisBasedWarnings.cpp

1278–1279

Yeah, I wanted to check if lang is C but I didnt find a nice way :(

[efriedma]

test/Sema/address_spaces.c

12 ↗

(On Diff #204584)

This is caused by the change to lib/Parse/ParseStmt.cpp, right? You should be able to split that into a separate patch, even if there aren't any usable statement attributes without the other parts of the patch.

[nickdesaulniers]

test/Sema/address_spaces.c

12 ↗

(On Diff #204584)

Is it because the added call to MaybeParseGNUAttributes() in lib/Parse/ParseStmt.cpp can eventually produce this error? Your change as is seems to be breaking declarations of implicit integers.

[xbolva00]

Split.

Now patch only adds _attribute__ ((fallthrough)) support. Parsing will be hopefully solved in second patch.

[xbolva00]

Fixed EOF in test case.

[nickdesaulniers]

test/Sema/fallthrough-attr.c

49

Seems like lines 24-48 were an accidental copy+paste?

[xbolva00]

fixed testcase

[aaron.ballman]

lib/Sema/AnalysisBasedWarnings.cpp

1233

I'd prefer to see this be __attribute__((fallthrough)) without the extra whitespace.

1278–1279

I think we want to support this attribute in ObjC because that language is based on C. In fact, I think we are supporting this attribute in all language modes regardless of -std level because the user can always use __attribute__((fallthrough)) as a spelling.

[xbolva00]

As noted in D63299, seems like this would require more surgery - I cannot continue D63299 since I have no enough knowledge to rework it fully.

I will update this patch, in case somebody picks D63299.

[xbolva00]

Addressed notes. Thanks.

[rjmccall]

lib/Sema/AnalysisBasedWarnings.cpp

1278–1279

As of a few weeks ago, you can provide custom code in a LangOpt; see how ObjCNonFragileRuntime is defined.

[kees]

For latest version see https://reviews.llvm.org/D64838

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];

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_break_fixit) <<
-        FixItHint::CreateInsertion(L, "break; ");
+        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; ");
     }
   }
 
   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,

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;
+}

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;
+}