[clang] 68ee5ec - [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via cfe-commits]

Author: Marco Antognini
Date: 2022-05-03T11:27:45+02:00
New Revision: 68ee5ec07d4a169baf287acad9ad7fa85d764a22

URL: https://github.com/llvm/llvm-project/commit/68ee5ec07d4a169baf287acad9ad7fa85d764a22
DIFF: https://github.com/llvm/llvm-project/commit/68ee5ec07d4a169baf287acad9ad7fa85d764a22.diff

LOG: [Analyzer] Fix assumptions about const field with member-initializer

Essentially, having a default member initializer for a constant member
does not necessarily imply the member will have the given default value.

Remove part of a2e053638bbf ([analyzer] Treat more const variables and
fields as known contants., 2018-05-04).

Fix #47878

Reviewed By: r.stahl, steakhal

Differential Revision: https://reviews.llvm.org/D124621

Added: 
    clang/test/Analysis/cxx-member-initializer-const-field.cpp

Modified: 
    clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 9d062ddb3e8c1..20b167c9b3b22 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1983,15 +1983,9 @@ SVal RegionStoreManager::getBindingForField(RegionBindingsConstRef B,
   if (const Optional<SVal> &V = B.getDirectBinding(R))
     return *V;
 
-  // Is the field declared constant and has an in-class initializer?
+  // If the containing record was initialized, try to get its constant value.
   const FieldDecl *FD = R->getDecl();
   QualType Ty = FD->getType();
-  if (Ty.isConstQualified())
-    if (const Expr *Init = FD->getInClassInitializer())
-      if (Optional<SVal> V = svalBuilder.getConstantVal(Init))
-        return *V;
-
-  // If the containing record was initialized, try to get its constant value.
   const MemRegion* superR = R->getSuperRegion();
   if (const auto *VR = dyn_cast<VarRegion>(superR)) {
     const VarDecl *VD = VR->getDecl();

diff  --git a/clang/test/Analysis/cxx-member-initializer-const-field.cpp b/clang/test/Analysis/cxx-member-initializer-const-field.cpp
new file mode 100644
index 0000000000000..f0abbddbc4441
--- /dev/null
+++ b/clang/test/Analysis/cxx-member-initializer-const-field.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// This tests false-positive issues related to PR48534.
+//
+// Essentially, having a default member initializer for a constant member does
+// not necessarily imply the member will have the given default value.
+
+struct WithConstructor {
+  int *const ptr = nullptr;
+  WithConstructor(int *x) : ptr(x) {}
+
+  static auto compliant() {
+    WithConstructor c(new int);
+    return *(c.ptr); // no warning
+  }
+
+  static auto compliantWithParam(WithConstructor c) {
+    return *(c.ptr); // no warning
+  }
+
+  static auto issue() {
+    WithConstructor c(nullptr);
+    return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct RegularAggregate {
+  int *const ptr = nullptr;
+
+  static int compliant() {
+    RegularAggregate c{new int};
+    return *(c.ptr); // no warning
+  }
+
+  static int issue() {
+    RegularAggregate c;
+    return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct WithConstructorAndArithmetic {
+  int const i = 0;
+  WithConstructorAndArithmetic(int x) : i(x + 1) {}
+
+  static int compliant(int y) {
+    WithConstructorAndArithmetic c(0);
+    return y / c.i; // no warning
+  }
+
+  static int issue(int y) {
+    WithConstructorAndArithmetic c(-1);
+    return y / c.i; // expected-warning{{Division by zero}}
+  }
+};
+
+struct WithConstructorDeclarationOnly {
+  int const i = 0;
+  WithConstructorDeclarationOnly(int x); // definition not visible.
+
+  static int compliant1(int y) {
+    WithConstructorDeclarationOnly c(0);
+    return y / c.i; // no warning
+  }
+
+  static int compliant2(int y) {
+    WithConstructorDeclarationOnly c(-1);
+    return y / c.i; // no warning
+  }
+};
+
+// NonAggregateFP is not an aggregate (j is a private non-static field) and has no custom constructor.
+// So we know i and j will always be 0 and 42, respectively.
+// That being said, this is not implemented because it is deemed too rare to be worth the complexity.
+struct NonAggregateFP {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+public:
+  static int falsePositive1(NonAggregateFP c) {
+    return 10 / c.i; // FIXME: Currently, no warning.
+  }
+
+  static int falsePositive2(NonAggregateFP c) {
+    return 10 / (c.j - 42); // FIXME: Currently, no warning.
+  }
+};
+
+struct NonAggregate {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+  NonAggregate(NonAggregate const &); // not provided, could set i and j to arbitrary values.
+
+public:
+  static int compliant1(NonAggregate c) {
+    return 10 / c.i; // no warning
+  }
+
+  static int compliant2(NonAggregate c) {
+    return 10 / (c.j - 42); // no warning
+  }
+};
+
+struct WithStaticMember {
+  static int const i = 0;
+
+  static int issue1(WithStaticMember c) {
+    return 10 / c.i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+
+  static int issue2() {
+    return 10 / WithStaticMember::i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+};