[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
joerg added a comment. Please fix the spelling errors in the titel / summary before commit. I somewhat agree with Hal -- I think this is too aggressive. Common use cases for local volatile include atomic ops or returns-twice functions like setjmp/longjmp. Disabling the warning in those cases has a high chance of hiding real problems. I would find it much more useful to move this case into a warning subgroup, so that it can be selectively disabled by command line or pragma. https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
thakis added a comment. In https://reviews.llvm.org/D28543#671953, @hfinkel wrote: > What's the motivation for this? The placement of a local volatile variable is > still under the compiler's direction, and unless the address escapes, we > still assume we can reason about its aliasing (and, thus, whether or not it > is initialized). volatile means that the value can change at any time, which might initialize the variable, no? https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
hfinkel added a comment. What's the motivation for this? The placement of a local volatile variable is still under the compiler's direction, and unless the address escapes, we still assume we can reason about its aliasing (and, thus, whether or not it is initialized). https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
lethalantidote added a comment. Any updates on this? https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Looks great to me, thanks! Unless someone else shouts, I'll land this for you soon. https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
lethalantidote updated this revision to Diff 84045.
lethalantidote added a comment.
Moves check in IsTracked().
https://reviews.llvm.org/D28543
Files:
clang/lib/Analysis/UninitializedValues.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/Sema/uninit-variables.c
Index: clang/test/Sema/uninit-variables.c
===
--- clang/test/Sema/uninit-variables.c
+++ clang/test/Sema/uninit-variables.c
@@ -22,7 +22,7 @@
int test4() {
int x; // expected-note{{initialize the variable 'x' to silence this
warning}}
++x; // expected-warning{{variable 'x' is uninitialized when used here}}
- return x;
+ return x;
}
int test5() {
@@ -442,6 +442,21 @@
struct { struct { void *p; } a; } test55 = { { }}; // no-warning
struct { struct { void *p; } a; } test56 = { { &(test56.a) }}; // no-warning
+int test57() {
+ volatile int x;
+ return x; // no-warning
+}
+
+int init(volatile int* num) {
+ return 1;
+}
+
+int test58() {
+ volatile int a, b, c;
+ if (init() || init() || init()) {}
+ return a+b+c; // no-warning
+}
+
void uninit_in_loop() {
int produce(void);
void consume(int);
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -80,7 +80,7 @@
}
S.Diag(L, diag) << R1 << R2;
-
+
SourceLocation Open = SilenceableCondVal.getBegin();
if (Open.isValid()) {
SourceLocation Close = SilenceableCondVal.getEnd();
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -36,7 +36,8 @@
static bool isTrackedVar(const VarDecl *vd, const DeclContext *dc) {
if (vd->isLocalVarDecl() && !vd->hasGlobalStorage() &&
!vd->isExceptionVariable() && !vd->isInitCapture() &&
- !vd->isImplicit() && vd->getDeclContext() == dc) {
+ !vd->isImplicit() && !vd->getType().isVolatileQualified() &&
+ vd->getDeclContext() == dc) {
QualType ty = vd->getType();
return ty->isScalarType() || ty->isVectorType() || ty->isRecordType();
}
Index: clang/test/Sema/uninit-variables.c
===
--- clang/test/Sema/uninit-variables.c
+++ clang/test/Sema/uninit-variables.c
@@ -22,7 +22,7 @@
int test4() {
int x; // expected-note{{initialize the variable 'x' to silence this warning}}
++x; // expected-warning{{variable 'x' is uninitialized when used here}}
- return x;
+ return x;
}
int test5() {
@@ -442,6 +442,21 @@
struct { struct { void *p; } a; } test55 = { { }}; // no-warning
struct { struct { void *p; } a; } test56 = { { &(test56.a) }}; // no-warning
+int test57() {
+ volatile int x;
+ return x; // no-warning
+}
+
+int init(volatile int* num) {
+ return 1;
+}
+
+int test58() {
+ volatile int a, b, c;
+ if (init() || init() || init()) {}
+ return a+b+c; // no-warning
+}
+
void uninit_in_loop() {
int produce(void);
void consume(int);
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -80,7 +80,7 @@
}
S.Diag(L, diag) << R1 << R2;
-
+
SourceLocation Open = SilenceableCondVal.getBegin();
if (Open.isValid()) {
SourceLocation Close = SilenceableCondVal.getEnd();
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -36,7 +36,8 @@
static bool isTrackedVar(const VarDecl *vd, const DeclContext *dc) {
if (vd->isLocalVarDecl() && !vd->hasGlobalStorage() &&
!vd->isExceptionVariable() && !vd->isInitCapture() &&
- !vd->isImplicit() && vd->getDeclContext() == dc) {
+ !vd->isImplicit() && !vd->getType().isVolatileQualified() &&
+ vd->getDeclContext() == dc) {
QualType ty = vd->getType();
return ty->isScalarType() || ty->isVectorType() || ty->isRecordType();
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
lethalantidote added a comment. So I tried the update you suggested (moving it up into IsTracked) and it seems to work and the reasoning makes sense to me. https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
thakis added a comment. Thanks, this is looking pretty good! From clicking around a bit on cs, do you think it's better to put the check where you have it, or is maybe http://llvm-cs.pcc.me.uk/tools/clang/lib/Analysis/UninitializedValues.cpp#36 more appropriate? I think having it where you have it means saying "we can reason about volatile vars, and we want to treat them as initialized" while the other spot means "we can't reason about volatile variables at all". I don't know which place is better (of someone else reading this knows, please speak up). If the other place makes your test fail, having the check where you have it is probably fine. Else I'd say that moving the check up is probably better. Comment at: clang/lib/Analysis/UninitializedValues.cpp:763 // // int x = x; // No idea what should happen with `volatile int x = x` -- but I've never seen that in practice, so it probably doesn't matter too much either way. https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
lethalantidote updated this revision to Diff 84026.
lethalantidote marked an inline comment as done.
lethalantidote added a comment.
Addresses moving check further up, during analysis.
Adds test to check for sometimes branch. Please review.
https://reviews.llvm.org/D28543
Files:
clang/lib/Analysis/UninitializedValues.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/Sema/uninit-variables.c
Index: clang/test/Sema/uninit-variables.c
===
--- clang/test/Sema/uninit-variables.c
+++ clang/test/Sema/uninit-variables.c
@@ -22,7 +22,7 @@
int test4() {
int x; // expected-note{{initialize the variable 'x' to silence this
warning}}
++x; // expected-warning{{variable 'x' is uninitialized when used here}}
- return x;
+ return x;
}
int test5() {
@@ -442,6 +442,21 @@
struct { struct { void *p; } a; } test55 = { { }}; // no-warning
struct { struct { void *p; } a; } test56 = { { &(test56.a) }}; // no-warning
+int test57() {
+ volatile int x;
+ return x; // no-warning
+}
+
+int init(volatile int* num) {
+ return 1;
+}
+
+int test58() {
+ volatile int a, b, c;
+ if (init() || init() || init()) {}
+ return a+b+c; // no-warning
+}
+
void uninit_in_loop() {
int produce(void);
void consume(int);
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -80,7 +80,7 @@
}
S.Diag(L, diag) << R1 << R2;
-
+
SourceLocation Open = SilenceableCondVal.getBegin();
if (Open.isValid()) {
SourceLocation Close = SilenceableCondVal.getEnd();
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -767,7 +767,7 @@
// appropriately, but we need to continue to analyze subsequent uses
// of the variable.
vals[VD] = Uninitialized;
- } else if (VD->getInit()) {
+ } else if (VD->getInit() || VD->getType().isVolatileQualified()) {
// Treat the new variable as initialized.
vals[VD] = Initialized;
} else {
Index: clang/test/Sema/uninit-variables.c
===
--- clang/test/Sema/uninit-variables.c
+++ clang/test/Sema/uninit-variables.c
@@ -22,7 +22,7 @@
int test4() {
int x; // expected-note{{initialize the variable 'x' to silence this warning}}
++x; // expected-warning{{variable 'x' is uninitialized when used here}}
- return x;
+ return x;
}
int test5() {
@@ -442,6 +442,21 @@
struct { struct { void *p; } a; } test55 = { { }}; // no-warning
struct { struct { void *p; } a; } test56 = { { &(test56.a) }}; // no-warning
+int test57() {
+ volatile int x;
+ return x; // no-warning
+}
+
+int init(volatile int* num) {
+ return 1;
+}
+
+int test58() {
+ volatile int a, b, c;
+ if (init() || init() || init()) {}
+ return a+b+c; // no-warning
+}
+
void uninit_in_loop() {
int produce(void);
void consume(int);
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -80,7 +80,7 @@
}
S.Diag(L, diag) << R1 << R2;
-
+
SourceLocation Open = SilenceableCondVal.getBegin();
if (Open.isValid()) {
SourceLocation Close = SilenceableCondVal.getEnd();
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -767,7 +767,7 @@
// appropriately, but we need to continue to analyze subsequent uses
// of the variable.
vals[VD] = Uninitialized;
- } else if (VD->getInit()) {
+ } else if (VD->getInit() || VD->getType().isVolatileQualified()) {
// Treat the new variable as initialized.
vals[VD] = Initialized;
} else {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
thakis added a comment.
Thanks for the test!
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:907
else
DiagUninitUse(S, VD, Use, true);
}
lethalantidote wrote:
> thakis wrote:
> > Should the check be in DiagUninitUse()? Or is there a reason this one
> > should happen for volatiles?
> Good point. Thanks for catching that. I hoped I understood this use case
> correctly, lemme know if the update makes sense.
Is it possible to test that both branches have this early return now?
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:699
case UninitUse::Always:
+if (VD->getType().isVolatileQualified()) {
+ return;
What about the other cases in this switch? Does a volatile still warn in those
cases? Should it? (Probably not?)
Right now the approach is to add an early return when the warning is emitted.
Maybe this change should instead be somewhere where we compute if a var could
be uninitialized, and that should always be false for volatile variables? Then
future other warnings looking at that bit would get volatiles right for free.
https://reviews.llvm.org/D28543
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
lethalantidote added inline comments. Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:907 else DiagUninitUse(S, VD, Use, true); } thakis wrote: > Should the check be in DiagUninitUse()? Or is there a reason this one should > happen for volatiles? Good point. Thanks for catching that. I hoped I understood this use case correctly, lemme know if the update makes sense. https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
thakis added a comment. Thanks! Can you add a test somewhere? `grep -R W.*uninitialized test/Sema*` will probably show you the existing test for this code. (Another technique: Comment some of the existing tests, run `ninja check-clang`, and see which tests start failing.) You can run an individual test via `bin/llvm-lit ../../llvm/tools/clang/test/Sema/mytest.cc`. Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:907 else DiagUninitUse(S, VD, Use, true); } Should the check be in DiagUninitUse()? Or is there a reason this one should happen for volatiles? https://reviews.llvm.org/D28543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.
lethalantidote created this revision.
lethalantidote added a reviewer: thakis.
lethalantidote added a subscriber: cfe-commits.
Elliminates uninitialized warning for volitile variables.
https://reviews.llvm.org/D28543
Files:
clang/lib/Sema/AnalysisBasedWarnings.cpp
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -864,7 +864,7 @@
/// false.
static bool DiagnoseUninitializedUse(Sema , const VarDecl *VD,
const UninitUse ,
- bool alwaysReportSelfInit = false) {
+ bool alwaysReportSelfInit = false) {
if (const DeclRefExpr *DRE = dyn_cast(Use.getUser())) {
// Inspect the initializer of the variable declaration which is
// being referenced prior to its initialization. We emit
@@ -891,6 +891,11 @@
}
}
+// If volatile, we don't raise this warning.
+if (VD->getType().isVolatileQualified()) {
+ return false;
+}
+
DiagUninitUse(S, VD, Use, false);
} else {
const BlockExpr *BE = cast(Use.getUser());
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -864,7 +864,7 @@
/// false.
static bool DiagnoseUninitializedUse(Sema , const VarDecl *VD,
const UninitUse ,
- bool alwaysReportSelfInit = false) {
+ bool alwaysReportSelfInit = false) {
if (const DeclRefExpr *DRE = dyn_cast(Use.getUser())) {
// Inspect the initializer of the variable declaration which is
// being referenced prior to its initialization. We emit
@@ -891,6 +891,11 @@
}
}
+// If volatile, we don't raise this warning.
+if (VD->getType().isVolatileQualified()) {
+ return false;
+}
+
DiagUninitUse(S, VD, Use, false);
} else {
const BlockExpr *BE = cast(Use.getUser());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
