[PATCH] D63889: Check possible warnings on global initializers for reachability
rsmith added inline comments.
Comment at: clang/include/clang/Parse/Parser.h:1873
+ ExprResult ParseInitializer(Decl *DeclForInitializer = nullptr) {
+Actions.ExprEvalContexts.back().DeclForInitializer = DeclForInitializer;
+ExprResult init;
This should be done by calling a function on Sema (add an
`ActOnStartDeclInitializer` or similar), not by directly modifying Sema's
internal state.
Comment at: clang/include/clang/Parse/Parser.h:1880
+}
+Actions.ExprEvalContexts.back().DeclForInitializer = nullptr;
+return init;
Please add an assertion when you first set this that the old value was null.
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2263
+VarDecl *VD) {
+ AnalysisDeclContext AC(nullptr, VD);
+
Consider grabbing `VarDeclPossiblyUnreachableDiags[VD]` first and avoiding
constructing the `AnalysisDeclContext` at all in the (presumably overwhelmingly
common case) that there are no such diagnostics.
Comment at: clang/lib/Sema/SemaDecl.cpp:11853
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(var);
+
We should (presumably) only do this for file-scope variables. The initializers
for block-scope variables will be checked when checking the enclosing function
(if the variables' declarations are reachable at all).
Comment at: clang/lib/Sema/SemaExpr.cpp:4884
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
We should only do this once, when we instantiate the default argument, rather
than once each time we use it. (Move this up to just before line 4856?)
Comment at: clang/lib/Sema/SemaExpr.cpp:16670
case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
+ FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
This call to `getCurFunctionOrMethodDecl()` is wrong; it will skip over
lambda-expressions. (This is why you're still seeing diagnostics in file-scope
lambdas.) Something like this should work:
```
if (Stmts.empty()) {
Diag(Loc, PD);
return true;
}
if (FunctionScopeInfo *FSI = getCurFunction()) {
FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
PossiblyUnreachableDiag(PD, Loc, Stmts));
return true;
}
// [handle VarDecl case]
```
Comment at: clang/lib/Sema/SemaExpr.cpp:16680-16682
+ if (VD->getDefinition()) {
+VD = VD->getDefinition();
+ }
No braces here please. Also, this appears to be wrong: we want the declaration
with the initializer (which is always `VD`), not the definition (which might be
a different declaration for a static data member), don't we?
Comment at: clang/lib/Sema/SemaExpr.cpp:16690
break;
- // FIXME: For any other kind of variable, we should build a CFG for its
- // initializer and check whether the context in question is reachable.
+ // FIXME: Some cases aren't picked up by path analysis currently
+ if (!Stmts.empty() && VD->isFileVarDecl()) {
What does this fixme mean?
Comment at: clang/test/SemaTemplate/instantiate-static-var.cpp:9
static const T value = 10 / Divisor; // expected-error{{in-class initializer
for static data member is not a constant expression}}
+ //expected-warning@-1 {{division by zero is undefined}}
};
We should not warn here (because this initializer is required to be a constant
expression).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry marked 4 inline comments as done.
Nathan-Huckleberry added inline comments.
Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:360
-constexpr int ok_byte = (__builtin_bit_cast(std::byte[8], pad{1, 2}), 0);
-constexpr int ok_uchar = (__builtin_bit_cast(unsigned char[8], pad{1, 2}), 0);
+constexpr int ok_byte = (__builtin_bit_cast(std::byte[8], pad{1, 2}), 0);
// expected-warning {{expression result unused}}
+constexpr int ok_uchar = (__builtin_bit_cast(unsigned char[8], pad{1, 2}), 0);
// expected-warning {{expression result unused}}
These new warnings seem valid.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry updated this revision to Diff 215699.
Nathan-Huckleberry added a comment.
- Use ExprEvalContext and remove mangling context code
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
Files:
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/AnalysisBasedWarnings.h
clang/include/clang/Sema/Sema.h
clang/lib/Analysis/AnalysisDeclContext.cpp
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Parse/ParseOpenMP.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-unreachable-warning-var-decl.cpp
clang/test/SemaCXX/constant-expression-cxx11.cpp
clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
clang/test/SemaTemplate/instantiate-static-var.cpp
Index: clang/test/SemaTemplate/instantiate-static-var.cpp
===
--- clang/test/SemaTemplate/instantiate-static-var.cpp
+++ clang/test/SemaTemplate/instantiate-static-var.cpp
@@ -6,6 +6,7 @@
class X {
public:
static const T value = 10 / Divisor; // expected-error{{in-class initializer for static data member is not a constant expression}}
+ //expected-warning@-1 {{division by zero is undefined}}
};
int array1[X::value == 5? 1 : -1];
Index: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
===
--- clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
+++ clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
@@ -357,8 +357,8 @@
int b;
};
-constexpr int ok_byte = (__builtin_bit_cast(std::byte[8], pad{1, 2}), 0);
-constexpr int ok_uchar = (__builtin_bit_cast(unsigned char[8], pad{1, 2}), 0);
+constexpr int ok_byte = (__builtin_bit_cast(std::byte[8], pad{1, 2}), 0); // expected-warning {{expression result unused}}
+constexpr int ok_uchar = (__builtin_bit_cast(unsigned char[8], pad{1, 2}), 0); // expected-warning {{expression result unused}}
#ifdef __CHAR_UNSIGNED__
// expected-note@+5 {{indeterminate value can only initialize an object of type 'unsigned char', 'char', or 'std::byte'; 'my_byte' is invalid
@@ -366,12 +366,12 @@
// expected-note@+3 {{indeterminate value can only initialize an object of type 'unsigned char' or 'std::byte'; 'my_byte' is invalid}}
#endif
// expected-error@+1 {{constexpr variable 'bad_my_byte' must be initialized by a constant expression}}
-constexpr int bad_my_byte = (__builtin_bit_cast(my_byte[8], pad{1, 2}), 0);
+constexpr int bad_my_byte = (__builtin_bit_cast(my_byte[8], pad{1, 2}), 0); // expected-warning {{expression result unused}}
#ifndef __CHAR_UNSIGNED__
// expected-error@+3 {{constexpr variable 'bad_char' must be initialized by a constant expression}}
// expected-note@+2 {{indeterminate value can only initialize an object of type 'unsigned char' or 'std::byte'; 'char' is invalid}}
#endif
-constexpr int bad_char = (__builtin_bit_cast(char[8], pad{1, 2}), 0);
+constexpr int bad_char = (__builtin_bit_cast(char[8], pad{1, 2}), 0); // expected-warning {{expression result unused}}
struct pad_buffer { unsigned char data[sizeof(pad)]; };
constexpr bool test_pad_buffer() {
Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -383,7 +383,7 @@
constexpr B b3 { { 1 }, { 2 } }; // expected-error {{constant expression}} expected-note {{reference to temporary}} expected-note {{here}}
}
-constexpr B & = ((1, 2), 3, 4, B { {10}, {{20}} });
+constexpr B & = ((1, 2), 3, 4, B{{10}, {{20}}}); //expected-warning {{expression result unused}}
static_assert( != , "");
// Proposed DR: copy-elision doesn't trigger lifetime extension.
Index: clang/test/Sema/warn-unreachable-warning-var-decl.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unreachable-warning-var-decl.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -verify %s
+int e = 1 ? 0 : 1 / 0;
+int g = 1 ? 1 / 0 : 0; // expected-warning{{division by zero is undefined}}
+
+#define SHIFT(n) (((n) == 32) ? 0 : ((1 << (n)) - 1))
+
+int x = SHIFT(32);
+int y = SHIFT(0);
+
+// FIXME: Expressions in lambdas aren't ignored
+int z = []() {
+ return 1 ? 0 : 1 / 0; // expected-warning{{division by zero is undefined}}
+}();
+
+int f(void) {
+ int x = 1 ? 0 : 1 / 0;
+ return x;
+}
+
+template
+struct X0 {
+ static T value;
+};
+
+template
+struct X1 {
+ static T value;
+};
+
+template
+T X0::value = 3.14; // expected-warning{{implicit conversion from 'double' to 'int' changes value from 3.14 to 3}}
+
+template
+T X1::value = 1 ? 0 : 1 / 0;
+
+template struct X0; // expected-note{{in instantiation of static data member 'X0::value' requested here}}
+
+constexpr
[PATCH] D63889: Check possible warnings on global initializers for reachability
rsmith added inline comments.
Comment at: clang/include/clang/Sema/Sema.h:1684-1686
+ void pushDeclForInitializer(Decl *D) { DeclForInitializer.push_back(D); }
+
+ void popDeclForInitializer() { DeclForInitializer.pop_back(); }
I don't think a simple list of these can work. Consider a case like:
```
auto x = [] {
// something with a runtime diagnostic
};
```
Here, we'll have a `DeclForInitializer` set, so we'll suppress warnings on the
body of the lambda, but we shouldn't: the body of the lambda is a completely
different evaluation context from the initialization of the variable `x`.
Can you store the declaration on the `ExpressionEvaluationContextRecord`
instead of storing a list of them directly on `Sema`? (You shouldn't need a
list there, just a single pointer should work, since we can't parse two nested
initializers without an intervening evaluation context.)
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2015
+
+ bool analyzed = false;
+
Please capitalize the names of variables.
Comment at: clang/lib/Sema/SemaExpr.cpp:16685-16690
if (auto *VD = dyn_cast_or_null(
ExprEvalContexts.back().ManglingContextDecl)) {
if (VD->isConstexpr() ||
(VD->isStaticDataMember() && VD->isFirstDecl() && !VD->isInline()))
break;
+}
Please get rid of the pre-existing hacky use of `ManglingContextDecl` here and
move this into the `getDeclForInitializer` block below.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry marked an inline comment as done.
Nathan-Huckleberry added inline comments.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:352
SetParamDefaultArgument(Param, DefaultArg, EqualLoc);
+ CurContext->removeDecl(Param);
+ CurContext = Cur;
rsmith wrote:
> We may need to delay the diagnostics here until the default argument is
> *used*: if a default argument references a template instantiation, the
> instantiation is not performed until that point, which may mean that our
> semantic checking can't complete correctly until use.
Currently this patch really only works with globals. There are many places
where the following check is made instead of calling `ParseInitializer`. These
should probably be rewritten into a single function and do something similar
with pushing/popping declarations. Not sure if that change should be made in
this patch or not.
```
if (getLangOpts().CPlusPlus11 && Tok.is(tok::l_brace)) {
Diag(Tok, diag::warn_cxx98_compat_generalized_initializer_lists);
DefArgResult = ParseBraceInitializer();
} else
DefArgResult = ParseAssignmentExpression();
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
nickdesaulniers added inline comments.
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2009
+AnalysisDeclContext ,
+SmallVector PUDs) {
+
is `clang` namespace required here?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry updated this revision to Diff 211224.
Nathan-Huckleberry added a comment.
- Style fixes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
Files:
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/AnalysisBasedWarnings.h
clang/include/clang/Sema/Sema.h
clang/lib/Analysis/AnalysisDeclContext.cpp
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Parse/ParseOpenMP.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-unreachable-warning-var-decl.cpp
Index: clang/test/Sema/warn-unreachable-warning-var-decl.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unreachable-warning-var-decl.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -verify %s
+int e = 1 ? 0 : 1 / 0;
+int g = 1 ? 1 / 0 : 0; // expected-warning{{division by zero is undefined}}
+
+#define SHIFT(n) (((n) == 32) ? 0 : ((1 << (n)) - 1))
+
+int x = SHIFT(32);
+int y = SHIFT(0);
+
+// FIXME: Expressions in lambdas aren't ignored
+int z = []() {
+ return 1 ? 0 : 1 / 0; // expected-warning{{division by zero is undefined}}
+}();
+
+int f(void) {
+ int x = 1 ? 0 : 1 / 0;
+ return x;
+}
+
+template
+struct X0 {
+ static T value;
+};
+
+template
+struct X1 {
+ static T value;
+};
+
+template
+T X0::value = 3.14; // expected-warning{{implicit conversion from 'double' to 'int' changes value from 3.14 to 3}}
+
+template
+T X1::value = 1 ? 0 : 1 / 0;
+
+template struct X0; // expected-note{{in instantiation of static data member 'X0::value' requested here}}
+
+constexpr signed char c1 = 100 * 2; // expected-warning{{changes value}}
+constexpr signed char c2 = '\x64' * '\2'; // expected-warning{{changes value}}
+constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4881,6 +4881,8 @@
"default argument expression has capturing blocks?");
}
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
// We already type-checked the argument, so we know it works.
// Just mark all of the declarations in this potentially-evaluated expression
// as being "referenced".
@@ -1,8 +16668,8 @@
case ExpressionEvaluationContext::PotentiallyEvaluated:
case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
- FunctionScopes.back()->PossiblyUnreachableDiags.
-push_back(sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
+ FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
+ PossiblyUnreachableDiag(PD, Loc, Stmts));
return true;
}
@@ -16676,13 +16678,29 @@
// but nonetheless is always required to be a constant expression, so we
// can skip diagnosing.
// FIXME: Using the mangling context here is a hack.
+//
+// Mangling context seems to only be defined on constexpr vardecl that
+// displayed errors.
+// This skips warnings that were already emitted as notes on errors.
if (auto *VD = dyn_cast_or_null(
ExprEvalContexts.back().ManglingContextDecl)) {
if (VD->isConstexpr() ||
(VD->isStaticDataMember() && VD->isFirstDecl() && !VD->isInline()))
break;
- // FIXME: For any other kind of variable, we should build a CFG for its
- // initializer and check whether the context in question is reachable.
+}
+
+// For any other kind of variable, we should build a CFG for its
+// initializer and check whether the context in question is reachable.
+if (auto *VD = dyn_cast_or_null(getDeclForInitializer())) {
+ if (VD->getDefinition()) {
+VD = VD->getDefinition();
+ }
+ // FIXME: Some cases aren't picked up by path analysis currently
+ if (!Stmts.empty() && VD->isFileVarDecl()) {
+AnalysisWarnings.RegisterVarDeclWarning(
+VD, PossiblyUnreachableDiag(PD, Loc, Stmts));
+return true;
+ }
}
Diag(Loc, PD);
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -288,6 +288,9 @@
UnparsedDefaultArgInstantiations.erase(InstPos);
}
+ // Check for delayed warnings
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
return false;
}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -31,6 +31,7 @@
#include "clang/Lex/Lexer.h" // TODO: Extract static
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry updated this revision to Diff 211221.
Nathan-Huckleberry added a comment.
- Style fixes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
Files:
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/AnalysisBasedWarnings.h
clang/include/clang/Sema/Sema.h
clang/lib/Analysis/AnalysisDeclContext.cpp
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Parse/ParseOpenMP.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-unreachable-warning-var-decl.cpp
Index: clang/test/Sema/warn-unreachable-warning-var-decl.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unreachable-warning-var-decl.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -verify %s
+int e = 1 ? 0 : 1 / 0;
+int g = 1 ? 1 / 0 : 0; // expected-warning{{division by zero is undefined}}
+
+#define SHIFT(n) (((n) == 32) ? 0 : ((1 << (n)) - 1))
+
+int x = SHIFT(32);
+int y = SHIFT(0);
+
+// FIXME: Expressions in lambdas aren't ignored
+int z = []() {
+ return 1 ? 0 : 1 / 0; // expected-warning{{division by zero is undefined}}
+}();
+
+int f(void) {
+ int x = 1 ? 0 : 1 / 0;
+ return x;
+}
+
+template
+struct X0 {
+ static T value;
+};
+
+template
+struct X1 {
+ static T value;
+};
+
+template
+T X0::value = 3.14; // expected-warning{{implicit conversion from 'double' to 'int' changes value from 3.14 to 3}}
+
+template
+T X1::value = 1 ? 0 : 1 / 0;
+
+template struct X0; // expected-note{{in instantiation of static data member 'X0::value' requested here}}
+
+constexpr signed char c1 = 100 * 2; // expected-warning{{changes value}}
+constexpr signed char c2 = '\x64' * '\2'; // expected-warning{{changes value}}
+constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4881,6 +4881,8 @@
"default argument expression has capturing blocks?");
}
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
// We already type-checked the argument, so we know it works.
// Just mark all of the declarations in this potentially-evaluated expression
// as being "referenced".
@@ -1,8 +16668,8 @@
case ExpressionEvaluationContext::PotentiallyEvaluated:
case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
- FunctionScopes.back()->PossiblyUnreachableDiags.
-push_back(sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
+ FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
+ PossiblyUnreachableDiag(PD, Loc, Stmts));
return true;
}
@@ -16676,13 +16678,29 @@
// but nonetheless is always required to be a constant expression, so we
// can skip diagnosing.
// FIXME: Using the mangling context here is a hack.
+//
+// Mangling context seems to only be defined on constexpr vardecl that
+// displayed errors.
+// This skips warnings that were already emitted as notes on errors.
if (auto *VD = dyn_cast_or_null(
ExprEvalContexts.back().ManglingContextDecl)) {
if (VD->isConstexpr() ||
(VD->isStaticDataMember() && VD->isFirstDecl() && !VD->isInline()))
break;
- // FIXME: For any other kind of variable, we should build a CFG for its
- // initializer and check whether the context in question is reachable.
+}
+
+// For any other kind of variable, we should build a CFG for its
+// initializer and check whether the context in question is reachable.
+if (auto *VD = dyn_cast_or_null(getDeclForInitializer())) {
+ if (VD->getDefinition()) {
+VD = VD->getDefinition();
+ }
+ // FIXME: Some cases aren't picked up by path analysis currently
+ if (!Stmts.empty() && VD->isFileVarDecl()) {
+AnalysisWarnings.RegisterVarDeclWarning(
+VD, PossiblyUnreachableDiag(PD, Loc, Stmts));
+return true;
+ }
}
Diag(Loc, PD);
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -288,6 +288,9 @@
UnparsedDefaultArgInstantiations.erase(InstPos);
}
+ // Check for delayed warnings
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
return false;
}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -31,6 +31,7 @@
#include "clang/Lex/Lexer.h" // TODO: Extract static
[PATCH] D63889: Check possible warnings on global initializers for reachability
nickdesaulniers added inline comments.
Comment at: clang/include/clang/Sema/Sema.h:1698
+}
+return nullptr;
+ }
Does:
`return DeclForInitializer.empty() ? DeclForInitializer.back() : nullptr;`
fit on one line?
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2036
+ // Can this block be reached from the entrance?
+ if (!cra->isReachable(()->getEntry(), block)) {
+AllReachable = false;
I'm not sure that `analyzed`, `block`, or `cra` follow the naming conventions
used throughout the codebase.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
nickdesaulniers added inline comments.
Comment at: clang/include/clang/Sema/Sema.h:1689
+ void popDeclForInitializer() {
+DeclForInitializer.pop_back();
+ }
might be nice to return the result, but maybe YAGNI?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry updated this revision to Diff 211213.
Nathan-Huckleberry added a comment.
- Add tracking of declaration of initializers in Sema.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
Files:
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/AnalysisBasedWarnings.h
clang/include/clang/Sema/Sema.h
clang/lib/Analysis/AnalysisDeclContext.cpp
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Parse/ParseOpenMP.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-unreachable-warning-var-decl.cpp
Index: clang/test/Sema/warn-unreachable-warning-var-decl.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unreachable-warning-var-decl.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -verify %s
+int e = 1 ? 0 : 1 / 0;
+int g = 1 ? 1 / 0 : 0; // expected-warning{{division by zero is undefined}}
+
+#define SHIFT(n) (((n) == 32) ? 0 : ((1 << (n)) - 1))
+
+int x = SHIFT(32);
+int y = SHIFT(0);
+
+// FIXME: Expressions in lambdas aren't ignored
+int z = []() {
+ return 1 ? 0 : 1 / 0; // expected-warning{{division by zero is undefined}}
+}();
+
+int f(void) {
+ int x = 1 ? 0 : 1 / 0;
+ return x;
+}
+
+template
+struct X0 {
+ static T value;
+};
+
+template
+struct X1 {
+ static T value;
+};
+
+template
+T X0::value = 3.14; // expected-warning{{implicit conversion from 'double' to 'int' changes value from 3.14 to 3}}
+
+template
+T X1::value = 1 ? 0 : 1 / 0;
+
+template struct X0; // expected-note{{in instantiation of static data member 'X0::value' requested here}}
+
+constexpr signed char c1 = 100 * 2; // expected-warning{{changes value}}
+constexpr signed char c2 = '\x64' * '\2'; // expected-warning{{changes value}}
+constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4881,6 +4881,8 @@
"default argument expression has capturing blocks?");
}
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
// We already type-checked the argument, so we know it works.
// Just mark all of the declarations in this potentially-evaluated expression
// as being "referenced".
@@ -1,8 +16668,8 @@
case ExpressionEvaluationContext::PotentiallyEvaluated:
case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
- FunctionScopes.back()->PossiblyUnreachableDiags.
-push_back(sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
+ FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
+ PossiblyUnreachableDiag(PD, Loc, Stmts));
return true;
}
@@ -16676,13 +16678,29 @@
// but nonetheless is always required to be a constant expression, so we
// can skip diagnosing.
// FIXME: Using the mangling context here is a hack.
+//
+// Mangling context seems to only be defined on constexpr vardecl that
+// displayed errors.
+// This skips warnings that were already emitted as notes on errors.
if (auto *VD = dyn_cast_or_null(
ExprEvalContexts.back().ManglingContextDecl)) {
if (VD->isConstexpr() ||
(VD->isStaticDataMember() && VD->isFirstDecl() && !VD->isInline()))
break;
- // FIXME: For any other kind of variable, we should build a CFG for its
- // initializer and check whether the context in question is reachable.
+}
+
+// For any other kind of variable, we should build a CFG for its
+// initializer and check whether the context in question is reachable.
+if (auto *VD = dyn_cast_or_null(getDeclForInitializer())) {
+ if (VD->getDefinition()) {
+VD = VD->getDefinition();
+ }
+ // FIXME: Some cases aren't picked up by path analysis currently
+ if (!Stmts.empty() && VD->isFileVarDecl()) {
+AnalysisWarnings.RegisterVarDeclWarning(
+VD, PossiblyUnreachableDiag(PD, Loc, Stmts));
+return true;
+ }
}
Diag(Loc, PD);
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -288,6 +288,9 @@
UnparsedDefaultArgInstantiations.erase(InstPos);
}
+ // Check for delayed warnings
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
return false;
}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -31,6 +31,7 @@
#include
[PATCH] D63889: Check possible warnings on global initializers for reachability
rsmith added inline comments.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:352
SetParamDefaultArgument(Param, DefaultArg, EqualLoc);
+ CurContext->removeDecl(Param);
+ CurContext = Cur;
We may need to delay the diagnostics here until the default argument is *used*:
if a default argument references a template instantiation, the instantiation is
not performed until that point, which may mean that our semantic checking can't
complete correctly until use.
Comment at: clang/lib/Sema/SemaExpr.cpp:16694
+// initializer and check whether the context in question is reachable.
+if (auto *VD = dyn_cast_or_null(CurContext->getLastDecl())) {
+ if (VD->getDefinition()) {
It's not correct to assume that the last declaration in the context is the
right one to be considering here. Instead, you should track whether you're in a
variable initializer (and for what) in `Sema`. Examples:
```
int x = ([]{}(), x); // in C++; last decl is the lambda
```
```
int y = (struct Q { int n; }){y}; // in C; last decl is the struct
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry updated this revision to Diff 207925.
Nathan-Huckleberry added a comment.
- Stylistic fixes of function names and removal of namespace prefixes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
Files:
clang/include/clang/AST/DeclBase.h
clang/include/clang/Sema/AnalysisBasedWarnings.h
clang/lib/Analysis/AnalysisDeclContext.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-unreachable-warning-var-decl.cpp
Index: clang/test/Sema/warn-unreachable-warning-var-decl.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unreachable-warning-var-decl.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -verify %s
+int e = 1 ? 0 : 1 / 0;
+int g = 1 ? 1 / 0 : 0; // expected-warning{{division by zero is undefined}}
+
+#define SHIFT(n) (((n) == 32) ? 0 : ((1 << (n)) - 1))
+
+int x = SHIFT(32);
+int y = SHIFT(0);
+
+// FIXME: Expressions in lambdas aren't ignored
+int z = []() {
+ return 1 ? 0 : 1 / 0; // expected-warning{{division by zero is undefined}}
+}();
+
+int f(void) {
+ int x = 1 ? 0 : 1 / 0;
+ return x;
+}
+
+template
+struct X0 {
+ static T value;
+};
+
+template
+struct X1 {
+ static T value;
+};
+
+template
+T X0::value = 3.14; // expected-warning{{implicit conversion from 'double' to 'int' changes value from 3.14 to 3}}
+
+template
+T X1::value = 1 ? 0 : 1 / 0;
+
+template struct X0; // expected-note{{in instantiation of static data member 'X0::value' requested here}}
+
+constexpr signed char c1 = 100 * 2; // expected-warning{{changes value}}
+constexpr signed char c2 = '\x64' * '\2'; // expected-warning{{changes value}}
+constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4881,6 +4881,8 @@
"default argument expression has capturing blocks?");
}
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
// We already type-checked the argument, so we know it works.
// Just mark all of the declarations in this potentially-evaluated expression
// as being "referenced".
@@ -1,8 +16668,8 @@
case ExpressionEvaluationContext::PotentiallyEvaluated:
case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
- FunctionScopes.back()->PossiblyUnreachableDiags.
-push_back(sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
+ FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
+ PossiblyUnreachableDiag(PD, Loc, Stmts));
return true;
}
@@ -16676,13 +16678,29 @@
// but nonetheless is always required to be a constant expression, so we
// can skip diagnosing.
// FIXME: Using the mangling context here is a hack.
+//
+// Mangling context seems to only be defined on constexpr vardecl that
+// displayed errors.
+// This skips warnings that were already emitted as notes on errors.
if (auto *VD = dyn_cast_or_null(
ExprEvalContexts.back().ManglingContextDecl)) {
if (VD->isConstexpr() ||
(VD->isStaticDataMember() && VD->isFirstDecl() && !VD->isInline()))
break;
- // FIXME: For any other kind of variable, we should build a CFG for its
- // initializer and check whether the context in question is reachable.
+}
+
+// For any other kind of variable, we should build a CFG for its
+// initializer and check whether the context in question is reachable.
+if (auto *VD = dyn_cast_or_null(CurContext->getLastDecl())) {
+ if (VD->getDefinition()) {
+VD = VD->getDefinition();
+ }
+ // FIXME: Some cases aren't picked up by path analysis currently
+ if (!Stmts.empty() && VD->isFileVarDecl()) {
+AnalysisWarnings.RegisterVarDeclWarning(
+VD, PossiblyUnreachableDiag(PD, Loc, Stmts));
+return true;
+ }
}
Diag(Loc, PD);
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -288,6 +288,9 @@
UnparsedDefaultArgInstantiations.erase(InstPos);
}
+ // Check for delayed warnings
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
return false;
}
@@ -333,7 +336,21 @@
return;
}
+ // Temporarily change the context and add param to it.
+ // Allows DiagRuntimeBehavior to register this param with
+ // any possibly warnings.
+ // Param gets added back to context when function is added
+ // to context.
+ // FIXME: Params should probably be diagnosed
[PATCH] D63889: Check possible warnings on global initializers for reachability
nickdesaulniers added inline comments. Comment at: clang/include/clang/Sema/AnalysisBasedWarnings.h:95 + void flushDiagnostics(SmallVector); + Methods should be UpperCamelCased. Comment at: clang/include/clang/Sema/AnalysisBasedWarnings.h:112 + void + emitPossiblyUnreachableDiags(AnalysisDeclContext , + SmallVector PUDs); UpperCamelCase Comment at: clang/lib/Sema/SemaExpr.cpp:16672 + FunctionScopes.back()->PossiblyUnreachableDiags.push_back( + clang::sema::PossiblyUnreachableDiag(PD, Loc, Stmts)); return true; does this need the `clang::` qualifier? Comment at: clang/lib/Sema/SemaExpr.cpp:16701 +AnalysisWarnings.RegisterVarDeclWarning( +VD, clang::sema::PossiblyUnreachableDiag(PD, Loc, Stmts)); +return true; does this need `clang::`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63889/new/ https://reviews.llvm.org/D63889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry updated this revision to Diff 207918.
Nathan-Huckleberry added a comment.
- Small functional and formatting changes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
Files:
clang/include/clang/AST/DeclBase.h
clang/include/clang/Sema/AnalysisBasedWarnings.h
clang/lib/Analysis/AnalysisDeclContext.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-unreachable-warning-var-decl.cpp
Index: clang/test/Sema/warn-unreachable-warning-var-decl.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unreachable-warning-var-decl.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -verify %s
+int e = 1 ? 0 : 1 / 0;
+int g = 1 ? 1 / 0 : 0; // expected-warning{{division by zero is undefined}}
+
+#define SHIFT(n) (((n) == 32) ? 0 : ((1 << (n)) - 1))
+
+int x = SHIFT(32);
+int y = SHIFT(0);
+
+// FIXME: Expressions in lambdas aren't ignored
+int z = []() {
+ return 1 ? 0 : 1 / 0; // expected-warning{{division by zero is undefined}}
+}();
+
+int f(void) {
+ int x = 1 ? 0 : 1 / 0;
+ return x;
+}
+
+template
+struct X0 {
+ static T value;
+};
+
+template
+struct X1 {
+ static T value;
+};
+
+template
+T X0::value = 3.14; // expected-warning{{implicit conversion from 'double' to 'int' changes value from 3.14 to 3}}
+
+template
+T X1::value = 1 ? 0 : 1 / 0;
+
+template struct X0; // expected-note{{in instantiation of static data member 'X0::value' requested here}}
+
+constexpr signed char c1 = 100 * 2; // expected-warning{{changes value}}
+constexpr signed char c2 = '\x64' * '\2'; // expected-warning{{changes value}}
+constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4881,6 +4881,8 @@
"default argument expression has capturing blocks?");
}
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
// We already type-checked the argument, so we know it works.
// Just mark all of the declarations in this potentially-evaluated expression
// as being "referenced".
@@ -1,8 +16668,8 @@
case ExpressionEvaluationContext::PotentiallyEvaluated:
case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
- FunctionScopes.back()->PossiblyUnreachableDiags.
-push_back(sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
+ FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
+ clang::sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
return true;
}
@@ -16676,13 +16678,29 @@
// but nonetheless is always required to be a constant expression, so we
// can skip diagnosing.
// FIXME: Using the mangling context here is a hack.
+//
+// Mangling context seems to only be defined on constexpr vardecl that
+// displayed errors.
+// This skips warnings that were already emitted as notes on errors.
if (auto *VD = dyn_cast_or_null(
ExprEvalContexts.back().ManglingContextDecl)) {
if (VD->isConstexpr() ||
(VD->isStaticDataMember() && VD->isFirstDecl() && !VD->isInline()))
break;
- // FIXME: For any other kind of variable, we should build a CFG for its
- // initializer and check whether the context in question is reachable.
+}
+
+// For any other kind of variable, we should build a CFG for its
+// initializer and check whether the context in question is reachable.
+if (auto *VD = dyn_cast_or_null(CurContext->getLastDecl())) {
+ if (VD->getDefinition()) {
+VD = VD->getDefinition();
+ }
+ // FIXME: Some cases aren't picked up by path analysis currently
+ if (!Stmts.empty() && VD->isFileVarDecl()) {
+AnalysisWarnings.RegisterVarDeclWarning(
+VD, clang::sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
+return true;
+ }
}
Diag(Loc, PD);
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -288,6 +288,9 @@
UnparsedDefaultArgInstantiations.erase(InstPos);
}
+ // Check for delayed warnings
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
return false;
}
@@ -333,7 +336,21 @@
return;
}
+ // Temporarily change the context and add param to it.
+ // Allows DiagRuntimeBehavior to register this param with
+ // any possibly warnings.
+ // Param gets added back to context when function is added
+ // to context.
+ // FIXME: Params should probably be diagnosed after
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry added inline comments.
Comment at: clang/lib/Analysis/AnalysisDeclContext.cpp:124
+if(VD->hasGlobalStorage()) {
+ return const_cast(dyn_cast(VD->getInit()));
+}
nickdesaulniers wrote:
> The `const_cast` doesn't look necessary here. Is it?
`VD->getInit` returns a `const Expr *`. In order to remove the `const_cast` I
would need to make `getBody()` `const` and every call to `getBody()`. The
change required to add `const` seemed too large to be included in this patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
nickdesaulniers added inline comments.
Comment at: clang/include/clang/Sema/AnalysisBasedWarnings.h:101
+ void RegisterVarDeclWarning(VarDecl *VD, PossiblyUnreachableDiag
+ PossiblyUnreachableDiag);
+
`git-clang-format HEAD~`
The formal parameter should be abbreviated, maybe `PUD`?
Comment at: clang/include/clang/Sema/AnalysisBasedWarnings.h:110
+void emitPossiblyUnreachableDiags(Sema , AnalysisDeclContext ,
+SmallVector PossiblyUnreachableDiags);
How about making this a proper method of `AnalysisBasedWarnings` rather than a
free floating function that's only ever called from other methods of
`AnalysisBasedWarnings`? That way you don't have to pass in a `Sema`.
Comment at: clang/lib/Analysis/AnalysisDeclContext.cpp:124
+if(VD->hasGlobalStorage()) {
+ return const_cast(dyn_cast(VD->getInit()));
+}
The `const_cast` doesn't look necessary here. Is it?
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2012
+void clang::sema::emitPossiblyUnreachableDiags(Sema , AnalysisDeclContext
,
+SmallVector PossiblyUnreachableDiags)
{
having the full namespace spelled out here in the definition smells funny. Is
there a pair of `namespace` blocks below for `clang` and `sema` where the
definition of `emitPossiblyUnreachableDiags` could be moved into?
Actually looking at the file, I see you simply matched the existing style, but
line 49 currently has a `using` statement that should inject the `clang`
namespace into the current scope. That's why you see `sema::` used in other
places in this translation unit without the `clang` namespace prefix. The
whole file should remove the `clang::` prefixes or additionally replace the
`using` statement with explicit `namespace` blocks.
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2260
+void clang::sema::AnalysisBasedWarnings::RegisterVarDeclWarning(VarDecl *VD,
+clang::sema::PossiblyUnreachableDiag PossiblyUnreachableDiag) {
+ VarDeclPossiblyUnreachableDiags[VD].emplace_back(PossiblyUnreachableDiag);
`PUD`
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2275
+
+ clang::sema::emitPossiblyUnreachableDiags(S, AC,
VarDeclPossiblyUnreachableDiags[VD]);
+}
If you make `emitPossiblyUnreachableDiags` then it doesn't need all the
prefixes.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:347
+ // context before the function scope or diagnostics are
+ // delayed until function scope is added
+ DeclContext *Cur = CurContext;
Use punctuation in your comments.
Comment at: clang/lib/Sema/SemaExpr.cpp:16678
+// Mangling context seems to only be defined on constexpr vardecl that
+// displayed errors
+// This skips warnings that were already emitted as notes on errors.
Punctuation.
Comment at: clang/test/Sema/warn-unreachable-warning-var-decl.cpp:39-40
+
+constexpr signed char c1 = 100 * 2; // ok expected-warning{{changes value}}
+constexpr signed char c2 = '\x64' * '\2'; // also ok
expected-warning{{changes value}}
+constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}}
expected-note {{shift count 32 >= width of type}}
`ok` and `also ok` can probably be removed?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
pirama added inline comments.
Comment at: clang/include/clang/Sema/AnalysisBasedWarnings.h:111
+void emitPossiblyUnreachableDiags(Sema , AnalysisDeclContext ,
+SmallVector PossiblyUnreachableDiags);
+
Fix indentation.
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2007
+static void flushDiagnostics(Sema ,
+SmallVector PossiblyUnreachableDiags)
{
+ for (const auto : PossiblyUnreachableDiags)
Should `PossiblyUnreachableDiags` be const?
Also, fix indentation.
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2015
+
+ if (!PossiblyUnreachableDiags.empty()) {
+bool analyzed = false;
How about returning early if `PossiblyUnreachableDiags` here is empty? That'll
avoid putting the entire logic of this function in the `true` branch.
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2031
+ CFGReverseBlockReachabilityAnalysis *cra =
+ AC.getCFGReachablityAnalysis();
+ // FIXME: We should be able to assert that block is non-null, but
`getCFGReachablityAnalysis` has a typo. It's missing an 'i'. Consider fixing
this in a separate patch.
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2051
+
+if (!analyzed)
+ flushDiagnostics(S, PossiblyUnreachableDiags);
Rewrite this as the `else` clause for `if (AC.getCFG())`? In the current
structure, it's not immediately clear that `flushDiagnostics` is called iff
`AC.getCFG()` fails to return a valid CFG.
Upon further reading, this seems to be refactored from another function below
so it probably makes sense to leave it as-is.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63889/new/
https://reviews.llvm.org/D63889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63889: Check possible warnings on global initializers for reachability
Nathan-Huckleberry created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Create CFG for initializers and perform analysis based warnings on global
variables
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D63889
Files:
clang/include/clang/AST/DeclBase.h
clang/include/clang/Sema/AnalysisBasedWarnings.h
clang/lib/Analysis/AnalysisDeclContext.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-unreachable-warning-var-decl.cpp
Index: clang/test/Sema/warn-unreachable-warning-var-decl.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unreachable-warning-var-decl.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -verify %s
+int e = 1 ? 0 : 1/0;
+int g = 1 ? 1/0 : 0;// expected-warning{{division by zero is undefined}}
+
+#define SHIFT(n)(((n) == 32) ? 0 : ((1<<(n))-1))
+
+int x = SHIFT(32);
+int y = SHIFT(0);
+
+// FIXME: Expressions in lambdas aren't ignored
+int z = [](){
+ return 1 ? 0 : 1/0; // expected-warning{{division by zero is undefined}}
+}();
+
+int f(void)
+{
+int x = 1 ? 0 : 1/0;
+return x;
+}
+
+template
+struct X0 {
+static T value;
+};
+
+template
+struct X1 {
+static T value;
+};
+
+template
+T X0::value = 3.14; // expected-warning{{implicit conversion from 'double' to 'int' changes value from 3.14 to 3}}
+
+template
+T X1::value = 1 ? 0 : 1/0;
+
+template struct X0; // expected-note{{in instantiation of static data member 'X0::value' requested here}}
+
+constexpr signed char c1 = 100 * 2; // ok expected-warning{{changes value}}
+constexpr signed char c2 = '\x64' * '\2'; // also ok expected-warning{{changes value}}
+constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4881,6 +4881,8 @@
"default argument expression has capturing blocks?");
}
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
// We already type-checked the argument, so we know it works.
// Just mark all of the declarations in this potentially-evaluated expression
// as being "referenced".
@@ -16662,7 +16664,7 @@
case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
FunctionScopes.back()->PossiblyUnreachableDiags.
-push_back(sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
+push_back(clang::sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
return true;
}
@@ -16671,17 +16673,36 @@
// but nonetheless is always required to be a constant expression, so we
// can skip diagnosing.
// FIXME: Using the mangling context here is a hack.
+//
+// Mangling context seems to only be defined on constexpr vardecl that
+// displayed errors
+// This skips warnings that were already emitted as notes on errors.
if (auto *VD = dyn_cast_or_null(
ExprEvalContexts.back().ManglingContextDecl)) {
if (VD->isConstexpr() ||
(VD->isStaticDataMember() && VD->isFirstDecl() && !VD->isInline()))
break;
+}
+
+// For any other kind of variable, we should build a CFG for its
+// initializer and check whether the context in question is reachable.
+if(auto *VD = dyn_cast_or_null(
+CurContext->getLastDecl())) {
+ if(VD->getDefinition()) {
+VD = VD->getDefinition();
+ }
// FIXME: For any other kind of variable, we should build a CFG for its
// initializer and check whether the context in question is reachable.
+ // Some cases aren't picked up by path analysis currently
+ if(!Stmts.empty() && VD->isFileVarDecl()) {
+AnalysisWarnings.RegisterVarDeclWarning(VD, clang::sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
+return true;
+ }
}
Diag(Loc, PD);
return true;
+
}
return false;
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -288,6 +288,9 @@
UnparsedDefaultArgInstantiations.erase(InstPos);
}
+ // Check for delayed warnings
+ AnalysisWarnings.IssueWarningsForRegisteredVarDecl(Param);
+
return false;
}
@@ -333,7 +336,21 @@
return;
}
+ // Temporarily change the context and add param to it
+ // Allows DiagRuntimeBehavior to register this param with
+ // any possibly warnings
+ // Param gets added back to context when function is added
+ // to context
+ // FIXME: Params should probably be diagnosed after being
+ // actually added to context. Either params get added to
+ // context before the function
