[analyzer] Update the undefined assignment checker diagnostics to not use the term 'garbage' #126596
Conversation
A clang user pointed out that messages for the static analyzer undefined assignment checker use the term 'garbage'. This is kind of snarky and also imprecise. This change replaces the term 'garbage' in those messages with 'not meaningful'. It moves the term 'undefined' to be first in the messages because of the possible ambiguous parsing of the term 'not meaningful and undefined'. That could be parsed as '(not meaningful) and undefined' or 'not (meaningful and undefined'). The use of the term 'meaningless' was considered, but not chosen because it has two meanings in English. One meaning is 'without meaning'. The other meaning is 'having no point'. The 2nd meaning could be construed as indicating the computation could be deleted. rdar://133418644
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: David Tarditi (dtarditi) ChangesA clang user pointed out that messages for the static analyzer undefined assignment checker use the term 'garbage'. This is kind of snarky and also imprecise. This change replaces the term 'garbage' in those messages with 'not meaningful'. It moves the term 'undefined' to be first in the messages because of the possible ambiguous parsing of the term 'not The use of the term 'meaningless' was considered, but not chosen because it has two meanings in English. One meaning is 'without meaning'. The other meaning is 'having no point'. The 2nd meaning could be construed rdar://133418644 Patch is 42.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126596.diff 25 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
index ddc6cc9e8202c7c..bdf705279351496 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
@@ -23,7 +23,7 @@ using namespace ento;
namespace {
class UndefinedAssignmentChecker
: public Checker<check::Bind> {
- const BugType BT{this, "Assigned value is garbage or undefined"};
+ const BugType BT{this, "Assigned value is undefined and not meaningful"};
public:
void checkBind(SVal location, SVal val, const Stmt *S,
@@ -57,8 +57,8 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val,
while (StoreE) {
if (const UnaryOperator *U = dyn_cast<UnaryOperator>(StoreE)) {
- OS << "The expression is an uninitialized value. "
- "The computed value will also be garbage";
+ OS << "The expression is an uninitialized value, so the computed value "
+ << "is not meaningful";
ex = U->getSubExpr();
break;
@@ -68,7 +68,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val,
if (B->isCompoundAssignmentOp()) {
if (C.getSVal(B->getLHS()).isUndef()) {
OS << "The left expression of the compound assignment is an "
- "uninitialized value. The computed value will also be garbage";
+ << "uninitialized value, so the computed value is not meaningful";
ex = B->getLHS();
break;
}
@@ -89,7 +89,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val,
for (auto *I : CD->inits()) {
if (I->getInit()->IgnoreImpCasts() == StoreE) {
OS << "Value assigned to field '" << I->getMember()->getName()
- << "' in implicit constructor is garbage or undefined";
+ << "' in implicit constructor is undefined and not meaningful.";
break;
}
}
diff --git a/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist b/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
index 31b6286b4465e4d..dd731e705c9b0b2 100644
--- a/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ b/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -2578,17 +2578,17 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
- <string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
<key>message</key>
- <string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
</dict>
</array>
- <key>description</key><string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <key>description</key><string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
<key>category</key><string>Logic error</string>
- <key>type</key><string>Assigned value is garbage or undefined</string>
+ <key>type</key><string>Assigned value is undefined and not meaningful</string>
<key>check_name</key><string>core.uninitialized.Assign</string>
<!-- This hash is experimental and going to change! -->
- <key>issue_hash_content_of_line_in_context</key><string>025372576cd3ba6716044f93a51c978c</string>
+ <key>issue_hash_content_of_line_in_context</key><string>936a5fabf36091d0c1e1e1553232d6f5</string>
<key>issue_context_kind</key><string>function</string>
<key>issue_context</key><string>test_objc_fast_enumeration_2</string>
<key>issue_hash_function_offset</key><string>5</string>
diff --git a/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist b/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
index 8b8cc3239bd4bd8..392474d24487c6b 100644
--- a/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ b/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -5864,17 +5864,17 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
- <string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
<key>message</key>
- <string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
</dict>
</array>
- <key>description</key><string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <key>description</key><string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
<key>category</key><string>Logic error</string>
- <key>type</key><string>Assigned value is garbage or undefined</string>
+ <key>type</key><string>Assigned value is undefined and not meaningful</string>
<key>check_name</key><string>core.uninitialized.Assign</string>
<!-- This hash is experimental and going to change! -->
- <key>issue_hash_content_of_line_in_context</key><string>21c774309bdfd487c3d09a61a671bbcc</string>
+ <key>issue_hash_content_of_line_in_context</key><string>c1d7b1284317d7e45bda4bfa6b3a281e</string>
<key>issue_context_kind</key><string>function</string>
<key>issue_context</key><string>test_loop_fast_enumeration</string>
<key>issue_hash_function_offset</key><string>5</string>
diff --git a/clang/test/Analysis/a_flaky_crash.cpp b/clang/test/Analysis/a_flaky_crash.cpp
index f350c1e1280cf3e..5a3fa25f82baa4c 100644
--- a/clang/test/Analysis/a_flaky_crash.cpp
+++ b/clang/test/Analysis/a_flaky_crash.cpp
@@ -14,7 +14,7 @@ bool bar(S);
void foo() {
int x;
if (true && bar(S()))
- ++x; // expected-warning{{The expression is an uninitialized value. The computed value will also be garbage}}
+ ++x; // expected-warning{{The expression is an uninitialized value, so the computed value is not meaningful}}
}
// 256 copies of the same run-line to make it crash more often when it breaks.
diff --git a/clang/test/Analysis/analysis-after-multiple-dtors.cpp b/clang/test/Analysis/analysis-after-multiple-dtors.cpp
index a8f6d38fbd1bbfc..c202752ee1a56b9 100644
--- a/clang/test/Analysis/analysis-after-multiple-dtors.cpp
+++ b/clang/test/Analysis/analysis-after-multiple-dtors.cpp
@@ -23,6 +23,6 @@ int main() {
int x;
int y = x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
(void)y;
}
diff --git a/clang/test/Analysis/array-init-loop.cpp b/clang/test/Analysis/array-init-loop.cpp
index b28468b7f560b2c..cb26bda95abf881 100644
--- a/clang/test/Analysis/array-init-loop.cpp
+++ b/clang/test/Analysis/array-init-loop.cpp
@@ -19,7 +19,7 @@ void array_uninit() {
auto [a, b, c, d, e] = arr;
- int x = e; // expected-warning{{Assigned value is garbage or undefined}}
+ int x = e; // expected-warning{{Assigned value is undefined and not meaningful}}
}
void lambda_init() {
@@ -168,7 +168,7 @@ struct S3_duplicate {
void array_uninit_non_pod() {
S3 arr[1];
- auto [a] = arr; // expected-warning@159{{ in implicit constructor is garbage or undefined }}
+ auto [a] = arr; // expected-warning@159{{ in implicit constructor is undefined and not meaningful}}
}
void lambda_init_non_pod() {
@@ -191,7 +191,7 @@ void lambda_init_non_pod() {
void lambda_uninit_non_pod() {
S3_duplicate arr[4];
- int l = [arr] { return arr[3].i; }(); // expected-warning@164{{ in implicit constructor is garbage or undefined }}
+ int l = [arr] { return arr[3].i; }(); // expected-warning@164{{ in implicit constructor is undefined and not meaningful }}
}
// If this struct is being copy/move constructed by the implicit ctors, ArrayInitLoopExpr
diff --git a/clang/test/Analysis/array-punned-region.c b/clang/test/Analysis/array-punned-region.c
index d319fd7367ec5b0..c997611e2d3a75b 100644
--- a/clang/test/Analysis/array-punned-region.c
+++ b/clang/test/Analysis/array-punned-region.c
@@ -20,7 +20,7 @@ void array_struct_bitfield_1() {
int array_struct_bitfield_2() {
BITFIELD_CAST ff = {0};
BITFIELD_CAST *pff = &ff;
- int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+ int a = *((int *)pff + 2); // expected-warning{{Assigned value is undefined and not meaningful [core.uninitialized.Assign]}}
return a;
}
diff --git a/clang/test/Analysis/builtin_overflow_notes.c b/clang/test/Analysis/builtin_overflow_notes.c
index 20f333a4a6cca56..e8d0e60ad80b046 100644
--- a/clang/test/Analysis/builtin_overflow_notes.c
+++ b/clang/test/Analysis/builtin_overflow_notes.c
@@ -23,8 +23,8 @@ void test_overflow_note(int a, int b)
if (__builtin_add_overflow(a, b, &res)) { // expected-note {{Assuming overflow}}
// expected-note@-1 {{Taking true branch}}
- int var = res; // expected-warning{{Assigned value is garbage or undefined}}
- // expected-note@-1 {{Assigned value is garbage or undefined}}
+ int var = res; // expected-warning{{Assigned value is undefined and not meaningful}}
+ // expected-note@-1 {{Assigned value is undefined and not meaningful}}
return;
}
}
diff --git a/clang/test/Analysis/call-invalidation.cpp b/clang/test/Analysis/call-invalidation.cpp
index fb2b892b31a1f73..96869c5abeabba8 100644
--- a/clang/test/Analysis/call-invalidation.cpp
+++ b/clang/test/Analysis/call-invalidation.cpp
@@ -197,7 +197,7 @@ int testStdCtorDoesNotInvalidateParentObject() {
int testStdCtorDoesNotInvalidateParentObjectSwapped() {
StdWrappingOpaqueSwapped obj;
int x = obj.o.nested_member; // no-garbage: std::Opaque::ctor might initialized this
- int y = obj.uninit; // expected-warning {{Assigned value is garbage or undefined}}
+ int y = obj.uninit; // expected-warning {{Assigned value is undefined and not meaningful}}
return x + y;
}
@@ -277,6 +277,6 @@ struct StdWrappingFancyOpaque {
int testNestedStdNamespacesAndRecords() {
StdWrappingFancyOpaque obj;
int x = obj.o.nested_member; // no-garbage: ctor
- int y = obj.uninit; // expected-warning {{Assigned value is garbage or undefined}}
+ int y = obj.uninit; // expected-warning {{Assigned value is undefined and not meaningful}}
return x + y;
}
diff --git a/clang/test/Analysis/ctor-array.cpp b/clang/test/Analysis/ctor-array.cpp
index 52600b314b010f3..4a35df7834d6abd 100644
--- a/clang/test/Analysis/ctor-array.cpp
+++ b/clang/test/Analysis/ctor-array.cpp
@@ -12,19 +12,19 @@ struct s {
void a1(void) {
s arr[3];
int x = arr[0].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void a2(void) {
s arr[3];
int x = arr[1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void a3(void) {
s arr[3];
int x = arr[2].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
struct s2 {
@@ -37,7 +37,7 @@ void b1(void) {
clang_analyzer_eval(arr[0].y == 2); // expected-warning{{TRUE}}
int x = arr[0].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void b2(void) {
@@ -45,7 +45,7 @@ void b2(void) {
clang_analyzer_eval(arr[1].y == 2); // expected-warning{{TRUE}}
int x = arr[1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void b3(void) {
@@ -53,7 +53,7 @@ void b3(void) {
clang_analyzer_eval(arr[2].y == 2); // expected-warning{{TRUE}}
int x = arr[2].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void c1(void) {
@@ -70,7 +70,7 @@ void c1(void) {
clang_analyzer_eval(arr[1].y == 2); // expected-warning{{TRUE}}
int x = arr[1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
}
@@ -100,7 +100,7 @@ void e1(void) {
clang_analyzer_eval(arr[1].arr[1].y == 2); // expected-warning{{TRUE}}
int x = arr[1].sarr[1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void f1(void) {
@@ -108,7 +108,7 @@ void f1(void) {
clang_analyzer_eval(arr[1][1].y == 2); // expected-warning{{TRUE}}
int x = arr[1][1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
struct s5 {
@@ -168,14 +168,14 @@ void h2(void) {
s a[2][2], b[2][2];
int x = a[1][1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void h3(void) {
s a[2][2], b[2][2];
int x = b[1][1].y;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
struct Base {
diff --git a/clang/test/Analysis/diagnostics/no-store-func-path-notes.m b/clang/test/Analysis/diagnostics/no-store-func-path-notes.m
index 4826b38b98a56b3..c972b85771e5357 100644
--- a/clang/test/Analysis/diagnostics/no-store-func-path-notes.m
+++ b/clang/test/Analysis/diagnostics/no-store-func-path-notes.m
@@ -47,8 +47,8 @@ int initFromBlock(void) {
int p; // expected-note{{'p' declared without an initial value}}
initializer1(&p, 0); // expected-note{{Calling 'initializer1'}}
// expected-note@-1{{Returning from 'initializer1'}}
- z = p; // expected-warning{{Assigned value is garbage or undefined}}
- // expected-note@-1{{Assigned value is garbage or undefined}}
+ z = p; // expected-warning{{Assigned value is undefined and not meaningful}}
+ // expected-note@-1{{Assigned value is undefined and not meaningful}}
}();
return z;
}
diff --git a/clang/test/Analysis/fread.c b/clang/test/Analysis/fread.c
index 8dc998ea1e899d0..61b72122d328e17 100644
--- a/clang/test/Analysis/fread.c
+++ b/clang/test/Analysis/fread.c
@@ -113,9 +113,9 @@ void random_access_read1(int index) {
case 0:
// c[0] is not mutated by fread.
if (success) {
- char p = c[0]; // expected-warning {{Assigned value is garbage or undefined}} We kept the first byte intact.
+ char p = c[0]; // expected-warning {{Assigned value is undefined and not meaningful}} We kept the first byte intact.
} else {
- char p = c[0]; // expected-warning {{Assigned value is garbage or undefined}} We kept the first byte intact.
+ char p = c[0]; // expected-warning {{Assigned value is undefined and not meaningful}} We kept the first byte intact.
}
break;
@@ -147,9 +147,9 @@ void random_access_read1(int index) {
case 3:
// c[3] is not mutated by fread.
if (success) {
- long p = c[3]; // expected-warning {{Assigned value is garbage or undefined}}
+ long p = c[3]; // expected-warning {{Assigned value is undefined and not meaningful}}
} else {
- long p = c[3]; // expected-warning {{Assigned value is garbage or undefined}}
+ long p = c[3]; // expected-warning {{Assigned value is undefined and not meaningful}}
}
break;
}
@@ -169,10 +169,10 @@ void random_access_read2(int b) {
clang_analyzer_isTainted(p); // expected-warning {{YES}}
clang_analyzer_dump(p); // expected-warning {{conj_}}
} else {
- int p = buffer[0]; // expected-warning {{Assigned value is garbage or undefined}}
+ int p = buffer[0]; // expected-warning {{Assigned value is undefined and not meaningful}}
}
} else {
- int p = buffer[0]; // expected-warning {{Assigned value is garbage or undefined}}
+ int p = buffer[0]; // expected-warning {{Assigned value is undefined and not meaningful}}
}
fclose(fp);
}
@@ -283,9 +283,9 @@ void compound_read2(void) {
if (fp) {
struct S s; // s.a is not touched by fread.
if (1 == fread(&s.b, sizeof(s.b), 1, fp)) {
- long p = s.a; // expected-warning {{Assigned value is garbage or undefined}}
+ long p = s.a; // expected-warning {{Assigned value is undefined and not meaningful}}
} else {
- long p = s.a; // expected-warning {{Assigned value is garbage or undefined}}
+ long p = s.a; // expected-warning {{Assigned value is undefined and not meaningful}}
}
fclose(fp);
}
@@ -296,9 +296,9 @@ void var_read(void) {
if (fp) {
int a, b; // 'a' is not touched by fread.
if (1 == fread(&b, sizeof(b), 1, fp)) {
- long p = a; // expected-warning{{Assigned value is garbage or undefined}}
+ long p = a; // expected-warning{{Assigned value is undefined and not meaningful}}
} else {
- long p = a; // expected-warning{{Assigned value is garbage or undefined}}
+ long p = a; // expected-warning{{Assigned value is undefined and not meaningful}}
}
fclose(fp);
}
diff --git a/clang/test/Analysis/implicit-ctor-undef-value.cpp b/clang/test/Analysis/implicit-ctor-undef-value.cpp
index 87824c0533a0a36..f5d805609e5cce6 100644
--- a/clang/test/Analysis/implicit-ctor-undef-value.cpp
+++ b/clang/test/Analysis/implicit-ctor-undef-value.cpp
@@ -9,8 +9,8 @@ struct S {
// Warning is in a weird position because the body of the constructor is
// missing. Specify which field is being assigned.
-class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
- // expected-note@-1{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
+class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is undefined and not meaningful}}
+ // expected-note@-1{{Value assigned to field 'y' in implicit constructor is undefined and not meaningful}}
int x, y;
S s;
@@ -34,8 +34,8 @@ class C {
// It is not necessary to specify which field is being assigned to.
C(const C &c):
x(c.x),
- y(c.y) // expected-warning{{Assigned value is garbage or undefined}}
- // expected-note@-1{{Assigned value is garbage or undefined}}
+ y(c.y) // expected-warning{{Assigned value is undefined and not meaningful}}
+ // expected-note@-1{{Assigned value is undefined and not meaningful}}
{}
};
@@ -53,8 +53,8 @@ struct S {
S(const S &) {}
};
-class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
- // expected-note@-1{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
+class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is undefined and not meaningful}}
+ // expected-note@-1{{Value assigned to field 'y' in implicit constructor is undefined and not meaningful}}
int x, y;
S s;
diff --git a/clang/test/Analysis/initialization.c b/clang/test/Analysis/initialization.c
index d394a902ffeb78c..daa40662477d93c 100644
--- a/clang/test/Analysis/initialization.c
+++ b/clang/test/Analysis/initialization.c
@@ -49,13 +49,13 @@ void glob...
[truncated]
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: David Tarditi (dtarditi) ChangesA clang user pointed out that messages for the static analyzer undefined assignment checker use the term 'garbage'. This is kind of snarky and also imprecise. This change replaces the term 'garbage' in those messages with 'not meaningful'. It moves the term 'undefined' to be first in the messages because of the possible ambiguous parsing of the term 'not The use of the term 'meaningless' was considered, but not chosen because it has two meanings in English. One meaning is 'without meaning'. The other meaning is 'having no point'. The 2nd meaning could be construed rdar://133418644 Patch is 42.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126596.diff 25 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
index ddc6cc9e8202c7c..bdf705279351496 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
@@ -23,7 +23,7 @@ using namespace ento;
namespace {
class UndefinedAssignmentChecker
: public Checker<check::Bind> {
- const BugType BT{this, "Assigned value is garbage or undefined"};
+ const BugType BT{this, "Assigned value is undefined and not meaningful"};
public:
void checkBind(SVal location, SVal val, const Stmt *S,
@@ -57,8 +57,8 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val,
while (StoreE) {
if (const UnaryOperator *U = dyn_cast<UnaryOperator>(StoreE)) {
- OS << "The expression is an uninitialized value. "
- "The computed value will also be garbage";
+ OS << "The expression is an uninitialized value, so the computed value "
+ << "is not meaningful";
ex = U->getSubExpr();
break;
@@ -68,7 +68,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val,
if (B->isCompoundAssignmentOp()) {
if (C.getSVal(B->getLHS()).isUndef()) {
OS << "The left expression of the compound assignment is an "
- "uninitialized value. The computed value will also be garbage";
+ << "uninitialized value, so the computed value is not meaningful";
ex = B->getLHS();
break;
}
@@ -89,7 +89,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val,
for (auto *I : CD->inits()) {
if (I->getInit()->IgnoreImpCasts() == StoreE) {
OS << "Value assigned to field '" << I->getMember()->getName()
- << "' in implicit constructor is garbage or undefined";
+ << "' in implicit constructor is undefined and not meaningful.";
break;
}
}
diff --git a/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist b/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
index 31b6286b4465e4d..dd731e705c9b0b2 100644
--- a/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ b/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -2578,17 +2578,17 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
- <string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
<key>message</key>
- <string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
</dict>
</array>
- <key>description</key><string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <key>description</key><string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
<key>category</key><string>Logic error</string>
- <key>type</key><string>Assigned value is garbage or undefined</string>
+ <key>type</key><string>Assigned value is undefined and not meaningful</string>
<key>check_name</key><string>core.uninitialized.Assign</string>
<!-- This hash is experimental and going to change! -->
- <key>issue_hash_content_of_line_in_context</key><string>025372576cd3ba6716044f93a51c978c</string>
+ <key>issue_hash_content_of_line_in_context</key><string>936a5fabf36091d0c1e1e1553232d6f5</string>
<key>issue_context_kind</key><string>function</string>
<key>issue_context</key><string>test_objc_fast_enumeration_2</string>
<key>issue_hash_function_offset</key><string>5</string>
diff --git a/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist b/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
index 8b8cc3239bd4bd8..392474d24487c6b 100644
--- a/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ b/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -5864,17 +5864,17 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
- <string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
<key>message</key>
- <string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
</dict>
</array>
- <key>description</key><string>The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage</string>
+ <key>description</key><string>The left expression of the compound assignment is an uninitialized value, so the computed value is not meaningful</string>
<key>category</key><string>Logic error</string>
- <key>type</key><string>Assigned value is garbage or undefined</string>
+ <key>type</key><string>Assigned value is undefined and not meaningful</string>
<key>check_name</key><string>core.uninitialized.Assign</string>
<!-- This hash is experimental and going to change! -->
- <key>issue_hash_content_of_line_in_context</key><string>21c774309bdfd487c3d09a61a671bbcc</string>
+ <key>issue_hash_content_of_line_in_context</key><string>c1d7b1284317d7e45bda4bfa6b3a281e</string>
<key>issue_context_kind</key><string>function</string>
<key>issue_context</key><string>test_loop_fast_enumeration</string>
<key>issue_hash_function_offset</key><string>5</string>
diff --git a/clang/test/Analysis/a_flaky_crash.cpp b/clang/test/Analysis/a_flaky_crash.cpp
index f350c1e1280cf3e..5a3fa25f82baa4c 100644
--- a/clang/test/Analysis/a_flaky_crash.cpp
+++ b/clang/test/Analysis/a_flaky_crash.cpp
@@ -14,7 +14,7 @@ bool bar(S);
void foo() {
int x;
if (true && bar(S()))
- ++x; // expected-warning{{The expression is an uninitialized value. The computed value will also be garbage}}
+ ++x; // expected-warning{{The expression is an uninitialized value, so the computed value is not meaningful}}
}
// 256 copies of the same run-line to make it crash more often when it breaks.
diff --git a/clang/test/Analysis/analysis-after-multiple-dtors.cpp b/clang/test/Analysis/analysis-after-multiple-dtors.cpp
index a8f6d38fbd1bbfc..c202752ee1a56b9 100644
--- a/clang/test/Analysis/analysis-after-multiple-dtors.cpp
+++ b/clang/test/Analysis/analysis-after-multiple-dtors.cpp
@@ -23,6 +23,6 @@ int main() {
int x;
int y = x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
(void)y;
}
diff --git a/clang/test/Analysis/array-init-loop.cpp b/clang/test/Analysis/array-init-loop.cpp
index b28468b7f560b2c..cb26bda95abf881 100644
--- a/clang/test/Analysis/array-init-loop.cpp
+++ b/clang/test/Analysis/array-init-loop.cpp
@@ -19,7 +19,7 @@ void array_uninit() {
auto [a, b, c, d, e] = arr;
- int x = e; // expected-warning{{Assigned value is garbage or undefined}}
+ int x = e; // expected-warning{{Assigned value is undefined and not meaningful}}
}
void lambda_init() {
@@ -168,7 +168,7 @@ struct S3_duplicate {
void array_uninit_non_pod() {
S3 arr[1];
- auto [a] = arr; // expected-warning@159{{ in implicit constructor is garbage or undefined }}
+ auto [a] = arr; // expected-warning@159{{ in implicit constructor is undefined and not meaningful}}
}
void lambda_init_non_pod() {
@@ -191,7 +191,7 @@ void lambda_init_non_pod() {
void lambda_uninit_non_pod() {
S3_duplicate arr[4];
- int l = [arr] { return arr[3].i; }(); // expected-warning@164{{ in implicit constructor is garbage or undefined }}
+ int l = [arr] { return arr[3].i; }(); // expected-warning@164{{ in implicit constructor is undefined and not meaningful }}
}
// If this struct is being copy/move constructed by the implicit ctors, ArrayInitLoopExpr
diff --git a/clang/test/Analysis/array-punned-region.c b/clang/test/Analysis/array-punned-region.c
index d319fd7367ec5b0..c997611e2d3a75b 100644
--- a/clang/test/Analysis/array-punned-region.c
+++ b/clang/test/Analysis/array-punned-region.c
@@ -20,7 +20,7 @@ void array_struct_bitfield_1() {
int array_struct_bitfield_2() {
BITFIELD_CAST ff = {0};
BITFIELD_CAST *pff = &ff;
- int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+ int a = *((int *)pff + 2); // expected-warning{{Assigned value is undefined and not meaningful [core.uninitialized.Assign]}}
return a;
}
diff --git a/clang/test/Analysis/builtin_overflow_notes.c b/clang/test/Analysis/builtin_overflow_notes.c
index 20f333a4a6cca56..e8d0e60ad80b046 100644
--- a/clang/test/Analysis/builtin_overflow_notes.c
+++ b/clang/test/Analysis/builtin_overflow_notes.c
@@ -23,8 +23,8 @@ void test_overflow_note(int a, int b)
if (__builtin_add_overflow(a, b, &res)) { // expected-note {{Assuming overflow}}
// expected-note@-1 {{Taking true branch}}
- int var = res; // expected-warning{{Assigned value is garbage or undefined}}
- // expected-note@-1 {{Assigned value is garbage or undefined}}
+ int var = res; // expected-warning{{Assigned value is undefined and not meaningful}}
+ // expected-note@-1 {{Assigned value is undefined and not meaningful}}
return;
}
}
diff --git a/clang/test/Analysis/call-invalidation.cpp b/clang/test/Analysis/call-invalidation.cpp
index fb2b892b31a1f73..96869c5abeabba8 100644
--- a/clang/test/Analysis/call-invalidation.cpp
+++ b/clang/test/Analysis/call-invalidation.cpp
@@ -197,7 +197,7 @@ int testStdCtorDoesNotInvalidateParentObject() {
int testStdCtorDoesNotInvalidateParentObjectSwapped() {
StdWrappingOpaqueSwapped obj;
int x = obj.o.nested_member; // no-garbage: std::Opaque::ctor might initialized this
- int y = obj.uninit; // expected-warning {{Assigned value is garbage or undefined}}
+ int y = obj.uninit; // expected-warning {{Assigned value is undefined and not meaningful}}
return x + y;
}
@@ -277,6 +277,6 @@ struct StdWrappingFancyOpaque {
int testNestedStdNamespacesAndRecords() {
StdWrappingFancyOpaque obj;
int x = obj.o.nested_member; // no-garbage: ctor
- int y = obj.uninit; // expected-warning {{Assigned value is garbage or undefined}}
+ int y = obj.uninit; // expected-warning {{Assigned value is undefined and not meaningful}}
return x + y;
}
diff --git a/clang/test/Analysis/ctor-array.cpp b/clang/test/Analysis/ctor-array.cpp
index 52600b314b010f3..4a35df7834d6abd 100644
--- a/clang/test/Analysis/ctor-array.cpp
+++ b/clang/test/Analysis/ctor-array.cpp
@@ -12,19 +12,19 @@ struct s {
void a1(void) {
s arr[3];
int x = arr[0].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void a2(void) {
s arr[3];
int x = arr[1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void a3(void) {
s arr[3];
int x = arr[2].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
struct s2 {
@@ -37,7 +37,7 @@ void b1(void) {
clang_analyzer_eval(arr[0].y == 2); // expected-warning{{TRUE}}
int x = arr[0].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void b2(void) {
@@ -45,7 +45,7 @@ void b2(void) {
clang_analyzer_eval(arr[1].y == 2); // expected-warning{{TRUE}}
int x = arr[1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void b3(void) {
@@ -53,7 +53,7 @@ void b3(void) {
clang_analyzer_eval(arr[2].y == 2); // expected-warning{{TRUE}}
int x = arr[2].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void c1(void) {
@@ -70,7 +70,7 @@ void c1(void) {
clang_analyzer_eval(arr[1].y == 2); // expected-warning{{TRUE}}
int x = arr[1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
}
@@ -100,7 +100,7 @@ void e1(void) {
clang_analyzer_eval(arr[1].arr[1].y == 2); // expected-warning{{TRUE}}
int x = arr[1].sarr[1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void f1(void) {
@@ -108,7 +108,7 @@ void f1(void) {
clang_analyzer_eval(arr[1][1].y == 2); // expected-warning{{TRUE}}
int x = arr[1][1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
struct s5 {
@@ -168,14 +168,14 @@ void h2(void) {
s a[2][2], b[2][2];
int x = a[1][1].x;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
void h3(void) {
s a[2][2], b[2][2];
int x = b[1][1].y;
- // expected-warning@-1{{Assigned value is garbage or undefined}}
+ // expected-warning@-1{{Assigned value is undefined and not meaningful}}
}
struct Base {
diff --git a/clang/test/Analysis/diagnostics/no-store-func-path-notes.m b/clang/test/Analysis/diagnostics/no-store-func-path-notes.m
index 4826b38b98a56b3..c972b85771e5357 100644
--- a/clang/test/Analysis/diagnostics/no-store-func-path-notes.m
+++ b/clang/test/Analysis/diagnostics/no-store-func-path-notes.m
@@ -47,8 +47,8 @@ int initFromBlock(void) {
int p; // expected-note{{'p' declared without an initial value}}
initializer1(&p, 0); // expected-note{{Calling 'initializer1'}}
// expected-note@-1{{Returning from 'initializer1'}}
- z = p; // expected-warning{{Assigned value is garbage or undefined}}
- // expected-note@-1{{Assigned value is garbage or undefined}}
+ z = p; // expected-warning{{Assigned value is undefined and not meaningful}}
+ // expected-note@-1{{Assigned value is undefined and not meaningful}}
}();
return z;
}
diff --git a/clang/test/Analysis/fread.c b/clang/test/Analysis/fread.c
index 8dc998ea1e899d0..61b72122d328e17 100644
--- a/clang/test/Analysis/fread.c
+++ b/clang/test/Analysis/fread.c
@@ -113,9 +113,9 @@ void random_access_read1(int index) {
case 0:
// c[0] is not mutated by fread.
if (success) {
- char p = c[0]; // expected-warning {{Assigned value is garbage or undefined}} We kept the first byte intact.
+ char p = c[0]; // expected-warning {{Assigned value is undefined and not meaningful}} We kept the first byte intact.
} else {
- char p = c[0]; // expected-warning {{Assigned value is garbage or undefined}} We kept the first byte intact.
+ char p = c[0]; // expected-warning {{Assigned value is undefined and not meaningful}} We kept the first byte intact.
}
break;
@@ -147,9 +147,9 @@ void random_access_read1(int index) {
case 3:
// c[3] is not mutated by fread.
if (success) {
- long p = c[3]; // expected-warning {{Assigned value is garbage or undefined}}
+ long p = c[3]; // expected-warning {{Assigned value is undefined and not meaningful}}
} else {
- long p = c[3]; // expected-warning {{Assigned value is garbage or undefined}}
+ long p = c[3]; // expected-warning {{Assigned value is undefined and not meaningful}}
}
break;
}
@@ -169,10 +169,10 @@ void random_access_read2(int b) {
clang_analyzer_isTainted(p); // expected-warning {{YES}}
clang_analyzer_dump(p); // expected-warning {{conj_}}
} else {
- int p = buffer[0]; // expected-warning {{Assigned value is garbage or undefined}}
+ int p = buffer[0]; // expected-warning {{Assigned value is undefined and not meaningful}}
}
} else {
- int p = buffer[0]; // expected-warning {{Assigned value is garbage or undefined}}
+ int p = buffer[0]; // expected-warning {{Assigned value is undefined and not meaningful}}
}
fclose(fp);
}
@@ -283,9 +283,9 @@ void compound_read2(void) {
if (fp) {
struct S s; // s.a is not touched by fread.
if (1 == fread(&s.b, sizeof(s.b), 1, fp)) {
- long p = s.a; // expected-warning {{Assigned value is garbage or undefined}}
+ long p = s.a; // expected-warning {{Assigned value is undefined and not meaningful}}
} else {
- long p = s.a; // expected-warning {{Assigned value is garbage or undefined}}
+ long p = s.a; // expected-warning {{Assigned value is undefined and not meaningful}}
}
fclose(fp);
}
@@ -296,9 +296,9 @@ void var_read(void) {
if (fp) {
int a, b; // 'a' is not touched by fread.
if (1 == fread(&b, sizeof(b), 1, fp)) {
- long p = a; // expected-warning{{Assigned value is garbage or undefined}}
+ long p = a; // expected-warning{{Assigned value is undefined and not meaningful}}
} else {
- long p = a; // expected-warning{{Assigned value is garbage or undefined}}
+ long p = a; // expected-warning{{Assigned value is undefined and not meaningful}}
}
fclose(fp);
}
diff --git a/clang/test/Analysis/implicit-ctor-undef-value.cpp b/clang/test/Analysis/implicit-ctor-undef-value.cpp
index 87824c0533a0a36..f5d805609e5cce6 100644
--- a/clang/test/Analysis/implicit-ctor-undef-value.cpp
+++ b/clang/test/Analysis/implicit-ctor-undef-value.cpp
@@ -9,8 +9,8 @@ struct S {
// Warning is in a weird position because the body of the constructor is
// missing. Specify which field is being assigned.
-class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
- // expected-note@-1{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
+class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is undefined and not meaningful}}
+ // expected-note@-1{{Value assigned to field 'y' in implicit constructor is undefined and not meaningful}}
int x, y;
S s;
@@ -34,8 +34,8 @@ class C {
// It is not necessary to specify which field is being assigned to.
C(const C &c):
x(c.x),
- y(c.y) // expected-warning{{Assigned value is garbage or undefined}}
- // expected-note@-1{{Assigned value is garbage or undefined}}
+ y(c.y) // expected-warning{{Assigned value is undefined and not meaningful}}
+ // expected-note@-1{{Assigned value is undefined and not meaningful}}
{}
};
@@ -53,8 +53,8 @@ struct S {
S(const S &) {}
};
-class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
- // expected-note@-1{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
+class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is undefined and not meaningful}}
+ // expected-note@-1{{Value assigned to field 'y' in implicit constructor is undefined and not meaningful}}
int x, y;
S s;
diff --git a/clang/test/Analysis/initialization.c b/clang/test/Analysis/initialization.c
index d394a902ffeb78c..daa40662477d93c 100644
--- a/clang/test/Analysis/initialization.c
+++ b/clang/test/Analysis/initialization.c
@@ -49,13 +49,13 @@ void glob...
[truncated]
|
|
@Xazax-hun and @haoNoQ. Could one of you review this or suggest someone else to review it? Thanks. |
|
Hi, just to be sure I understand you correctly, by snarky, you mean "garbage" has a negative co-notation? I'm in general open for changing the diagnostic message, but we should also consider the friction such renames can cause. Usually tools like CodeChecker or other frontends hash the messages, thus if it changes, the suppressions may not work ask expected before and after the rename. /cc @NagyDonat for a CodeChecker opinion |
There was a problem hiding this comment.
LG! There is definitely a cost to changing the diagnostic message as @steakhal pointed out but LLVM in general has the philosophy not being afraid of breaking users to do the right thing. Hopefully, as this is one of the less noisy checkers, the fallout would not be too bad.
There is ample precedent for changing messages that are objectively bad, but you're right that they can break some suppressions. (I don't have an intuition about the impact/severity of this, because I'm the clang analyzer backend guy within the CodeChecker team 😅 .) |
There was a problem hiding this comment.
I think "garbage" is a perfectly acceptable word, I don't think that its "snarkiness" is a problem. (Note that e.g. "garbage collector" is a well-established notion in programming.)
Even if you want to replace it, you need to preserve that in the original phrasing
- "undefined" is a specific term that's defined by the standard (relying on that value is undefined behavior™ aka may launch the nukes if the compiler wants to do so);
- "garbage" represents a variety of other cases that do not trigger undefined behavior™ but are still somehow problematic values that should not be used within a normal code (e.g. values that are "implementation-defined", moved-from objects etc.).
This means that the messages should look like "undefined or X" (instead of your wording, which uses "and").
I think replacing "garbage" with "not meaningful" would be a step backwards. (E.g. for me saying that a value is "not meaningful" would include cases like zeros or 0xDEADBEEF that fill an array just to ensure that it's initialized, but without any semantic meaning.) I'd say that "not meaningful" is also barely acceptable, but I'm casting one vote against it.
|
@steakhal Yes, the term garbage has a negative connotation when used as an adjective in English. By a little snarky, the user meant that the message could be seen as a little overly critical. The user suggested using more neutral terminology. |
|
Yes, obviously "garbage" has negative connotations, but we want to report an error, so we need to have some negative connotations. It's pointless to start an euphemism treadmill because it just introduces churn without actually going forward. I'd suggest pushing back against this request, citing the precedent that "garbage collector" is also an established term. |
|
Welcome!! I completely agree that the message sounds a bit too much like "your code is garbage" and that's not very nice. Here's a tangent suggestion to get a better wording out of this. You can exploit the fact that the static analyzer usually catches undefined values very early. In other words, undefined values typically very short-lived. In particular, they don't travel from one variable to another. Because, well, assigning an undefined value to a variable produces one of those warnings that you're about to patch. And when it does, it terminates the exploration of that execution path entirely. Additionally, if an arithmetic expression produces an undefined value (eg. division by zero), that's an immediate warning too, and this also terminates the execution path immediately. It doesn't even wait for the parent expression to consume the value; it simply terminates the path at the moment of calculation. With that in mind, whenever you actually see an undefined value in a checker, you can safely assume that it was never "computed" or "stored" anywhere. It was simply always there from the start. This is a sufficiently strong contract in the static analyzer; I'm not aware of any long-lived undefined values and I'd consider it a bug if I ever see a long-lived undefined value. I'm pretty sure there are a few assertions that would crash if this ever happened. So you can try to rely on this implicit contract to send a stronger message. For example: - ++x; // expected-warning{{The expression is an uninitialized value. The computed value will also be garbage}}
+ ++x; // expected-warning{{Uninitialized variable incremented}}- int y = x; // expected-warning@-1{{Assigned value is garbage or undefined}}
+ int y = x; // expected-warning@-1{{Value of uninitialized variable assigned}}- b.x |= 1; // expected-warning{{The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage}}
+ b.x |= 1; // expected-warning{{Uninitialized variable used in compound assignment}}(You might want to make a distinction between variables and fields. But even if you don't, it's not unreasonable to simply say "variable" unconditionally, even without looking at the exact expression. You can use the word "location" if you're worried about locations of exotic nature. But you can be fairly certain that it's always going to be a location, not any other source of values.) |
|
@haoNoQ Thank you for the welcome and the helpful information! Your suggestion sounds good to me. I had actually started down this route initially of pointing out the logical error of using a variable before it is initialized. I changed direction because I was not sure where the undefined values were coming from and how they propagate. I prefer to focus on the logical error that a programmer should take action to fix, which in this case is using a variable or data before it is initialized. I've found that programmers are often unfamiliar with standards terms like 'undefined behavior'. |
|
Yes, I think such a change is overdue even outside of our little wording conundrum. And I'm reasonably confident that it's legal to do this even without any extra logic in the checker, with a simple warning text change. Also note that the static analyzer will point at the uninitialized variable itself automatically as part of the full bug report: so you aren't technically required to specify it again in your warning message. (It may still be nice to call out the variable but it would require extra logic.) |
|
The most common "exotic" memory location is probably uninitialized heap memory: https://godbolt.org/z/h9MPzn7EP (We have tests for this right? ...right?) So in this case we need to make sure the user doesn't think that we're talking about the pointer variable (esp. when it's the only variable around). We can also try to simply call it "memory", e.g. "increment over uninitialized memory", or something more verbose of that nature. |
|
How do other tools report this kind of issue? Like gcc or cppcheck. I wonder if we should consider harmonize our message with any of them. |
Ohh, now I see how this could be interpreted in a negative way. I never thought of this. I like where the discussion is heading with emphasizing the use of "uninitialized". |
The message clearly says that the value is garbage 🙄 and not the code or the developer. There is no reason to take this as a personal insult. |
On the other hand, directly saying "uninitialized" could be a good direction because it's more concise and you're right that it's pretty much the only source of
I'd suggest writing "uninitialized memory" to (1) support the common case where the memory comes from |
NagyDonat
dismissed
their stale review
I still think that "garbage" is OK, but the "uninitialized" suggested by @haoNoQ would be a step forward.
|
@haoNoQ I believe the static analyzer can produce undefined values from out-of-bounds memory accesses also. There are some test cases in this change that show that. Some possible error message that I think cover both cases are:
|
|
Yes, right, this is more of a side effect of how we do things, not quite the intended behavior. In particular, if you write something out of bounds first, we will no longer report it as an uninitialized read, even though it's still an out-of-bounds read: https://godbolt.org/z/PYaYa1Yos So either way we've obtained a proof that it wasn't "initialized" through any legal means, and we've even considered some illegal means! (Initializations through formally-unrelated objects don't work this way. For example, if you have stack variables So I'm still mildly in favor of simply saying "uninitialized". It may or may not be someone else's actively used memory but that's beyond the programmer's control and they know it (hopefully). It would be somewhat dishonest to give the user an impression that this checker has considered the object's bounds before emitting a warning. Both the checker and the engine itself effectively ignore allocation bounds for all intents and purposes. |
|
So when it comes to the ArrayBoundChecker, the checker that checks that every access is within bounds, I think that its underlying "model" should be enabled by default, so that to proactively terminate execution paths on which uninitialized accesses happen. It should probably even be considered part of And if this means losing some of those uninitialized read warnings by default, that's probably for the best. Because, well, they work and behave the same way as other ArrayBoundChecker warnings. It is likely that these "uninitialized" read warnings are desired if and only if ArrayBoundChecker warnings are desired. |
I'm seconding this. |
|
@haoNoQ thanks for the explanation! Yes, this makes sense. I agree that the right long-term approach is that the ArrayBoundChecker issues warnings about out-of-bounds accesses. In general, I think it is desirable to report a problem about undefined behavior as close to its cause as possible, using a logical explanation where possible. |
|
We could also go with something like "uninitialized or inaccessible memory" so that to technically cover the OOB case without triggering an immediate visceral reaction to buffer overruns. But it'll probably still be net-negative in confusion compared to a simple "uninitialized". FWIW, if we feel like we're in no position to go for the stronger "uninitialized" message just yet, I'm completely open to landing the initial version of the patch. I think it's even ok to let go of the "and not meaningful" part and simply stick to hard facts: "Assigned value is undefined" period. "Undefined" is the official term so there's no risk of running into a euphemism treadmill.
It is absolutely the goal, yes. But because there's so much emergent behavior in our tool, we don't always have that luxury. It is often very valuable to accurately explain the tool's actual "thought process" first, jump to conclusions later. |
The prior change was using the message 'not meaningful and undefined'. This changes updates it to use the term uninitialized based on PR review feedback and discussion.
|
@haoNoQ I think sticking with uninitialized is good. I've updated the patch with new error messages using that. Please take a look at it and let me know what you think. I think the right path is to issue a more precise error message for out-of-bounds reads. As you point, we could do that by including the bounds checker as part of core checkers. If that checker is too noisy for that, we could create a more limited version that covers the cases that are making it to this checker. |
There was a problem hiding this comment.
LGTM, I agree that "uninitialized" is a better phrasing -- it's shorter and explicitly names the "normal" reason for getting UndefinedVals.
I think the right path is to issue a more precise error message for out-of-bounds reads. As you point, we could do that by including the bounds checker as part of core checkers. If that checker is too noisy for that, we could create a more limited version that covers the cases that are making it to this checker.
Previously the array bound checker was very noisy, but now its "noise level" is comparable to e.g. core.NullDereference, so there is no blocking reason that keeps it out of core.
|
Thanks everyone for the reviews and feedback. I don't have write access for LLVM. @NagyDonat or @haoNoQ, could you merge it on my behalf? Do you want me to condense the change to a single commit as recommended here? Alternately, I could post a draft commit message in a comment for use with GitHub's UI for a squash-merge. |
Please put a draft commit message in a comment, that would be easier for us in a GitHub squash-and-merge. Also note that you can directly edit the PR title and the description (first comment), which are combined by the GitHub UI to form the default commit message for the squash-and-merge (which can be then manually edited). |
|
Thanks - I updated the PR title and description so that it can be used for the commit. It could be a little confusing to read the PR thread with the updated description. Here is the original description, in case anyone reading the thread needs the context.
|
|
Thanks for the update, I'm merging the commit. By the way, Github also has built-in support for viewing the before-the-edit version of the PR description: |
|
@dtarditi Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
Should we try to avoid calling the value uninitialized? Technically you initialize variables (or memory), not values. In the static analyzer terminology in particular, it's somewhat important that values are seen as immutable data that's temporarily associated with mutable memory. I think the rest of the static analyzer does an ok job making this distinction. |
A clang user pointed out that messages for the static analyzer undefined assignment checker use the term ‘garbage’, which might have a negative connotation to some users. This change updates the messages to use the term ‘uninitialized’. This is the usual reason why a value is undefined in the static analyzer and describes the logical error that a programmer should take action to fix.
Out-of-bounds reads can also produce undefined values in the static analyzer. The right long-term design is to have to the array bounds checker cover out-of-bounds reads, so we do not cover that case in the updated messages. The recent improvements to the array bounds checker make it a candidate to add to the core set of checkers.
rdar://133418644