[C] Add -Wjump-bypasses-init (#138009)

We already diagnose when a jump bypasses initialization in C++ because
such code is ill-formed there. However, we were not using this to
diagnose those same jumps in C.

-Wjump-bypasses-init is grouped under -Wc++-compat and diagnoses this
situation as a compatibility issue with C++. This diagnostic is off by
default.

The diagnostic could perhaps be enabled by default for C, but due to the
design of jump diagnostic handling, it not a trivial task. For now,
we'll add the diagnostic as off-by-default so we get incremental
improvement, but a follow-up could try to refactor jump diagnostics so
we can enable this by default in C and have it as a C++ compatibility
diagnostic as well.

Author: AaronBallman

clang/docs/ReleaseNotes.rst

@@ -203,6 +203,10 @@ C Language Changes
   ``-Wunterminated-string-initialization``. However, this diagnostic is not
   silenced by the ``nonstring`` attribute as these initializations are always
   incompatible with C++.
+- Added ``-Wjump-bypasses-init``, which is off by default and grouped under
+  ``-Wc++-compat``. It diagnoses when a jump (``goto`` to its label, ``switch``
+  to its ``case``) will bypass the initialization of a local variable, which is
+  invalid in C++.
 - Added the existing ``-Wduplicate-decl-specifier`` diagnostic, which is on by
   default, to ``-Wc++-compat`` because duplicated declaration specifiers are
   not valid in C++.

clang/include/clang/Basic/DiagnosticGroups.td

@@ -177,11 +177,12 @@ def DefaultConstInit : DiagGroup<"default-const-init",
 def ImplicitVoidPtrCast : DiagGroup<"implicit-void-ptr-cast">;
 def ImplicitIntToEnumCast : DiagGroup<"implicit-int-enum-cast",
                                       [ImplicitEnumEnumCast]>;
+def JumpBypassesInit : DiagGroup<"jump-bypasses-init">;
 def TentativeDefnCompat : DiagGroup<"tentative-definition-compat">;
 def CXXCompat: DiagGroup<"c++-compat", [ImplicitVoidPtrCast, DefaultConstInit,
-                                        ImplicitIntToEnumCast, CppKeywordInC,
-                                        HiddenCppDecl, TentativeDefnCompat,
-                                        InitStringTooLongForCpp,                                        
+                                        ImplicitIntToEnumCast, HiddenCppDecl,
+                                        InitStringTooLongForCpp, CppKeywordInC,
+                                        TentativeDefnCompat, JumpBypassesInit,
                                         DuplicateDeclSpecifier]>;
 
 def ExternCCompat : DiagGroup<"extern-c-compat">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -6561,18 +6561,28 @@ def ext_goto_into_protected_scope : ExtWarn<
 def warn_cxx98_compat_goto_into_protected_scope : Warning<
   "jump from this goto statement to its label is incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
+def warn_cpp_compat_goto_into_protected_scope : Warning<
+  "jump from this goto statement to its label is incompatible with C++">,
+  InGroup<JumpBypassesInit>, DefaultIgnore;
 def err_switch_into_protected_scope : Error<
   "cannot jump from switch statement to this case label">;
 def warn_cxx98_compat_switch_into_protected_scope : Warning<
   "jump from switch statement to this case label is incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
+def warn_cpp_compat_switch_into_protected_scope : Warning<
+  "jump from switch statement to this case label is incompatible with C++">,
+  InGroup<JumpBypassesInit>, DefaultIgnore;
 def err_indirect_goto_without_addrlabel : Error<
   "indirect goto in function with no address-of-label expressions">;
 def err_indirect_goto_in_protected_scope : Error<
   "cannot jump from this %select{indirect|asm}0 goto statement to one of its possible targets">;
 def warn_cxx98_compat_indirect_goto_in_protected_scope : Warning<
   "jump from this %select{indirect|asm}0 goto statement to one of its possible targets "
   "is incompatible with C++98">, InGroup<CXX98Compat>, DefaultIgnore;
+def warn_cpp_compat_indirect_goto_in_protected_scope : Warning<
+  "jump from this %select{indirect|asm}0 goto statement to one of its possible "
+  "targets is incompatible with C++">,
+  InGroup<JumpBypassesInit>, DefaultIgnore;
 def note_indirect_goto_target : Note<
   "possible target of %select{indirect|asm}0 goto statement">;
 def note_protected_by_variable_init : Note<

clang/lib/Sema/JumpDiagnostics.cpp

@@ -93,7 +93,7 @@ class JumpScopeChecker {
                                  unsigned TargetScope);
   void CheckJump(Stmt *From, Stmt *To, SourceLocation DiagLoc,
                  unsigned JumpDiag, unsigned JumpDiagWarning,
-                 unsigned JumpDiagCXX98Compat);
+                 unsigned JumpDiagCompat);
   void CheckGotoStmt(GotoStmt *GS);
   const Attr *GetMustTailAttr(AttributedStmt *AS);
 
