[analyzer][NFC] Reorganize bstring.cpp tests

[gamesh411]

This change eliminates preprocessor-based suppression of test cases by introducing multi-prefix verify options to run-lines. This slightly increases coverage.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Endre Fülöp (gamesh411)

Changes

This change eliminates preprocessor-based suppression of test cases, and moves special cases to separate test files.

---

Full diff: https://github.com/llvm/llvm-project/pull/188709.diff

3 Files Affected:

• (added) clang/test/Analysis/bstring-oob-suppressed.cpp (+92)
• (added) clang/test/Analysis/bstring-uninit-only.cpp (+23)
• (modified) clang/test/Analysis/bstring.cpp (+5-95)

diff --git a/clang/test/Analysis/bstring-oob-suppressed.cpp b/clang/test/Analysis/bstring-oob-suppressed.cpp
new file mode 100644
index 0000000000000..8eb9ad7a08dff
--- /dev/null
+++ b/clang/test/Analysis/bstring-oob-suppressed.cpp
@@ -0,0 +1,92 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:     -analyzer-checker=core \
+// RUN:     -analyzer-checker=unix.cstring \
+// RUN:     -analyzer-checker=unix.Malloc \
+// RUN:     -analyzer-checker=debug.ExprInspection \
+// RUN:     -analyzer-config eagerly-assume=false \
+// RUN:     -analyzer-checker=alpha.unix.cstring.BufferOverlap \
+// RUN:     -analyzer-checker=unix.cstring.NotNullTerminated \
+// RUN:     -verify %s
+
+// These tests exercise memset calls that go out-of-bounds. They are separated
+// from bstring.cpp because they require OutOfBounds to be disabled: with
+// OutOfBounds enabled, the analysis sinks at the OOB memset and the subsequent
+// clang_analyzer_eval calls are not reached.
+//
+// FIXME: These tests document the analyzer's current behavior when OutOfBounds
+// is disabled and OOB memset calls don't terminate the path. Once the
+// OutOfBounds checker is stable enough to always terminate execution at OOB
+// errors (regardless of whether the checker frontend is enabled), these tests
+// should be revisited: the clang_analyzer_eval calls after the OOB memset
+// would become unreachable.
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_eval(int);
+void *memset(void *dest, int ch, std::size_t count);
+
+namespace memset_non_pod {
+class Base {
+public:
+  int b_mem;
+  Base() : b_mem(1) {}
+};
+
+class Derived : public Base {
+public:
+  int d_mem;
+  Derived() : d_mem(2) {}
+};
+
+void memset2_inheritance_field() {
+  Derived d;
+  // FIXME: OOB memset on a derived field with sizeof(Derived).
+  // Current behavior: the not-set part is treated as UNKNOWN.
+  memset(&d.d_mem, 0, sizeof(Derived));
+  clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}}
+}
+
+void memset3_inheritance_field() {
+  Derived d;
+  // FIXME: memset on the base field with sizeof(Derived). This doesn't
+  // actually write past the object's extent, but it's UB because the memset
+  // accesses the object through a pointer to a member, violating aliasing
+  // rules. Current behavior: the field is treated as correctly set to 0.
+  memset(&d.b_mem, 0, sizeof(Derived));
+  clang_analyzer_eval(d.b_mem == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(d.d_mem == 0); // expected-warning{{TRUE}}
+}
+
+class BaseVirtual {
+public:
+  int b_mem;
+  virtual int get() { return 1; }
+};
+
+class DerivedVirtual : public BaseVirtual {
+public:
+  int d_mem;
+};
+
+void memset8_virtual_inheritance_field() {
+  DerivedVirtual d;
+  // FIXME: Same as memset3, but the base has a virtual function. In typical
+  // implementations &d.b_mem differs from &d because the vtable pointer
+  // precedes the first member, so this may also write past the object's
+  // extent.
+  memset(&d.b_mem, 0, sizeof(Derived));
+  clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}}
+}
+} // namespace memset_non_pod
+
+void memset1_new_array() {
+  int *array = new int[10];
+  memset(array, 0, 10 * sizeof(int));
+  clang_analyzer_eval(array[2] == 0); // expected-warning{{TRUE}}
+  // FIXME: OOB memset on a heap array. The analysis continues past it.
+  memset(array + 1, 'a', 10 * sizeof(9));
+  clang_analyzer_eval(array[2] == 0); // expected-warning{{UNKNOWN}}
+  delete[] array;
+}
diff --git a/clang/test/Analysis/bstring-uninit-only.cpp b/clang/test/Analysis/bstring-uninit-only.cpp
new file mode 100644
index 0000000000000..fa78ccc2f4c26
--- /dev/null
+++ b/clang/test/Analysis/bstring-uninit-only.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:     -analyzer-checker=core \
+// RUN:     -analyzer-checker=unix.cstring \
+// RUN:     -analyzer-checker=unix.Malloc \
+// RUN:     -analyzer-checker=debug.ExprInspection \
+// RUN:     -analyzer-config eagerly-assume=false \
+// RUN:     -analyzer-checker=alpha.unix.cstring.UninitializedRead \
+// RUN:     -verify %s
+
+// This test verifies that UninitializedRead produces warnings even when
+// OutOfBounds is disabled. Previously, CheckBufferAccess would early-return
+// before reaching checkInit() when OutOfBounds was disabled, suppressing
+// UninitializedRead as a side effect.
+
+#include "Inputs/system-header-simulator-cxx.h"
+#include "Inputs/system-header-simulator-for-malloc.h"
+
+void memmove_uninit_without_outofbound() {
+  int src[4];
+  int dst[4];
+  memmove(dst, src, sizeof(src)); // expected-warning{{The first element of the 2nd argument is undefined}}
+                                  // expected-note@-1{{Other elements might also be undefined}}
+}
diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp
index 732b36a92ac5a..dabd53d48cb4b 100644
--- a/clang/test/Analysis/bstring.cpp
+++ b/clang/test/Analysis/bstring.cpp
@@ -3,28 +3,14 @@
 // DEFINE:     -analyzer-checker=unix.cstring \
 // DEFINE:     -analyzer-checker=unix.Malloc \
 // DEFINE:     -analyzer-checker=debug.ExprInspection \
