[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 <jun at 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`


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D140753.485590.patch
Type: text/x-patch
Size: 2321 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221229/5c6361da/attachment.bin>