@@ -179,9 +179,8 @@ static ScopePair GetDiagForGotoScopeDecl(Sema &S, const Decl *D) {
       }
     }
 
-    if (const Expr *Init = VD->getInit(); S.Context.getLangOpts().CPlusPlus &&
-                                          VD->hasLocalStorage() && Init &&
-                                          !Init->containsErrors()) {
+    if (const Expr *Init = VD->getInit();
+        VD->hasLocalStorage() && Init && !Init->containsErrors()) {
       // C++11 [stmt.dcl]p3:
       //   A program that jumps from a point where a variable with automatic
       //   storage duration is not in scope to a point where it is in scope
@@ -680,7 +679,9 @@ void JumpScopeChecker::VerifyJumps() {
         CheckJump(GS, GS->getLabel()->getStmt(), GS->getGotoLoc(),
                   diag::err_goto_into_protected_scope,
                   diag::ext_goto_into_protected_scope,
-                  diag::warn_cxx98_compat_goto_into_protected_scope);
+                  S.getLangOpts().CPlusPlus
+                      ? diag::warn_cxx98_compat_goto_into_protected_scope
+                      : diag::warn_cpp_compat_goto_into_protected_scope);
       }
       CheckGotoStmt(GS);
       continue;
@@ -708,7 +709,9 @@ void JumpScopeChecker::VerifyJumps() {
       CheckJump(IGS, Target->getStmt(), IGS->getGotoLoc(),
                 diag::err_goto_into_protected_scope,
                 diag::ext_goto_into_protected_scope,
-                diag::warn_cxx98_compat_goto_into_protected_scope);
+                S.getLangOpts().CPlusPlus
+                    ? diag::warn_cxx98_compat_goto_into_protected_scope
+                    : diag::warn_cpp_compat_goto_into_protected_scope);
       continue;
     }
 
@@ -725,7 +728,9 @@ void JumpScopeChecker::VerifyJumps() {
       else
         Loc = SC->getBeginLoc();
       CheckJump(SS, SC, Loc, diag::err_switch_into_protected_scope, 0,
-                diag::warn_cxx98_compat_switch_into_protected_scope);
+                S.getLangOpts().CPlusPlus
+                    ? diag::warn_cxx98_compat_switch_into_protected_scope
+                    : diag::warn_cpp_compat_switch_into_protected_scope);
     }
   }
 }
@@ -867,6 +872,13 @@ static bool IsCXX98CompatWarning(Sema &S, unsigned InDiagNote) {
          InDiagNote == diag::note_protected_by_variable_non_pod;
 }
 
