[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
mboehme marked an inline comment as done.
Closed by commit rG762cb1d37736: [clang][dataflow] Create `Value`s for integer 
literals. (authored by mboehme).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

Files:
  clang/include/clang/Analysis/FlowSensitive/Arena.h
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/Arena.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -811,6 +811,31 @@
   });
 }
 
+TEST(TransferTest, BinaryOperatorAssignIntegerLiteral) {
+  std::string Code = R"(
+void target() {
+  int Foo = 1;
+  // [[before]]
+  Foo = 2;
+  // [[after]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Before =
+getEnvironmentAtAnnotation(Results, "before");
+const Environment &After = getEnvironmentAtAnnotation(Results, "after");
+
+const auto &ValBefore =
+getValueForDecl(ASTCtx, Before, "Foo");
+const auto &ValAfter =
+getValueForDecl(ASTCtx, After, "Foo");
+EXPECT_NE(&ValBefore, &ValAfter);
+  });
+}
+
 TEST(TransferTest, VarDeclInitAssign) {
   std::string Code = R"(
 void target() {
@@ -3441,6 +3466,24 @@
   });
 }
 
+TEST(TransferTest, IntegerLiteralEquality) {
+  std::string Code = R"(
+void target() {
+  bool equal = (42 == 42);
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+auto &Equal = getValueForDecl(ASTCtx, Env, "equal");
+EXPECT_TRUE(Env.flowConditionImplies(Equal));
+  });
+}
+
 TEST(TransferTest, CorrelatedBranches) {
   std::string Code = R"(
 void target(bool B, bool C) {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -48,9 +48,15 @@
 
 static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
   Environment &Env) {
-  if (auto *LHSValue = dyn_cast_or_null(Env.getValueStrict(LHS)))
-if (auto *RHSValue = dyn_cast_or_null(Env.getValueStrict(RHS)))
-  return Env.makeIff(*LHSValue, *RHSValue);
+  Value *LHSValue = Env.getValueStrict(LHS);
+  Value *RHSValue = Env.getValueStrict(RHS);
+
+  if (LHSValue == RHSValue)
+return Env.getBoolLiteralValue(true);
+
+  if (auto *LHSBool = dyn_cast_or_null(LHSValue))
+if (auto *RHSBool = dyn_cast_or_null(RHSValue))
+  return Env.makeIff(*LHSBool, *RHSBool);
 
   return Env.makeAtomicBoolValue();
 }
@@ -776,6 +782,10 @@
 Env.setValueStrict(*S, Env.getBoolLiteralValue(S->getValue()));
   }
 
+  void VisitIntegerLiteral(const IntegerLiteral *S) {
+Env.setValueStrict(*S, Env.getIntLiteralValue(S->getValue()));
+  }
+
   void VisitParenExpr(const ParenExpr *S) {
 // The CFG does not contain `ParenExpr` as top-level statements in basic
 // blocks, however manual traversal to sub-expressions may encounter them.
Index: clang/lib/Analysis/FlowSensitive/Arena.cpp
===
--- clang/lib/Analysis/FlowSensitive/Arena.cpp
+++ clang/lib/Analysis/FlowSensitive/Arena.cpp
@@ -68,4 +68,12 @@
   return *Res.first->second;
 }
 
+IntegerValue &Arena::makeIntLiteral(llvm::APInt Value) {
+  auto [It, Inserted] = IntegerLiterals.try_emplace(Value, nullptr);
+
+  if (Inserted)
+It->second = &create();
+  return *It->second;
+}
+
 } // namespace clang::dataflow
Index: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -448,6 +448,12 @@
 return DACtx->arena().create(std::forward(args)...);
   }
 
