[clang][dataflow] Fix crash when having boolean-to-integral casts.

[junaire]

Summary

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 <[e-mail]>

[gribozavr2]

Thank you for your contribution!

While adding a conditional check fixes the crash, the problem's root cause must be deeper. Mismatched types indicate that one code path in dataflow analysis computes a bool type for a storage location, while a different code path computes an integer type. That's the actual root cause. Could you try to investigate the reasons for that, and try to fix it? The dataflow analysis should be computing values of the same type no matter through which path we arrived at a program point.

[junaire]

In D140753#4019322, @gribozavr2 wrote:

Thank you for your contribution!

While adding a conditional check fixes the crash, the problem's root cause must be deeper. Mismatched types indicate that one code path in dataflow analysis computes a bool type for a storage location, while a different code path computes an integer type. That's the actual root cause. Could you try to investigate the reasons for that, and try to fix it? The dataflow analysis should be computing values of the same type no matter through which path we arrived at a program point.

Thanks for your thoughts about the bug. Sure I'd love to try to investigate and will let you know what I finally got :)

[junaire]

Update patch according to @ymandel 's suggestion. Thanks!

[junaire]

Fix typos

[gribozavr2]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

97–106

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

 /// meaning depends on `Model`.
 static Value *mergeDistinctValues(QualType Type, Value &Val1,
                                   const Environment &Env1, Value &Val2,
                                   const Environment &Env2,
                                   Environment &MergedEnv,
                                   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(
         MergedEnv.makeAnd(Env1.getFlowConditionToken(),
                           MergedEnv.makeIff(MergedVal, *Expr1)),
         MergedEnv.makeAnd(Env2.getFlowConditionToken(),
                           MergedEnv.makeIff(MergedVal, *Expr2))));
     return &MergedVal;

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

struct Target {
         opt.value();
       }
       $ns::$optional<int> opt;
     };
   )",
       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)
 // - invalidation (passing optional by non-const reference/pointer)