[analyzer][NFC] Remove irrelevant overcomplicated test #147536
Conversation
This commit removes the test file test-member-invalidation.cpp which was recently introduced in 39bc052 by splitting off a test case from new.cpp. In that commit I preserved that test in a slightly modified setting to demonstrate that it wasn't broken by the change; but in this separate commit I'm removing it because I don't think that it "deserves a place" among our tests. The primary issue is that this test examines the values of data members of a deleted object -- which is irrelevant, because code that relies on the value of these members should be reported as a use-after-free bug. (In fact, cplusplus.NewDelete reports it as a use-after-free bug, and the checker family `MallocChecker` sinks the execution path even if that particular frontend is not enabled.) Moreover, a comment claimed that this tests "Invalidate Region even in case of default destructor" while in fact it tested a situaton where the destructor is a plain declared-but-not-defined method. The invalidation of `this` is done by the conservative evaluation, and we don't need this overcomplicated test to validate that very basic behavior.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesThis commit removes the test file test-member-invalidation.cpp which was recently introduced in 39bc052 by splitting off a test case from new.cpp. In that commit I preserved that test in a slightly modified setting to demonstrate that it wasn't broken by the change; but in this separate commit I'm removing it because I don't think that it "deserves a place" among our tests. The primary issue is that this test examines the values of data members of a deleted object -- which is irrelevant, because code that relies on the value of these members should be reported as a use-after-free bug. (In fact, cplusplus.NewDelete reports it as a use-after-free bug, and the checker family Moreover, a comment claimed that this tests "Invalidate Region even in case of default destructor" while in fact it tested a situaton where the destructor is a plain declared-but-not-defined method. The invalidation of Full diff: https://github.com/llvm/llvm-project/pull/147536.diff 2 Files Affected:
diff --git a/clang/test/Analysis/new.cpp b/clang/test/Analysis/new.cpp
index 8439a4e55d812..15c27e7d01308 100644
--- a/clang/test/Analysis/new.cpp
+++ b/clang/test/Analysis/new.cpp
@@ -326,8 +326,3 @@ void testArrayDestr() {
delete[] p;
clang_analyzer_warnIfReached(); // no-warning
}
-
-// See also test-member-invalidation.cpp which validates that calling an
-// unknown destructor invalidates the members of an object. This behavior
-// cannot be tested in this file because here `MallocChecker.cpp` sinks
-// execution paths that refer to members of a deleted object.
diff --git a/clang/test/Analysis/test-member-invalidation.cpp b/clang/test/Analysis/test-member-invalidation.cpp
deleted file mode 100644
index cbff3986325df..0000000000000
--- a/clang/test/Analysis/test-member-invalidation.cpp
+++ /dev/null
@@ -1,47 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify=expected,nosink -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -std=c++11 -verify=expected,sink -analyzer-config eagerly-assume=false %s
-
-// This test validates that calling an unknown destructor invalidates the
-// members of an object. This was originally a part of the test file `new.cpp`,
-// but was split off into a separate file because the checker family
-// implemented in `MallocChecker.cpp` (which is activated via unix.Malloc in
-// `new.cpp` sinks all execution paths that refer to members of a deleted object.
-
-void clang_analyzer_eval(bool);
-
-// Invalidate Region even in case of default destructor
-class InvalidateDestTest {
-public:
- int x;
- int *y;
- ~InvalidateDestTest();
-};
-
-int test_member_invalidation() {
-
- //test invalidation of member variable
- InvalidateDestTest *test = new InvalidateDestTest();
- test->x = 5;
- int *k = &(test->x);
- clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
- delete test;
- clang_analyzer_eval(*k == 5); // nosink-warning{{UNKNOWN}}
-
- //test invalidation of member pointer
- int localVar = 5;
- test = new InvalidateDestTest();
- test->y = &localVar;
- delete test;
- clang_analyzer_eval(localVar == 5); // nosink-warning{{UNKNOWN}}
-
- // Test aray elements are invalidated.
- int Var1 = 5;
- int Var2 = 5;
- InvalidateDestTest *a = new InvalidateDestTest[2];
- a[0].y = &Var1;
- a[1].y = &Var2;
- delete[] a;
- clang_analyzer_eval(Var1 == 5); // nosink-warning{{UNKNOWN}}
- clang_analyzer_eval(Var2 == 5); // nosink-warning{{UNKNOWN}}
- return 0;
-}
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesThis commit removes the test file test-member-invalidation.cpp which was recently introduced in 39bc052 by splitting off a test case from new.cpp. In that commit I preserved that test in a slightly modified setting to demonstrate that it wasn't broken by the change; but in this separate commit I'm removing it because I don't think that it "deserves a place" among our tests. The primary issue is that this test examines the values of data members of a deleted object -- which is irrelevant, because code that relies on the value of these members should be reported as a use-after-free bug. (In fact, cplusplus.NewDelete reports it as a use-after-free bug, and the checker family Moreover, a comment claimed that this tests "Invalidate Region even in case of default destructor" while in fact it tested a situaton where the destructor is a plain declared-but-not-defined method. The invalidation of Full diff: https://github.com/llvm/llvm-project/pull/147536.diff 2 Files Affected:
diff --git a/clang/test/Analysis/new.cpp b/clang/test/Analysis/new.cpp
index 8439a4e55d812..15c27e7d01308 100644
--- a/clang/test/Analysis/new.cpp
+++ b/clang/test/Analysis/new.cpp
@@ -326,8 +326,3 @@ void testArrayDestr() {
delete[] p;
clang_analyzer_warnIfReached(); // no-warning
}
-
-// See also test-member-invalidation.cpp which validates that calling an
-// unknown destructor invalidates the members of an object. This behavior
-// cannot be tested in this file because here `MallocChecker.cpp` sinks
-// execution paths that refer to members of a deleted object.
diff --git a/clang/test/Analysis/test-member-invalidation.cpp b/clang/test/Analysis/test-member-invalidation.cpp
deleted file mode 100644
index cbff3986325df..0000000000000
--- a/clang/test/Analysis/test-member-invalidation.cpp
+++ /dev/null
@@ -1,47 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify=expected,nosink -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -std=c++11 -verify=expected,sink -analyzer-config eagerly-assume=false %s
-
-// This test validates that calling an unknown destructor invalidates the
-// members of an object. This was originally a part of the test file `new.cpp`,
-// but was split off into a separate file because the checker family
-// implemented in `MallocChecker.cpp` (which is activated via unix.Malloc in
-// `new.cpp` sinks all execution paths that refer to members of a deleted object.
-
-void clang_analyzer_eval(bool);
-
-// Invalidate Region even in case of default destructor
-class InvalidateDestTest {
-public:
- int x;
- int *y;
- ~InvalidateDestTest();
-};
-
-int test_member_invalidation() {
-
- //test invalidation of member variable
- InvalidateDestTest *test = new InvalidateDestTest();
- test->x = 5;
- int *k = &(test->x);
- clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
- delete test;
- clang_analyzer_eval(*k == 5); // nosink-warning{{UNKNOWN}}
-
- //test invalidation of member pointer
- int localVar = 5;
- test = new InvalidateDestTest();
- test->y = &localVar;
- delete test;
- clang_analyzer_eval(localVar == 5); // nosink-warning{{UNKNOWN}}
-
- // Test aray elements are invalidated.
- int Var1 = 5;
- int Var2 = 5;
- InvalidateDestTest *a = new InvalidateDestTest[2];
- a[0].y = &Var1;
- a[1].y = &Var2;
- delete[] a;
- clang_analyzer_eval(Var1 == 5); // nosink-warning{{UNKNOWN}}
- clang_analyzer_eval(Var2 == 5); // nosink-warning{{UNKNOWN}}
- return 0;
-}
|
3 participants
This commit removes the test file test-member-invalidation.cpp which was recently introduced in 39bc052 by splitting off a test case from new.cpp.
In that commit I preserved that test in a slightly modified setting to demonstrate that it wasn't broken by the change; but in this separate commit I'm removing it because I don't think that it "deserves a place" among our tests.
The primary issue is that this test examines the values of data members of a deleted object -- which is irrelevant, because code that relies on the value of these members should be reported as a use-after-free bug. (In fact, cplusplus.NewDelete reports it as a use-after-free bug, and the checker family
MallocCheckersinks the execution path even if that particular frontend is not enabled.)Moreover, a comment claimed that this tests "Invalidate Region even in case of default destructor" while in fact it tested a situaton where the destructor is a plain declared-but-not-defined method. The invalidation of
thisis done by the conservative evaluation, and we don't need this overcomplicated test to validate that very basic behavior.