+  /// Returns a symbolic integer value that models an integer literal equal to
+  /// `Value`
+  IntegerValue &getIntLiteralValue(llvm::APInt Value) const {
+return DACtx->arena().makeIntLiteral(Value);
+  }
+
   /// Returns a symbolic boolean value that models a boolean literal equal to
   /// `Value`
   AtomicBoolValue &getBoolLiteralValue(bool Value) const {
Index: clang/include/clang/Analysis/FlowSensitive/Arena.h
=

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 2 inline comments as done.
mboehme added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Arena.h:89
+  /// `Value`. These literals are the same every time.
+  IntegerValue &makeIntLiteral(llvm::APInt Value);
+

xazax.hun wrote:
> gribozavr2 wrote:
> > Should we be taking the type into account? If not, why not? Please add the 
> > type or document why the type isn't tracked.
> What would happen if we try to create the same value (like 5) but with 
> different bit widths? (E.g., 64 bit integer vs 32 bit integer). I suspect we 
> might end up having the same value twice in our constant pool, which might be 
> fine. Just wanted to double check what is the desired behavior.
> Should we be taking the type into account? If not, why not? Please add the 
> type or document why the type isn't tracked.

I've clarified that the type isn't tracked but is instead determined by the 
`Expr` that the integer literal is associated with. (This is consistent with 
our treatment of `IntegerValue`s more generally.)

If we want to do constant propagation through integral conversions, this should 
be done when processing the `IntegerCast` node.  This would, if necessary, 
produce an integer literal with a different value.



Comment at: clang/include/clang/Analysis/FlowSensitive/Arena.h:89
+  /// `Value`. These literals are the same every time.
+  IntegerValue &makeIntLiteral(llvm::APInt Value);
+

mboehme wrote:
> xazax.hun wrote:
> > gribozavr2 wrote:
> > > Should we be taking the type into account? If not, why not? Please add 
> > > the type or document why the type isn't tracked.
> > What would happen if we try to create the same value (like 5) but with 
> > different bit widths? (E.g., 64 bit integer vs 32 bit integer). I suspect 
> > we might end up having the same value twice in our constant pool, which 
> > might be fine. Just wanted to double check what is the desired behavior.
> > Should we be taking the type into account? If not, why not? Please add the 
> > type or document why the type isn't tracked.
> 
> I've clarified that the type isn't tracked but is instead determined by the 
> `Expr` that the integer literal is associated with. (This is consistent with 
> our treatment of `IntegerValue`s more generally.)
> 
> If we want to do constant propagation through integral conversions, this 
> should be done when processing the `IntegerCast` node.  This would, if 
> necessary, produce an integer literal with a different value.
> What would happen if we try to create the same value (like 5) but with 
> different bit widths? (E.g., 64 bit integer vs 32 bit integer). I suspect we 
> might end up having the same value twice in our constant pool, which might be 
> fine. Just wanted to double check what is the desired behavior.

This isn't a consideration, as integer literals aren't typed (see reply to 
comment above).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 531863.
mboehme added a comment.

Clarify that integer literals aren't typed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

Files:
  clang/include/clang/Analysis/FlowSensitive/Arena.h
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/Arena.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -811,6 +811,31 @@
   });
 }
 
+TEST(TransferTest, BinaryOperatorAssignIntegerLiteral) {
+  std::string Code = R"(
+void target() {
+  int Foo = 1;
+  // [[before]]
+  Foo = 2;
+  // [[after]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Before =
+getEnvironmentAtAnnotation(Results, "before");
+const Environment &After = getEnvironmentAtAnnotation(Results, "after");
+
+const auto &ValBefore =
+getValueForDecl(ASTCtx, Before, "Foo");
+const auto &ValAfter =
+getValueForDecl(ASTCtx, After, "Foo");
+EXPECT_NE(&ValBefore, &ValAfter);
+  });
+}
+
 TEST(TransferTest, VarDeclInitAssign) {
   std::string Code = R"(
 void target() {
@@ -3441,6 +3466,24 @@
   });
 }
 
+TEST(TransferTest, IntegerLiteralEquality) {
+  std::string Code = R"(
+void target() {
+  bool equal = (42 == 42);
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+auto &Equal = getValueForDecl(ASTCtx, Env, "equal");
+EXPECT_TRUE(Env.flowConditionImplies(Equal));
+  });
+}
+
 TEST(TransferTest, CorrelatedBranches) {
   std::string Code = R"(
 void target(bool B, bool C) {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -48,9 +48,15 @@
 
 static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
   Environment &Env) {
-  if (auto *LHSValue = dyn_cast_or_null(Env.getValueStrict(LHS)))
-if (auto *RHSValue = dyn_cast_or_null(Env.getValueStrict(RHS)))
-  return Env.makeIff(*LHSValue, *RHSValue);
+  Value *LHSValue = Env.getValueStrict(LHS);
+  Value *RHSValue = Env.getValueStrict(RHS);
+
+  if (LHSValue == RHSValue)
+return Env.getBoolLiteralValue(true);
+
+  if (auto *LHSBool = dyn_cast_or_null(LHSValue))
+if (auto *RHSBool = dyn_cast_or_null(RHSValue))
+  return Env.makeIff(*LHSBool, *RHSBool);
 
   return Env.makeAtomicBoolValue();
 }
@@ -775,6 +781,10 @@
 Env.setValueStrict(*S, Env.getBoolLiteralValue(S->getValue()));
   }
 