+/// Returns true if a particular note should be a C++ compatibility warning in
+/// C mode with -Wc++-compat.
+static bool IsCppCompatWarning(Sema &S, unsigned InDiagNote) {
+  return !S.getLangOpts().CPlusPlus &&
+         InDiagNote == diag::note_protected_by_variable_init;
+}
+
 /// Produce primary diagnostic for an indirect jump statement.
 static void DiagnoseIndirectOrAsmJumpStmt(Sema &S, Stmt *Jump,
                                           LabelDecl *Target, bool &Diagnosed) {
@@ -906,34 +918,43 @@ void JumpScopeChecker::DiagnoseIndirectOrAsmJump(Stmt *Jump, unsigned JumpScope,
       S.Diag(Scopes[I].Loc, Scopes[I].OutDiag);
     }
 
-  SmallVector<unsigned, 10> ToScopesCXX98Compat;
+  SmallVector<unsigned, 10> ToScopesCXX98Compat, ToScopesCppCompat;
 
   // Now walk into the scopes containing the label whose address was taken.
   for (unsigned I = TargetScope; I != Common; I = Scopes[I].ParentScope)
     if (IsCXX98CompatWarning(S, Scopes[I].InDiag))
       ToScopesCXX98Compat.push_back(I);
+    else if (IsCppCompatWarning(S, Scopes[I].InDiag))
+      ToScopesCppCompat.push_back(I);
     else if (Scopes[I].InDiag) {
       DiagnoseIndirectOrAsmJumpStmt(S, Jump, Target, Diagnosed);
       S.Diag(Scopes[I].Loc, Scopes[I].InDiag);
     }
 
-  // Diagnose this jump if it would be ill-formed in C++98.
-  if (!Diagnosed && !ToScopesCXX98Compat.empty()) {
+  // Diagnose this jump if it would be ill-formed in C++[98].
+  if (!Diagnosed) {
     bool IsAsmGoto = isa<GCCAsmStmt>(Jump);
-    S.Diag(Jump->getBeginLoc(),
-           diag::warn_cxx98_compat_indirect_goto_in_protected_scope)
-        << IsAsmGoto;
-    S.Diag(Target->getStmt()->getIdentLoc(), diag::note_indirect_goto_target)
-        << IsAsmGoto;
-    NoteJumpIntoScopes(ToScopesCXX98Compat);
+    auto Diag = [&](unsigned DiagId, const SmallVectorImpl<unsigned> &Notes) {
+      S.Diag(Jump->getBeginLoc(), DiagId) << IsAsmGoto;
+      S.Diag(Target->getStmt()->getIdentLoc(), diag::note_indirect_goto_target)
+          << IsAsmGoto;
+      NoteJumpIntoScopes(Notes);
+    };
+    if (!ToScopesCXX98Compat.empty())
+      Diag(diag::warn_cxx98_compat_indirect_goto_in_protected_scope,
+           ToScopesCXX98Compat);
+    else if (!ToScopesCppCompat.empty())
+      Diag(diag::warn_cpp_compat_indirect_goto_in_protected_scope,
+           ToScopesCppCompat);
   }
 }
 
 /// CheckJump - Validate that the specified jump statement is valid: that it is
 /// jumping within or out of its current scope, not into a deeper one.
 void JumpScopeChecker::CheckJump(Stmt *From, Stmt *To, SourceLocation DiagLoc,
-                               unsigned JumpDiagError, unsigned JumpDiagWarning,
-                                 unsigned JumpDiagCXX98Compat) {
+                                 unsigned JumpDiagError,
+                                 unsigned JumpDiagWarning,
+                                 unsigned JumpDiagCompat) {
   if (CHECK_PERMISSIVE(!LabelAndGotoScopes.count(From)))
     return;
   if (CHECK_PERMISSIVE(!LabelAndGotoScopes.count(To)))
@@ -973,15 +994,16 @@ void JumpScopeChecker::CheckJump(Stmt *From, Stmt *To, SourceLocation DiagLoc,
   if (CommonScope == ToScope) return;
 
   // Pull out (and reverse) any scopes we might need to diagnose skipping.
-  SmallVector<unsigned, 10> ToScopesCXX98Compat;
+  SmallVector<unsigned, 10> ToScopesCompat;
   SmallVector<unsigned, 10> ToScopesError;
   SmallVector<unsigned, 10> ToScopesWarning;
   for (unsigned I = ToScope; I != CommonScope; I = Scopes[I].ParentScope) {
     if (S.getLangOpts().MSVCCompat && JumpDiagWarning != 0 &&
         IsMicrosoftJumpWarning(JumpDiagError, Scopes[I].InDiag))
       ToScopesWarning.push_back(I);
-    else if (IsCXX98CompatWarning(S, Scopes[I].InDiag))
-      ToScopesCXX98Compat.push_back(I);
+    else if (IsCXX98CompatWarning(S, Scopes[I].InDiag) ||
+             IsCppCompatWarning(S, Scopes[I].InDiag))
+      ToScopesCompat.push_back(I);
     else if (Scopes[I].InDiag)
       ToScopesError.push_back(I);
   }
@@ -1001,10 +1023,10 @@ void JumpScopeChecker::CheckJump(Stmt *From, Stmt *To, SourceLocation DiagLoc,
     NoteJumpIntoScopes(ToScopesError);
   }
 
-  // Handle -Wc++98-compat warnings if the jump is well-formed.
-  if (ToScopesError.empty() && !ToScopesCXX98Compat.empty()) {
-    S.Diag(DiagLoc, JumpDiagCXX98Compat);
-    NoteJumpIntoScopes(ToScopesCXX98Compat);
+  // Handle -Wc++98-compat or -Wc++-compat warnings if the jump is well-formed.
+  if (ToScopesError.empty() && !ToScopesCompat.empty()) {
+    S.Diag(DiagLoc, JumpDiagCompat);
+    NoteJumpIntoScopes(ToScopesCompat);
   }
 }
 

clang/lib/Sema/SemaDecl.cpp

@@ -13724,15 +13724,17 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
       return;
     }
 
-    if (VDecl->hasLocalStorage())
-      setFunctionHasBranchProtectedScope();
-
     if (DiagnoseUnexpandedParameterPack(Init, UPPC_Initializer)) {
       VDecl->setInvalidDecl();
       return;
     }
   }
 
+  // If the variable has an initializer and local storage, check whether
+  // anything jumps over the initialization.
+  if (VDecl->hasLocalStorage())
+    setFunctionHasBranchProtectedScope();
+
   // OpenCL 1.1 6.5.2: "Variables allocated in the __local address space inside
   // a kernel function cannot be initialized."
   if (VDecl->getType().getAddressSpace() == LangAS::opencl_local) {

clang/test/Sema/warn-jump-bypasses-init.c

@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -fsyntax-only -verify=c,both -Wjump-bypasses-init %s
+// RUN: %clang_cc1 -fsyntax-only -verify=c,both -Wc++-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify=good %s
+// RUN: %clang_cc1 -fsyntax-only -verify=cxx,both -x c++ %s
+// good-no-diagnostics
+
+void goto_func_1(void) {
+  goto ouch;  // c-warning {{jump from this goto statement to its label is incompatible with C++}} \
+                 cxx-error {{cannot jump from this goto statement to its label}}
+  int i = 12; // both-note {{jump bypasses variable initialization}}
+
+ouch:
+  ;
+}
+
+void goto_func_2(void) {
+  goto ouch;
+  static int i = 12; // This initialization is not jumped over, so no warning.
+
+ouch:
+  ;
+}
+
+void switch_func(int i) {
+  switch (i) {
+    int x = 12; // both-note {{jump bypasses variable initialization}}
+  case 0:       // c-warning {{jump from switch statement to this case label is incompatible with C++}} \
+                   cxx-error {{cannot jump from switch statement to this case label}}
+    break;
+  }
+}
+
+// Statement expressions are a bit strange in that they seem to allow for
+// jumping past initialization without being diagnosed, even in C++. Perhaps
+// this should change?
+void f(void) {
+  ({
+    goto ouch;
+    int i = 12;
+  });
+
+  for (int i = ({ goto ouch; int x = 10; x;}); i < 0; ++i) {
+  }
+
+ouch:
+  ;
+}
+
+void indirect(int n) {
+DirectJump:
+  ;
+
+  void *Table[] = {&&DirectJump, &&Later};
+  goto *Table[n]; // c-warning {{jump from this indirect goto statement to one of its possible targets is incompatible with C++}} \
+                     cxx-error {{cannot jump from this indirect goto statement to one of its possible targets}}
+
+  int x = 12;     // both-note {{jump bypasses variable initialization}}
+Later:            // both-note {{possible target of indirect goto statement}}
+  ;
+}