[PATCH] D53713: Add extension to always default-initialize nullptr_t.
rsmith added inline comments.
Comment at: test/Analysis/nullptr.cpp:136
-invokeF(p); // expected-warning{{1st function call argument is an
uninitialized value}}
-// expected-note@-1{{1st function call argument is an
uninitialized value}}
if (getSymbol()) // expected-note {{Assuming the condition is false}}
rsmith wrote:
> This bug is already fixed in trunk.
Never mind, the fix was reverted in r346065.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53713/new/
https://reviews.llvm.org/D53713
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
erichkeane added a comment. As stated in CFE commits (in response to Richard's comments): `I’ll push a revert in a few minutes and give it another try. Perhaps I can suppress the creation of a CK_LValueToRValue in the case where it’s a read from nullptr_t Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53713/new/ https://reviews.llvm.org/D53713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
rsmith added a comment.
I'm concerned that this will hide real bugs by suppressing the bogus warnings
in (only) the simple cases. There is no "read a value from memory" operation on
type `nullptr_t` (just like for, say, class types), and any warning that
believes there is is incorrect. For example, given
alignas(nullptr_t) char buffer[sizeof(nullptr_t)];
nullptr_t *p = (nullptr_t*)buffer;
nullptr_t q = *p;
... there is no uninitialized use, but this patch will do nothing to make
static analysis (etc) aware of that.
I think the real problem here is that our AST representation is wrong. We use
`CK_LValueToRValue` to model a read from memory, but we also use it in examples
like the above where there is no read from memory.
Comment at: test/Analysis/nullptr.cpp:136
-invokeF(p); // expected-warning{{1st function call argument is an
uninitialized value}}
-// expected-note@-1{{1st function call argument is an
uninitialized value}}
if (getSymbol()) // expected-note {{Assuming the condition is false}}
This bug is already fixed in trunk.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53713/new/
https://reviews.llvm.org/D53713
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC349201: Add extension to always default-initialize
nullptr_t. (authored by erichkeane, committed by ).
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53713/new/
https://reviews.llvm.org/D53713
Files:
lib/Sema/SemaInit.cpp
test/Analysis/nullptr.cpp
test/SemaCXX/ast-print-crash.cpp
test/SemaCXX/nullptr_t-init.cpp
Index: test/Analysis/nullptr.cpp
===
--- test/Analysis/nullptr.cpp
+++ test/Analysis/nullptr.cpp
@@ -125,21 +125,16 @@
};
void shouldNotCrash() {
- decltype(nullptr) p; // expected-note{{'p' declared without an initial
value}}
- if (getSymbol()) // expected-note {{Assuming the condition is false}}
- // expected-note@-1{{Taking false branch}}
- // expected-note@-2{{Assuming the condition is false}}
- // expected-note@-3{{Taking false branch}}
- // expected-note@-4{{Assuming the condition is true}}
- // expected-note@-5{{Taking true branch}}
-invokeF(p); // expected-warning{{1st function call argument is an
uninitialized value}}
-// expected-note@-1{{1st function call argument is an
uninitialized value}}
+ decltype(nullptr) p; // expected-note{{'p' initialized to a null pointer
value}}
if (getSymbol()) // expected-note {{Assuming the condition is false}}
// expected-note@-1{{Taking false branch}}
// expected-note@-2{{Assuming the condition is true}}
// expected-note@-3{{Taking true branch}}
-invokeF(nullptr); // expected-note {{Calling 'invokeF'}}
- // expected-note@-1{{Passing null pointer value via 1st
parameter 'x'}}
+invokeF(p); // expected-note{{Passing null pointer value via 1st parameter
'x'}}
+// expected-note@-1{{Calling 'invokeF'}}
+ if (getSymbol()) // expected-note {{Assuming the condition is false}}
+ // expected-note@-1{{Taking false branch}}
+invokeF(nullptr);
if (getSymbol()) { // expected-note {{Assuming the condition is true}}
// expected-note@-1{{Taking true branch}}
X *xx = Type().x; // expected-note {{Null pointer value stored to field
'x'}}
Index: test/SemaCXX/ast-print-crash.cpp
===
--- test/SemaCXX/ast-print-crash.cpp
+++ test/SemaCXX/ast-print-crash.cpp
@@ -7,6 +7,6 @@
// CHECK: struct {
// CHECK-NEXT: } dont_crash_on_syntax_error;
-// CHECK-NEXT: decltype(nullptr) p;
+// CHECK-NEXT: decltype(nullptr) p(/*implicit*/(decltype(nullptr))0);
struct {
} dont_crash_on_syntax_error /* missing ; */ decltype(nullptr) p;
Index: test/SemaCXX/nullptr_t-init.cpp
===
--- test/SemaCXX/nullptr_t-init.cpp
+++ test/SemaCXX/nullptr_t-init.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -ffreestanding
-Wuninitialized %s
+// expected-no-diagnostics
+typedef decltype(nullptr) nullptr_t;
+
+// Ensure no 'uninitialized when used here' warnings (Wuninitialized), for
+// nullptr_t always-initialized extension.
+nullptr_t default_init() {
+ nullptr_t a;
+ return a;
+}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -4881,6 +4881,13 @@
return;
}
+ // As an extension, and to fix Core issue 1013, zero initialize nullptr_t.
+ // Since there is only 1 valid value of nullptr_t, we can just use that.
+ if (DestType->isNullPtrType()) {
+Sequence.AddZeroInitializationStep(Entity.getType());
+return;
+ }
+
// - otherwise, no initialization is performed.
// If a program calls for the default initialization of an object of
Index: test/Analysis/nullptr.cpp
===
--- test/Analysis/nullptr.cpp
+++ test/Analysis/nullptr.cpp
@@ -125,21 +125,16 @@
};
void shouldNotCrash() {
- decltype(nullptr) p; // expected-note{{'p' declared without an initial value}}
- if (getSymbol()) // expected-note {{Assuming the condition is false}}
- // expected-note@-1{{Taking false branch}}
- // expected-note@-2{{Assuming the condition is false}}
- // expected-note@-3{{Taking false branch}}
- // expected-note@-4{{Assuming the condition is true}}
- // expected-note@-5{{Taking true branch}}
-invokeF(p); // expected-warning{{1st function call argument is an uninitialized value}}
-// expected-note@-1{{1st function call argument is an uninitialized value}}
+ decltype(nullptr) p; // expected-note{{'p' initialized to a null pointer value}}
if (getSymbol()) // expected-note
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I approve this for Static Analyzer. As long as this is the correct thing to do in the compiler, Static Analyzer should ideally behave similarly to the compiler. Exposing portability bugs that would show up on other compilers is generally less important than behaving similarly to Clang itself. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53713/new/ https://reviews.llvm.org/D53713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
aaron.ballman added reviewers: NoQ, george.karpenkov. aaron.ballman added a comment. Adding some reviewers for the static analyzer questions raised. I think this is a reasonable approach, but are there situations where we should diagnose this as an extension? I can't think of one, but I figured the question should still be asked. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53713/new/ https://reviews.llvm.org/D53713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
erichkeane added a comment. Is there any feedback here? Am I just completely incorrect in how I tried to fix this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53713/new/ https://reviews.llvm.org/D53713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
erichkeane updated this revision to Diff 171128.
erichkeane added a comment.
Update the 2 failing tests. Would love some help looking at the Analysis fix,
it seems to no longer have to 'assume' stuff due to the this now being defined
behavior.
Additionally, the other test is just an AST change, but seems to be exactly
what I'm suggesting as an extension here.
https://reviews.llvm.org/D53713
Files:
lib/Sema/SemaInit.cpp
test/Analysis/nullptr.cpp
test/SemaCXX/ast-print-crash.cpp
test/SemaCXX/nullptr_t-init.cpp
Index: test/SemaCXX/nullptr_t-init.cpp
===
--- /dev/null
+++ test/SemaCXX/nullptr_t-init.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -ffreestanding
-Wuninitialized %s
+// expected-no-diagnostics
+typedef decltype(nullptr) nullptr_t;
+
+// Ensure no 'uninitialized when used here' warnings (Wuninitialized), for
+// nullptr_t always-initialized extension.
+nullptr_t default_init() {
+ nullptr_t a;
+ return a;
+}
Index: test/SemaCXX/ast-print-crash.cpp
===
--- test/SemaCXX/ast-print-crash.cpp
+++ test/SemaCXX/ast-print-crash.cpp
@@ -7,6 +7,6 @@
// CHECK: struct {
// CHECK-NEXT: } dont_crash_on_syntax_error;
-// CHECK-NEXT: decltype(nullptr) p;
+// CHECK-NEXT: decltype(nullptr) p(/*implicit*/(decltype(nullptr))0);
struct {
} dont_crash_on_syntax_error /* missing ; */ decltype(nullptr) p;
Index: test/Analysis/nullptr.cpp
===
--- test/Analysis/nullptr.cpp
+++ test/Analysis/nullptr.cpp
@@ -125,21 +125,16 @@
};
void shouldNotCrash() {
- decltype(nullptr) p; // expected-note{{'p' declared without an initial
value}}
- if (getSymbol()) // expected-note {{Assuming the condition is false}}
- // expected-note@-1{{Taking false branch}}
- // expected-note@-2{{Assuming the condition is false}}
- // expected-note@-3{{Taking false branch}}
- // expected-note@-4{{Assuming the condition is true}}
- // expected-note@-5{{Taking true branch}}
-invokeF(p); // expected-warning{{1st function call argument is an
uninitialized value}}
-// expected-note@-1{{1st function call argument is an
uninitialized value}}
+ decltype(nullptr) p; // expected-note{{'p' initialized to a null pointer
value}}
if (getSymbol()) // expected-note {{Assuming the condition is false}}
// expected-note@-1{{Taking false branch}}
// expected-note@-2{{Assuming the condition is true}}
// expected-note@-3{{Taking true branch}}
-invokeF(nullptr); // expected-note {{Calling 'invokeF'}}
- // expected-note@-1{{Passing null pointer value via 1st
parameter 'x'}}
+invokeF(p); // expected-note{{Passing null pointer value via 1st parameter
'x'}}
+// expected-note@-1{{Calling 'invokeF'}}
+ if (getSymbol()) // expected-note {{Assuming the condition is false}}
+ // expected-note@-1{{Taking false branch}}
+invokeF(nullptr);
if (getSymbol()) { // expected-note {{Assuming the condition is true}}
// expected-note@-1{{Taking true branch}}
X *xx = Type().x; // expected-note {{Null pointer value stored to field
'x'}}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -4877,6 +4877,13 @@
return;
}
+ // As an extension, and to fix Core issue 1013, zero initialize nullptr_t.
+ // Since there is only 1 valid value of nullptr_t, we can just use that.
+ if (DestType->isNullPtrType()) {
+Sequence.AddZeroInitializationStep(Entity.getType());
+return;
+ }
+
// - otherwise, no initialization is performed.
// If a program calls for the default initialization of an object of
Index: test/SemaCXX/nullptr_t-init.cpp
===
--- /dev/null
+++ test/SemaCXX/nullptr_t-init.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -ffreestanding -Wuninitialized %s
+// expected-no-diagnostics
+typedef decltype(nullptr) nullptr_t;
+
+// Ensure no 'uninitialized when used here' warnings (Wuninitialized), for
+// nullptr_t always-initialized extension.
+nullptr_t default_init() {
+ nullptr_t a;
+ return a;
+}
Index: test/SemaCXX/ast-print-crash.cpp
===
--- test/SemaCXX/ast-print-crash.cpp
+++ test/SemaCXX/ast-print-crash.cpp
@@ -7,6 +7,6 @@
// CHECK: struct {
// CHECK-NEXT: } dont_crash_on_syntax_error;
-// CHECK-NEXT: decltype(nullptr) p;
+// CHECK-NEXT: decltype(nullptr) p(/*implicit*/(decltype(nullptr))0);
struct {
} dont_crash_on_syntax_error /* missing ; */
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
erichkeane added a comment. Woops, apparently a couple of tests fail on this that I somehow missed the 1st time. Looking into it. Repository: rC Clang https://reviews.llvm.org/D53713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53713: Add extension to always default-initialize nullptr_t.
erichkeane created this revision.
erichkeane added reviewers: rsmith, aaron.ballman.
Core issue 1013 suggests that having an uninitialied std::nullptr_t be
UB is a bit foolish, since there is only a single valid value. This DR
reports that DR616 fixes it, which does so by making lvalue-to-rvalue
conversions from nullptr_t be equal to nullptr.
However, just implementing that results in warnings/etc in many places.
In order to fix all situations where nullptr_t would seem uninitialized,
this patch instead (as an otherwise transparent extension) default
initializes uninitialized VarDecls of nullptr_t.
Repository:
rC Clang
https://reviews.llvm.org/D53713
Files:
lib/Sema/SemaInit.cpp
test/SemaCXX/nullptr_t-init.cpp
Index: test/SemaCXX/nullptr_t-init.cpp
===
--- /dev/null
+++ test/SemaCXX/nullptr_t-init.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -ffreestanding
-Wuninitialized %s
+// expected-no-diagnostics
+typedef decltype(nullptr) nullptr_t;
+
+// Ensure no 'uninitialized when used here' warnings (Wuninitialized), for
+// nullptr_t always-initialized extension.
+nullptr_t default_init() {
+ nullptr_t a;
+ return a;
+}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -4877,6 +4877,13 @@
return;
}
+ // As an extension, and to fix Core issue 1013, zero initialize nullptr_t.
+ // Since there is only 1 valid value of nullptr_t, we can just use that.
+ if (DestType->isNullPtrType()) {
+Sequence.AddZeroInitializationStep(Entity.getType());
+return;
+ }
+
// - otherwise, no initialization is performed.
// If a program calls for the default initialization of an object of
Index: test/SemaCXX/nullptr_t-init.cpp
===
--- /dev/null
+++ test/SemaCXX/nullptr_t-init.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -ffreestanding -Wuninitialized %s
+// expected-no-diagnostics
+typedef decltype(nullptr) nullptr_t;
+
+// Ensure no 'uninitialized when used here' warnings (Wuninitialized), for
+// nullptr_t always-initialized extension.
+nullptr_t default_init() {
+ nullptr_t a;
+ return a;
+}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -4877,6 +4877,13 @@
return;
}
+ // As an extension, and to fix Core issue 1013, zero initialize nullptr_t.
+ // Since there is only 1 valid value of nullptr_t, we can just use that.
+ if (DestType->isNullPtrType()) {
+Sequence.AddZeroInitializationStep(Entity.getType());
+return;
+ }
+
// - otherwise, no initialization is performed.
// If a program calls for the default initialization of an object of
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