+  void VisitIntegerLiteral(const IntegerLiteral *S) {
+Env.setValueStrict(*S, Env.getIntLiteralValue(S->getValue()));
+  }
+
   void VisitParenExpr(const ParenExpr *S) {
 // The CFG does not contain `ParenExpr` as top-level statements in basic
 // blocks, however manual traversal to sub-expressions may encounter them.
Index: clang/lib/Analysis/FlowSensitive/Arena.cpp
===
--- clang/lib/Analysis/FlowSensitive/Arena.cpp
+++ clang/lib/Analysis/FlowSensitive/Arena.cpp
@@ -68,4 +68,12 @@
   return *Res.first->second;
 }
 
+IntegerValue &Arena::makeIntLiteral(llvm::APInt Value) {
+  auto [It, Inserted] = IntegerLiterals.try_emplace(Value, nullptr);
+
+  if (Inserted)
+It->second = &create();
+  return *It->second;
+}
+
 } // namespace clang::dataflow
Index: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -448,6 +448,12 @@
 return DACtx->arena().create(std::forward(args)...);
   }
 
+  /// Returns a symbolic integer value that models an integer literal equal to
+  /// `Value`
+  IntegerValue &getIntLiteralValue(llvm::APInt Value) const {
+return DACtx->arena().makeIntLiteral(Value);
+  }
+
   /// Returns a symbolic boolean value that models a boolean literal equal to
   /// `Value`
   AtomicBoolValue &getBoolLiteralValue(bool Value) const {
Index: clang/include/clang/Analysis/FlowSensitive/Arena.h
===
--- clang/include/clang/Analysis/FlowSensitiv

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Arena.h:89
+  /// `Value`. These literals are the same every time.
+  IntegerValue &makeIntLiteral(llvm::APInt Value);
+

gribozavr2 wrote:
> Should we be taking the type into account? If not, why not? Please add the 
> type or document why the type isn't tracked.
What would happen if we try to create the same value (like 5) but with 
different bit widths? (E.g., 64 bit integer vs 32 bit integer). I suspect we 
might end up having the same value twice in our constant pool, which might be 
fine. Just wanted to double check what is the desired behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Arena.h:89
+  /// `Value`. These literals are the same every time.
+  IntegerValue &makeIntLiteral(llvm::APInt Value);
+

Should we be taking the type into account? If not, why not? Please add the type 
or document why the type isn't tracked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

PTAL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 531694.
mboehme added a comment.

Always return the same `Value` for the same integer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

Files:
  clang/include/clang/Analysis/FlowSensitive/Arena.h
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/Arena.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -811,6 +811,31 @@
   });
 }
 
+TEST(TransferTest, BinaryOperatorAssignIntegerLiteral) {
+  std::string Code = R"(
+void target() {
+  int Foo = 1;
+  // [[before]]
+  Foo = 2;
+  // [[after]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Before =
+getEnvironmentAtAnnotation(Results, "before");
+const Environment &After = getEnvironmentAtAnnotation(Results, "after");
+
+const auto &ValBefore =
+getValueForDecl(ASTCtx, Before, "Foo");
+const auto &ValAfter =
+getValueForDecl(ASTCtx, After, "Foo");
+EXPECT_NE(&ValBefore, &ValAfter);
+  });
+}
+
 TEST(TransferTest, VarDeclInitAssign) {
   std::string Code = R"(
 void target() {
@@ -3441,6 +3466,24 @@
   });
 }
 
