From eda2eaabf2949c08ba94c92b9aad6fccb3c8eaa2 Mon Sep 17 00:00:00 2001
From: Jun Zhang <jun@junz.org>
Date: Thu, 29 Dec 2022 14:40:40 +0800
Subject: [PATCH] [clang][dataflow] Fix crash when having boolean-to-integral
 casts.

Since now we just ignore all (implicit) integral casts, treating the
resulting value as the same as the underlying value, it could cause
inconsistency between values after `Join` if in some paths the type
doesn't strictly match. This could cause intermittent crashes.

std::optional<bool> o;
int x;
if (o.has_value()) {
  x = o.value();
}

Fixes: https://github.com/llvm/llvm-project/issues/59728

Signed-off-by: Jun Zhang <jun@junz.org>

Differential Revision: https://reviews.llvm.org/D140753
---
 .../lib/Analysis/FlowSensitive/DataflowEnvironment.cpp  | 14 +++++++++++++-
 .../FlowSensitive/UncheckedOptionalAccessModelTest.cpp  | 17 +++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index c883f90..b8e3e93 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -93,7 +93,19 @@ static Value *mergeDistinctValues(QualType Type, Value &Val1,
                                   Environment::ValueModel &Model) {
   // Join distinct boolean values preserving information about the constraints
   // in the respective path conditions.
-  if (auto *Expr1 = dyn_cast<BoolValue>(&Val1)) {
+  if (Type->isBooleanType()) {
+    // FIXME: The type check above is a workaround and should be unnecessary.
+    // However, right now we can end up with BoolValue's in integer-typed
+    // variables due to our incorrect handling of boolean-to-integer casts (we
+    // just propagate the BoolValue to the result of the cast). For example:
+    // std::optional<bool> o;
+    //
+    //
+    // int x;
+    // if (o.has_value()) {
+    //   x = o.value();
+    // }
+    auto *Expr1 = cast<BoolValue>(&Val1);
     auto *Expr2 = cast<BoolValue>(&Val2);
     auto &MergedVal = MergedEnv.makeAtomicBoolValue();
     MergedEnv.addToFlowCondition(MergedEnv.makeOr(
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 4d9c57f..1fcede5 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2970,6 +2970,23 @@ TEST_P(UncheckedOptionalAccessTest, CtorInitializerValue) {
       cxxConstructorDecl(ofClass(hasName("Target"))));
 }
 
+// This is regression test, it shouldn't crash.
+TEST_P(UncheckedOptionalAccessTest, Bitfield) {
+  using namespace ast_matchers;
+  ExpectDiagnosticsFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+    struct Dst {
+      unsigned int n : 1;
+    };
+    void target() {
+      $ns::$optional<bool> v;
+      Dst d;
+      if (v.has_value())
+        d.n = v.value();
+    }
+  )");
+}
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
-- 
2.7.4