[PATCH] D140753: [clang][dataflow] Check both operand's type in mergeDistinctValues

[Jun Zhang via Phabricator via cfe-commits]

junaire created this revision.
Herald added subscribers: martong, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
junaire requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Previously we assume RHS is a BoolValue if LHS is a BoolValue. However,
if RHS represents a bitfield in a struct/class, this could lead to bad
casting.

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

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140753

Files:
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp


Index: 
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2970,6 +2970,23 @@
       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)
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -94,14 +94,15 @@
   // Join distinct boolean values preserving information about the constraints
   // in the respective path conditions.
   if (auto *Expr1 = dyn_cast<BoolValue>(&Val1)) {
-    auto *Expr2 = cast<BoolValue>(&Val2);
-    auto &MergedVal = MergedEnv.makeAtomicBoolValue();
-    MergedEnv.addToFlowCondition(MergedEnv.makeOr(
-        MergedEnv.makeAnd(Env1.getFlowConditionToken(),
-                          MergedEnv.makeIff(MergedVal, *Expr1)),
-        MergedEnv.makeAnd(Env2.getFlowConditionToken(),
-                          MergedEnv.makeIff(MergedVal, *Expr2))));
-    return &MergedVal;
+    if (auto *Expr2 = dyn_cast<BoolValue>(&Val2)) {
+      auto &MergedVal = MergedEnv.makeAtomicBoolValue();
+      MergedEnv.addToFlowCondition(MergedEnv.makeOr(
+          MergedEnv.makeAnd(Env1.getFlowConditionToken(),
+                            MergedEnv.makeIff(MergedVal, *Expr1)),
+          MergedEnv.makeAnd(Env2.getFlowConditionToken(),
+                            MergedEnv.makeIff(MergedVal, *Expr2))));
+      return &MergedVal;
+    }
   }
 
   // FIXME: Consider destroying `MergedValue` immediately if 
`ValueModel::merge`



Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2970,6 +2970,23 @@
       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)
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -94,14 +94,15 @@
   // Join distinct boolean values preserving information about the constraints
   // in the respective path conditions.
   if (auto *Expr1 = dyn_cast<BoolValue>(&Val1)) {
-    auto *Expr2 = cast<BoolValue>(&Val2);
-    auto &MergedVal = MergedEnv.makeAtomicBoolValue();
-    MergedEnv.addToFlowCondition(MergedEnv.makeOr(
-        MergedEnv.makeAnd(Env1.getFlowConditionToken(),
-                          MergedEnv.makeIff(MergedVal, *Expr1)),
-        MergedEnv.makeAnd(Env2.getFlowConditionToken(),
-                          MergedEnv.makeIff(MergedVal, *Expr2))));
-    return &MergedVal;
+    if (auto *Expr2 = dyn_cast<BoolValue>(&Val2)) {
+      auto &MergedVal = MergedEnv.makeAtomicBoolValue();
+      MergedEnv.addToFlowCondition(MergedEnv.makeOr(
+          MergedEnv.makeAnd(Env1.getFlowConditionToken(),
+                            MergedEnv.makeIff(MergedVal, *Expr1)),
+          MergedEnv.makeAnd(Env2.getFlowConditionToken(),
+                            MergedEnv.makeIff(MergedVal, *Expr2))));
+      return &MergedVal;
+    }
   }
 
   // FIXME: Consider destroying `MergedValue` immediately if `ValueModel::merge`

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits