[analyzer] Revert #115918, so empty base class optimization works again (#157480)

[steakhal]

Tldr;
We can't unconditionally trivially copy empty classes because that would clobber the stored entries in the object that the optimized empty class overlaps with.

This regression was introduced by #115918, which introduced other clobbering issues, like the handling of [[no_unique_address]] fields in #137252.

Read issue #157467 for the detailed explanation, but in short, I'd propose reverting the original patch because these was a lot of problems with it for arguably not much gain.
In particular, that patch was motivated by unifying the handling of classes so that copy events would be triggered for a class no matter if it had data members or not.
So in hindsight, it was not worth it.

I plan to backport this to clang-21 as well, and mention in the release notes that this should fix the regression from clang-20.

PS: Also an interesting read D43714 in hindsight.

Fixes #157467
CPP-6574

(cherry picked from commit 38b948b)

[steakhal]

Here is the backport promised in the comment:

I plan to backport this to clang-21 as well, and mention in the release notes that this should fix the regression from clang-20.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Balazs Benics (steakhal)

Changes

Tldr;
We can't unconditionally trivially copy empty classes because that would clobber the stored entries in the object that the optimized empty class overlaps with.

This regression was introduced by #115918, which introduced other clobbering issues, like the handling of [[no_unique_address]] fields in #137252.

Read issue #157467 for the detailed explanation, but in short, I'd propose reverting the original patch because these was a lot of problems with it for arguably not much gain.
In particular, that patch was motivated by unifying the handling of classes so that copy events would be triggered for a class no matter if it had data members or not.
So in hindsight, it was not worth it.

I plan to backport this to clang-21 as well, and mention in the release notes that this should fix the regression from clang-20.

PS: Also an interesting read D43714 in hindsight.

Fixes #157467
CPP-6574

(cherry picked from commit 38b948b)

---

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