+// DEFINE:     -analyzer-checker=alpha.unix.cstring \
 // DEFINE:     -analyzer-config eagerly-assume=false \
 // DEFINE:     -verify %s
 
-// RUN: %{analyzer} \
-// RUN:     -analyzer-checker=alpha.unix.cstring
-
-// RUN: %{analyzer} -DUSE_BUILTINS \
-// RUN:     -analyzer-checker=alpha.unix.cstring
-
-// RUN: %{analyzer} -DVARIANT \
-// RUN:     -analyzer-checker=alpha.unix.cstring
-
-// RUN: %{analyzer} -DUSE_BUILTINS -DVARIANT \
-// RUN:     -analyzer-checker=alpha.unix.cstring
-
-// RUN: %{analyzer} -DSUPPRESS_OUT_OF_BOUND \
-// RUN:     -analyzer-checker=alpha.unix.cstring.BufferOverlap \
-// RUN:     -analyzer-checker=unix.cstring.NotNullTerminated
-
-// RUN: %{analyzer} \
-// RUN:   -DUNINIT_WITHOUT_OUTOFBOUND \
-// RUN:   -analyzer-checker=alpha.unix.cstring.UninitializedRead
+// RUN: %{analyzer}
+// RUN: %{analyzer} -DUSE_BUILTINS
+// RUN: %{analyzer} -DVARIANT
+// RUN: %{analyzer} -DUSE_BUILTINS -DVARIANT
 
 #include "Inputs/system-header-simulator-cxx.h"
 #include "Inputs/system-header-simulator-for-malloc.h"
@@ -122,33 +108,6 @@ void memset1_inheritance() {
   clang_analyzer_eval(d.d_mem == 0); // expected-warning{{TRUE}}
 }
 
