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

[Jun Zhang via Phabricator via cfe-commits]

junaire updated this revision to Diff 485654.
junaire added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140753/new/

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 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
@@ -93,7 +93,18 @@
   Environment::ValueModel &Model) {
   // Join distinct boolean values preserving information about the constraints
   // in the respective path conditions.
-  if (auto *Expr1 = dyn_cast(&Val1)) {
+  if (Type->isBooleanType()) {
+// FIXME: This is sort of workaound. Since now we just ignore all 
(implicit)
+// integral casts, treating the resulting value as same as the underling
+// value, it could cause inconsistency between values after `Join` if in
+// some paths the type doesn't strictly match.
+// Eg:
+// std::optional o;
+// int x;
+// if (o.has_value()) {
+//   x = o.value();
+// }
+auto *Expr1 = cast(&Val1);
 auto *Expr2 = cast(&Val2);
 auto &MergedVal = MergedEnv.makeAtomicBoolValue();
 MergedEnv.addToFlowCondition(MergedEnv.makeOr(


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 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
@@ -93,7 +93,18 @@
   Environment::ValueModel &Model) {
   // Join distinct boolean values preserving information about the constraints
   // in the respective path conditions.
-  if (auto *Expr1 = dyn_cast(&Val1)) {
+  if (Type->isBooleanType()) {
+// FIXME: This is sort of workaound. Since now we just ignore all (implicit)
+// integral casts, treating the resulting value as same as the underling
+// value, it could cause inconsistency between values after `Join` if in
+// some paths the type doesn't strictly match.
+// Eg:
+// std::optional o;
+// int x;
+// if (o.has_value()) {
+//   x = o.value();
+// }
+auto *Expr1 = cast(&Val1);
 auto *Expr2 = cast(&Val2);
 auto &MergedVal = MergedEnv.makeAtomicBoolValue();
 MergedEnv.addToFlowCondition(MergedEnv.makeOr(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jun Zhang via Phabricator via cfe-commits]

junaire added a comment.

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 :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140753/new/

https://reviews.llvm.org/D140753

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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 requested changes to this revision.
gribozavr2 added a comment.
This revision now requires changes to proceed.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140753/new/

https://reviews.llvm.org/D140753

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

[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 


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 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(&Val1)) {
-auto *Expr2 = cast(&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(&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 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(&Val1)) {
-auto *Expr2 = cast(&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(&Val2)) {
+  auto &MergedVal = MergedEnv.makeAtomicBoolValue();
+  MergedEnv.addToFlowCondition(MergedEnv.makeOr(
+  MergedEnv.makeAnd(Env1.getFlowConditionToken(),
+MergedEnv.ma