5 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+5)
• (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (+22-13)
• (modified) clang/test/Analysis/ctor-trivial-copy.cpp (+3-14)
• (added) clang/test/Analysis/issue-157467.cpp (+39)
• (modified) clang/test/Analysis/taint-generic.cpp (+2-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f03a3273c4518..43529b0f28c3d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1261,6 +1261,11 @@ New features
 Crash and bug fixes
 ^^^^^^^^^^^^^^^^^^^
 
+- Fixed a regression introduced by clang-20 in #GH115918 that lead to false
+  positive reports when ``[[no_unique_address]]`` or empty base class
+  optimization techniques were used. Most notably, some ``std::unique_ptr``
+  implementations. (#GH157467)
+
 - Fixed a crash when C++20 parenthesized initializer lists are used.
   This affected a crash of the well-known lambda overloaded pattern.
   (#GH136041, #GH135665)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 85353848aa124..dc715c7d46d8b 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -71,21 +71,30 @@ void ExprEngine::performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
   Bldr.takeNodes(Pred);
 
   assert(ThisRD);
-  SVal V = Call.getArgSVal(0);
-  const Expr *VExpr = Call.getArgExpr(0);
 
-  // If the value being copied is not unknown, load from its location to get
-  // an aggregate rvalue.
-  if (std::optional<Loc> L = V.getAs<Loc>())
-    V = Pred->getState()->getSVal(*L);
-  else
-    assert(V.isUnknownOrUndef());
+  if (!ThisRD->isEmpty()) {
+    SVal V = Call.getArgSVal(0);
+    const Expr *VExpr = Call.getArgExpr(0);
 
-  ExplodedNodeSet Tmp;
-  evalLocation(Tmp, CallExpr, VExpr, Pred, Pred->getState(), V,
-               /*isLoad=*/true);
-  for (ExplodedNode *N : Tmp)
-    evalBind(Dst, CallExpr, N, ThisVal, V, true);
+    // If the value being copied is not unknown, load from its location to get
+    // an aggregate rvalue.
+    if (std::optional<Loc> L = V.getAs<Loc>())
+      V = Pred->getState()->getSVal(*L);
+    else
+      assert(V.isUnknownOrUndef());
+
+    ExplodedNodeSet Tmp;
+    evalLocation(Tmp, CallExpr, VExpr, Pred, Pred->getState(), V,
+                 /*isLoad=*/true);
+    for (ExplodedNode *N : Tmp)
+      evalBind(Dst, CallExpr, N, ThisVal, V, true);
+  } else {
+    // We can't copy empty classes because of empty base class optimization.
+    // In that case, copying the empty base class subobject would overwrite the
+    // object that it overlaps with - so let's not do that.
+    // See issue-157467.cpp for an example.
+    Dst.Add(Pred);
+  }
 
   PostStmt PS(CallExpr, LCtx);
   for (ExplodedNode *N : Dst) {
diff --git a/clang/test/Analysis/ctor-trivial-copy.cpp b/clang/test/Analysis/ctor-trivial-copy.cpp
index 45c8ca4c51776..940ff9ba3ed9c 100644
--- a/clang/test/Analysis/ctor-trivial-copy.cpp
+++ b/clang/test/Analysis/ctor-trivial-copy.cpp
@@ -46,15 +46,10 @@ void _01_empty_structs() {
   empty Empty = conjure<empty>();
   empty Empty2 = Empty;
   empty Empty3 = Empty2;
-  // All of these should refer to the exact same symbol, because all of
-  // these trivial copies refer to the original conjured value.
-  // There were Unknown before:
-  clang_analyzer_denote(Empty, "$Empty");
-  clang_analyzer_express(Empty);  // expected-warning {{$Empty}}
-  clang_analyzer_express(Empty2); // expected-warning {{$Empty}}
-  clang_analyzer_express(Empty3); // expected-warning {{$Empty}}
 
-  // We should have the same Conjured symbol for "Empty", "Empty2" and "Empty3".
+  // We only have binding for the original Empty object, because copying empty
+  // objects is a no-op in the performTrivialCopy. This is fine, because empty
+  // objects don't have any data members that could be accessed anyway.
   clang_analyzer_printState();
   // CHECK:       "store": { "pointer": "0x{{[0-9a-f]+}}", "items": [
   // CHECK-NEXT:    { "cluster": "GlobalInternalSpaceRegion", "pointer": "0x{{[0-9a-f]+}}", "items": [
@@ -65,12 +60,6 @@ void _01_empty_structs() {
   // CHECK-NEXT:    ]},
   // CHECK-NEXT:    { "cluster": "Empty", "pointer": "0x{{[0-9a-f]+}}", "items": [
   // CHECK-NEXT:      { "kind": "Default", "offset": 0, "value": "[[EMPTY_CONJ:conj_\$[0-9]+{int, LC[0-9]+, S[0-9]+, #[0-9]+}]]" }
-  // CHECK-NEXT:    ]},
-  // CHECK-NEXT:    { "cluster": "Empty2", "pointer": "0x{{[0-9a-f]+}}", "items": [
-  // CHECK-NEXT:      { "kind": "Default", "offset": 0, "value": "[[EMPTY_CONJ]]" }
-  // CHECK-NEXT:    ]},
-  // CHECK-NEXT:    { "cluster": "Empty3", "pointer": "0x{{[0-9a-f]+}}", "items": [
-  // CHECK-NEXT:      { "kind": "Default", "offset": 0, "value": "[[EMPTY_CONJ]]" }
   // CHECK-NEXT:    ]}
   // CHECK-NEXT:  ]},
 
diff --git a/clang/test/Analysis/issue-157467.cpp b/clang/test/Analysis/issue-157467.cpp
new file mode 100644
index 0000000000000..8281ea1ee1aed
--- /dev/null
+++ b/clang/test/Analysis/issue-157467.cpp
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+template <class T, int Idx, bool CanBeEmptyBase = __is_empty(T) && (!__is_final(T))>
+struct compressed_pair_elem {
+  explicit compressed_pair_elem(T u) : value(u) {}
+  T value;
+};
+
+template <class T, int Idx>
+struct compressed_pair_elem<T, Idx, /*CanBeEmptyBase=*/true> : T {
+  explicit compressed_pair_elem(T u) : T(u) {}
+};
+
+template <class T1, class T2, class Base1 = compressed_pair_elem<T1, 0>, class Base2 = compressed_pair_elem<T2, 1>>
+struct compressed_pair : Base1, Base2 {
+  explicit compressed_pair(T1 t1, T2 t2) : Base1(t1), Base2(t2) {}
+};
+
+// empty deleter object
+template <class T>
+struct default_delete {
+  void operator()(T* p) {
+    delete p;
+  }
+};
+
+template <class T, class Deleter = default_delete<T> >
+struct some_unique_ptr {
+  // compressed_pair will employ the empty base class optimization, thus overlapping
+  // the `int*` and the empty `Deleter` object, clobbering the pointer.
+  compressed_pair<int*, Deleter> ptr;
+  some_unique_ptr(int* p, Deleter d) : ptr(p, d) {}
+  ~some_unique_ptr();
+};
+
+void entry_point() {
+  some_unique_ptr<int, default_delete<int> > u3(new int(12), default_delete<int>());
+}
diff --git a/clang/test/Analysis/taint-generic.cpp b/clang/test/Analysis/taint-generic.cpp
index c080313e4d172..fc7c37300d3fc 100644
--- a/clang/test/Analysis/taint-generic.cpp
+++ b/clang/test/Analysis/taint-generic.cpp
@@ -153,8 +153,9 @@ void top() {
   int Int = mySource1<int>();
   clang_analyzer_isTainted(Int); // expected-warning {{YES}}
 
+  // It's fine to not propagate taint to empty classes, since they don't have any data members.
   Empty E = mySource1<Empty>();
-  clang_analyzer_isTainted(E); // expected-warning {{YES}}
+  clang_analyzer_isTainted(E); // expected-warning {{NO}}
 
   Aggr A = mySource1<Aggr>();
   clang_analyzer_isTainted(A);      // expected-warning {{YES}}

[steakhal]

FYI the cherry-pick applied cleanly, then I amended some release notes to that patch. This is how we got this.

[github-actions]

@steakhal (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.