-#ifdef SUPPRESS_OUT_OF_BOUND
-void memset2_inheritance_field() {
-  Derived d;
-  // FIXME: This example wrongly calls `memset` on the derived field, with the
-  // size parameter that has the size of the whole derived class. The analysis
-  // should stop at that point as this is UB.
-  // This test asserts the current behavior of treating the not set part as
-  // UNKNOWN.
-  memset(&d.d_mem, 0, sizeof(Derived));
-  clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}}
-}
-
-void memset3_inheritance_field() {
-  Derived d;
-  // FIXME: Here we are setting the field of the base with the size of the
-  // Derived class. By the letter of the standard this is UB, but practically
-  // this only touches memory it is supposed to with the above class
-  // definitions. If we were to be strict the analysis should stop here.
-  // This test asserts the current behavior of nevertheless treating the
-  // wrongly set field as correctly set to 0.
-  memset(&d.b_mem, 0, sizeof(Derived));
-  clang_analyzer_eval(d.b_mem == 0); // expected-warning{{TRUE}}
-  clang_analyzer_eval(d.d_mem == 0); // expected-warning{{TRUE}}
-}
-#endif
-
 void memset4_array_nonpod_object() {
   Derived array[10];
   clang_analyzer_eval(array[1].b_mem == 1); // expected-warning{{UNKNOWN}}
@@ -195,53 +154,4 @@ void memset7_placement_new() {
   clang_analyzer_eval(d->d_mem == 0); // expected-warning{{TRUE}}
 }
 
-class BaseVirtual {
-public:
-  int b_mem;
-  virtual int get() { return 1; }
-};
-
-class DerivedVirtual : public BaseVirtual {
-public:
-  int d_mem;
-};
-
-#ifdef SUPPRESS_OUT_OF_BOUND
-void memset8_virtual_inheritance_field() {
-  DerivedVirtual d;
-  // FIXME: This example wrongly calls `memset` on the derived field, with the
-  // size parameter that has the size of the whole derived class. The analysis
-  // should stop at that point as this is UB. The situation is further
-  // complicated by the fact the base base a virtual function.
-  // This test asserts the current behavior of treating the not set part as
-  // UNKNOWN.
-  memset(&d.b_mem, 0, sizeof(Derived));
-  clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}}
-}
-#endif
 } // namespace memset_non_pod
-
-#ifdef SUPPRESS_OUT_OF_BOUND
-void memset1_new_array() {
-  int *array = new int[10];
-  memset(array, 0, 10 * sizeof(int));
-  clang_analyzer_eval(array[2] == 0); // expected-warning{{TRUE}}
-  // FIXME: The analyzer should stop analysis after memset. Maybe the intent of
-  // this test was to test for this as a desired behaviour, but it shouldn't be.
-  // Going out-of-bounds with memset is a fatal error, even if we decide not to
-  // report it.
-  memset(array + 1, 'a', 10 * sizeof(9));
-  clang_analyzer_eval(array[2] == 0); // expected-warning{{UNKNOWN}}
-  delete[] array;
-}
-#endif
-
-#ifdef UNINIT_WITHOUT_OUTOFBOUND
-void memmove_uninit_without_outofbound() {
-  int src[4];
-  int dst[4];
-  memmove(dst, src, sizeof(src)); // expected-warning{{The first element of the 2nd argument is undefined}}
-                                  // expected-note@-1{{Other elements might also be undefined}}
-}
-#endif

[steakhal]

Before I'd review the change I want to learn more about what do you propose in this patch and what alternatives you have considered?

I have the suspicion that the previously preprocessor macro guarded parts would be split into separate files in this patch - which is kind of the opposite of what I was expecting so far when I wanted to promote the use of different verify prefixes.

[gamesh411]

The purpose is just general code quality improvement.
I wanted to provide an environment for the future development of the string-checkers, where it is easier to see which test expectations should change as a result of stabilising those checkers.

I have considered the following approach first:
23514c5
That one increases test coverage at the cost of introducing some duplication in diagnostics expectations, as we have discussed in #186802.
The current PR makes the tests simpler (technically reducing coverage, but IMO, in practice, coverage stays the same: we only lose the non-header-guarded code's coverage from run-lines that do use those preprocessor symbols). So there is a tradeoff here.
I am not heavily invested in either solution.
@steakhal, if you think that direction is better than this, I'll just update this PR to go that way.

[steakhal]

Do you know that you can pass multiple verify prefixes? This is usually used as -verify=common,fancy and in the next RUN line -verify=common,foobar. Consequently, there is no duplicated test expectation?
Have you considered this?
If so, I'll actually have a look at this PR.

[gamesh411]

I was aware of the multiple prefixes, but it did not seem relevant. I had the impression that I could not just have different prefixes in the same file and remove all of the #ifdef code filtering at the same time, but by adding the negative of the oob prefix, there is a way. Is this more in the direction you thought @steakhal ?

[steakhal]

Yep, this is what I had in mind.
Thank you for revisiting this subject.

[steakhal]

@NagyDonat Please also have a look at this.

[NagyDonat]

LGTM, I like this approach. (Disclaimer: I didn't hunt for typos and subtle errors.)

[steakhal]

LGTM, I like this approach. (Disclaimer: I didn't hunt for typos and subtle errors.)

Let me have a look then before merging.

[steakhal]

LGTM, let's merge this.