+TEST(TransferTest, IntegerLiteralEquality) {
+  std::string Code = R"(
+void target() {
+  bool equal = (42 == 42);
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+auto &Equal = getValueForDecl(ASTCtx, Env, "equal");
+EXPECT_TRUE(Env.flowConditionImplies(Equal));
+  });
+}
+
 TEST(TransferTest, CorrelatedBranches) {
   std::string Code = R"(
 void target(bool B, bool C) {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -48,9 +48,15 @@
 
 static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
   Environment &Env) {
-  if (auto *LHSValue = dyn_cast_or_null(Env.getValueStrict(LHS)))
-if (auto *RHSValue = dyn_cast_or_null(Env.getValueStrict(RHS)))
-  return Env.makeIff(*LHSValue, *RHSValue);
+  Value *LHSValue = Env.getValueStrict(LHS);
+  Value *RHSValue = Env.getValueStrict(RHS);
+
+  if (LHSValue == RHSValue)
+return Env.getBoolLiteralValue(true);
+
+  if (auto *LHSBool = dyn_cast_or_null(LHSValue))
+if (auto *RHSBool = dyn_cast_or_null(RHSValue))
+  return Env.makeIff(*LHSBool, *RHSBool);
 
   return Env.makeAtomicBoolValue();
 }
@@ -775,6 +781,10 @@
 Env.setValueStrict(*S, Env.getBoolLiteralValue(S->getValue()));
   }
 
+  void VisitIntegerLiteral(const IntegerLiteral *S) {
+Env.setValueStrict(*S, Env.getIntLiteralValue(S->getValue()));
+  }
+
   void VisitParenExpr(const ParenExpr *S) {
 // The CFG does not contain `ParenExpr` as top-level statements in basic
 // blocks, however manual traversal to sub-expressions may encounter them.
Index: clang/lib/Analysis/FlowSensitive/Arena.cpp
===
--- clang/lib/Analysis/FlowSensitive/Arena.cpp
+++ clang/lib/Analysis/FlowSensitive/Arena.cpp
@@ -68,4 +68,12 @@
   return *Res.first->second;
 }
 
+IntegerValue &Arena::makeIntLiteral(llvm::APInt Value) {
+  auto [It, Inserted] = IntegerLiterals.try_emplace(Value, nullptr);
+
+  if (Inserted)
+It->second = &create();
+  return *It->second;
+}
+
 } // namespace clang::dataflow
Index: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -448,6 +448,12 @@
 return DACtx->arena().create(std::forward(args)...);
   }
 
+  /// Returns a symbolic integer value that models an integer literal equal to
+  /// `Value`
+  IntegerValue &getIntLiteralValue(llvm::APInt Value) const {
+return DACtx->arena().makeIntLiteral(Value);
+  }
+
   /// Returns a symbolic boolean value that models a boolean literal equal to
   /// `Value`
   AtomicBoolValue &getBoolLiteralValue(bool Value) const {
Index: clang/include/clang/Analysis/FlowSensitive/Arena.h
===
--- clang/include/clang/Analysis/Flo

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

In D152813#4420471 , @gribozavr2 
wrote:

> In D152813#4420448 , @mboehme wrote:
>
>> We would expect analysis of this code to converge too -- right?
>
> Yes - but we might need universal top values for that? maybe?

Ah, I see what you mean.

Anyway, I'll look at producing "reproducible" `Value`s for the same integer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

In D152813#4420448 , @mboehme wrote:

> We would expect analysis of this code to converge too -- right?

Yes - but we might need universal top values for that? maybe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

In D152813#4420445 , @gribozavr2 
wrote:

> In D152813#4420399 , @mboehme wrote:
>
>> It looks to me as if the values we're now newly producing for integer 
>> literals are causing non-convergence of the analysis on the for-loop.
>
> For integer literals specifically, we should be returning the same value for 
> the same literal over multiple loop iterations. And also, ideally, for 
> multiple equal literals, too, so that an integer literal "42", no matter how 
> many times spelled in the program, creates the same symbolic value.

That's true, and I'll modify the patch so that we do this. (I should probably 
also include a test that checks that `42 == 42` is equivalent to `true`.)

But even without this, wouldn't we expect the analysis to converge? The way I'm 
treating integer literals right now, the analysis is in effect "seeing" the 
example I quoted above like this:

  int return_some_random_int();
  void multiple_unchecked_accesses(absl::optional opt1,
   absl::optional opt2) {
for (int i = return_some_random_int(); i < return_some_random_int(); i++) {
  opt1.value();
  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional
}
opt2.value();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional 
value
  }

We would expect analysis of this code to converge too -- right? So I think the 
problem we're seeing here is independent of how we treat integer literals 
specifically (though I agree the same integer literal should always be 
represented by the same `Value`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

In D152813#4420399 , @mboehme wrote:

> It looks to me as if the values we're now newly producing for integer 
> literals are causing non-convergence of the analysis on the for-loop.

For integer literals specifically, we should be returning the same value for 
the same literal over multiple loop iterations. And also, ideally, for multiple 
equal literals, too, so that an integer literal "42", no matter how many times 
spelled in the program, creates the same symbolic value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

Pre-merge check failures are in a test for the unchecked-optional-access check 
clang-tidy. The test that's failing is the following from 
unchecked-optional-access.cpp:

  c++
  void multiple_unchecked_accesses(absl::optional opt1,
   absl::optional opt2) {
for (int i = 0; i < 10; i++) {
  opt1.value();
  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional
}
opt2.value();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional 
value
  }

Both of the expected diagnostics are not output.

It looks to me as if the values we're now newly producing for integer literals 
are causing non-convergence of the analysis on the for-loop.

This looks like a pretty serious issue. We would certainly want analysis of a 
simple loop like this to converge, especially with a small bounded trip count, 
but even with an unbounded trip count. It looks as if, currently, the analysis 
is converging merely because we don't generate values for integer literals.

Next step will be to repro this issue with a test in TransferTest.cpp so that I 
can take a closer look at why we're failing to converge.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Martin Böhme via Phabricator via cfe-commits]

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

This patch includes a test that fails without the fix.

I discovered that we weren't creating `Value`s for integer literals when, in a
different patch, I tried to overwrite the value of a struct field with a literal
for the purposes of a test and was surprised to find that the struct compared
the same before and after the assignment.

This functionality therefore seems useful at least for tests, but is probably
also useful for actual analysis of code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152813

Files:
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -811,6 +811,31 @@
   });
 }
 
