llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/138009.diff 7 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (+4) - (modified) clang/include/clang/Basic/DiagnosticGroups.td (+3-1) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+6) - (modified) clang/lib/Sema/JumpDiagnostics.cpp (+30-16) - (modified) clang/lib/Sema/SemaDecl.cpp (+5-3) - (added) clang/test/Sema/warn-jump-bypasses-init.c (+31) - (modified) clang/test/SemaOpenACC/no-branch-in-out.c (+3-2) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 5cc1a36fac1e8..91c526c4cbd42 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -188,6 +188,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++. C2y Feature Support ^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 75e8fc541305b..ec324cd5a8d62 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -166,9 +166,11 @@ def DefaultConstInit : DiagGroup<"default-const-init", [DefaultConstInitUnsafe]> def ImplicitVoidPtrCast : DiagGroup<"implicit-void-ptr-cast">; def ImplicitIntToEnumCast : DiagGroup<"implicit-int-enum-cast", [ImplicitEnumEnumCast]>; +def JumpBypassesInit : DiagGroup<"jump-bypasses-init">; def CXXCompat: DiagGroup<"c++-compat", [ImplicitVoidPtrCast, DefaultConstInit, ImplicitIntToEnumCast, HiddenCppDecl, - InitStringTooLongForCpp]>; + InitStringTooLongForCpp, + JumpBypassesInit]>; def ExternCCompat : DiagGroup<"extern-c-compat">; def KeywordCompat : DiagGroup<"keyword-compat">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 823d4bb687497..1c9a98f565fdc 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6561,11 +6561,17 @@ 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< diff --git a/clang/lib/Sema/JumpDiagnostics.cpp b/clang/lib/Sema/JumpDiagnostics.cpp index edcfffa2b3894..8b124fa76e046 100644 --- a/clang/lib/Sema/JumpDiagnostics.cpp +++ b/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) { @@ -932,8 +944,9 @@ void JumpScopeChecker::DiagnoseIndirectOrAsmJump(Stmt *Jump, unsigned JumpScope, /// 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 +986,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 +1015,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); } } diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index dfc718eedc1d9..2260159b68293 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -13705,15 +13705,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) { diff --git a/clang/test/Sema/warn-jump-bypasses-init.c b/clang/test/Sema/warn-jump-bypasses-init.c new file mode 100644 index 0000000000000..9ed7d0463f85a --- /dev/null +++ b/clang/test/Sema/warn-jump-bypasses-init.c @@ -0,0 +1,31 @@ +// 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; + } +} diff --git a/clang/test/SemaOpenACC/no-branch-in-out.c b/clang/test/SemaOpenACC/no-branch-in-out.c index 37126d8f2200e..b537799c38acc 100644 --- a/clang/test/SemaOpenACC/no-branch-in-out.c +++ b/clang/test/SemaOpenACC/no-branch-in-out.c @@ -560,13 +560,14 @@ LABEL3:{} // #GOTOLBL3 void IndirectGoto3_Loop() { void* ptr; #pragma acc parallel loop// #GOTOPAR_LOOP3 - for (unsigned i = 0; i < 5; ++i) { + for (unsigned i = 0; i < 5; ++i) { // #INIT LABEL3:{} // #GOTOLBL3_2 ptr = &&LABEL3; } -// expected-error@+3{{cannot jump from this indirect goto statement to one of its possible targets}} +// expected-error@+4{{cannot jump from this indirect goto statement to one of its possible targets}} // expected-note@#GOTOLBL3_2{{possible target of indirect goto statement}} // expected-note@#GOTOPAR_LOOP3{{invalid branch into OpenACC Compute/Combined Construct}} +// expected-note@#INIT {{jump bypasses variable initialization}} goto *ptr; } `````````` </details> https://github.com/llvm/llvm-project/pull/138009 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