+TEST(TransferTest, BinaryOperatorAssignLiteral) {
+  std::string Code = R"(
+void target() {
+  int Foo = 1;
+  // [[before]]
+  Foo = 2;
+  // [[after]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Before =
+getEnvironmentAtAnnotation(Results, "before");
+const Environment &After = getEnvironmentAtAnnotation(Results, 
"after");
+
+const auto &ValBefore =
+getValueForDecl(ASTCtx, Before, "Foo");
+const auto &ValAfter =
+getValueForDecl(ASTCtx, After, "Foo");
+EXPECT_NE(&ValBefore, &ValAfter);
+  });
+}
+
 TEST(TransferTest, VarDeclInitAssign) {
   std::string Code = R"(
 void target() {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -775,6 +775,11 @@
 Env.setValueStrict(*S, Env.getBoolLiteralValue(S->getValue()));
   }
 
+  void VisitIntegerLiteral(const IntegerLiteral *S) {
+if (Value *Val = Env.createValue(S->getType()))
+  Env.setValueStrict(*S, *Val);
+  }
+
   void VisitParenExpr(const ParenExpr *S) {
 // The CFG does not contain `ParenExpr` as top-level statements in basic
 // blocks, however manual traversal to sub-expressions may encounter them.


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -811,6 +811,31 @@
   });
 }
 
+TEST(TransferTest, BinaryOperatorAssignLiteral) {
+  std::string Code = R"(
+void target() {
+  int Foo = 1;
+  // [[before]]
+  Foo = 2;
+  // [[after]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Before =
+getEnvironmentAtAnnotation(Results, "before");
+const Environment &After = getEnvironmentAtAnnotation(Results, "after");
+
+const auto &ValBefore =
+getValueForDecl(ASTCtx, Before, "Foo");
+const auto &ValAfter =
+getValueForDecl(ASTCtx, After, "Foo");
+EXPECT_NE(&ValBefore, &ValAfter);
+  });
+}
+
 TEST(TransferTest, VarDeclInitAssign) {
   std::string Code = R"(
 void target() {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -775,6 +775,11 @@
 Env.setValueStrict(*S, Env.getBoolLiteralValue(S->getValue()));
   }
 
+  void VisitIntegerLiteral(const IntegerLiteral *S) {
+if (Value *Val = Env.createValue(S->getType()))
+  Env.setValueStrict(*S, *Val);
+  }
+
   void VisitParenExpr(const ParenExpr *S) {
 // The CFG does not contain `ParenExpr` as top-level statements in basic
 // blocks, however manual traversal to sub-expressions may encounter them.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits