[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

@rsmith Thanks a bunch!

And thanks @sepavloff for trying out this approach too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff abandoned this revision.
sepavloff added a comment.

@rsmith, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Alternative fix landed in rG30baa5d2a450d5e302d8cba3fc7a26a59d4b7ae1 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:1549
 SubobjectDesignator Designator;
+const Expr *LExpr = nullptr;
 bool IsNullPtr : 1;

I don't think this is an appropriate approach. A source expression is 
fundamentally not part of an lvalue, and so we shouldn't be tracking one. I 
think we can fix this bug by handling the active union member change for a 
class assignment at a higher level, working on a patch...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

Hello, I know I'm not helping move this patch along because I'm not the right 
person to review this. However, I'd like to kindly reiterate that fixing this 
would be helpful for libc++. I just had to re-XFAIL one of our tuple tests 
because of this :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

P1330R0 (https://wg21.link/P1330R0) allowed changing the active member of a 
union in constexpr expressions. To implement it the Constant Evaluator starting 
from
https://github.com/llvm/llvm-project/commit/31c69a3d6363463c08b86914c0c8cfc5c929c37e
 checks if an assignment changes active member of a union. This is why a change 
proposed for union only affects compilation of code that does not contain 
unions at all. The issue is in the assignment treatment.

The check is made by the function `HandleUnionActiveMemberChange`, which is 
called from `VisitBinAssign` and `HandleFunctionCall`, they handle builtin and 
trivial assignment operators respectively. The check is made according to the 
rule specified in the standard (http://eel.is/c++draft/class.union#general-6), 
which is based on syntactic form. Constant evaluator on the other hand uses 
objects of class `APValue` and related `LValue` to make the evaluations.  In 
some cases it is not possible to get `Expr` used for the evaluation.

Consider slightly modified test case provided in the bug report:

  struct Base { int m; };
  struct Derived : Base {};
   
  constexpr int func_01(int x) {
Base v1{1};
Derived v2{x};
v1 = v2;
return v1.m;
  }
   
  constexpr int k = func_01(0);

When Constant Evaluator evaluates the expression `v1 = v2`, it eventually calls 
`LValueExprEvaluator::handleCallExpr`  as this expression is a call of 
assignment operator. This is a method call, so the function evaluates the 
object reference, an `LValue`, which represents `v1` as `ValueDecl`. Then 
`HandleFunctionCall` is called, provided with the `LValue` for `v1` and an 
expression for `v2`. This function calls `HandleUnionActiveMemberChange`, which 
checks if the assignment changes an union active member.

`HandleUnionActiveMemberChange` makes the check according to the standard, so 
it needs `v1` in the form of expression. However, `v1` is represented by 
`LValue` in `HandleFunctionCall`, which contains `ValueDecl` and the original 
expression is unavailable. It makes the check impossible.

To make the check `HandleUnionActiveMemberChange` must be provided by the 
expression representing `v1`.  A simple solution proposed here is to keep the 
original expression in `LValue`. It is set when `LValue` is evaluated when by 
`LValueExprEvaluator` in its method `Evaluate`. If the evaluation produces 
`LValue` based on something other than an expression, the reference to the 
original method remains in that `LValue` and may be used when the original 
expression is required.

The obtained solution is low-invasive. Actually only the changes in 
`LValueExprEvaluator::Evaluate` and `Lvalue::set` are essential for maintaining 
expression reference in `LValue`, and the change in 
`HandleUnionActiveMemberChange` is the only usage of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D103395#3105677 , @tambre wrote:

> In D103395#3105668 , @sepavloff 
> wrote:
>
>> Strange, I see that it cannot be compiled neither by gcc nor by clang: 
>> https://godbolt.org/z/1dY9Gs6zM. Do I miss something?
>
> Sorry, should've been more specific. Try in C++20 mode: 
> https://godbolt.org/z/4v8b3nsET
> I think the difference might be due to P1331R2 
> , but 
> I'm not sure.

Thank you for this helpful test. I used a bit more accurate use of LHS 
expression and the issue seems resolved.

The new variant uses slightly more careful LHS expression tracking. Previously 
the expression was assigned only once. Now if the expression for LValue can be 
determined, it is updated. Only if the LValue starts to track non-expression 
entities, LHS expression is frozen to the last seen value. It does not affect 
the evaluation of active union member though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 386823.
sepavloff added a comment.

Update the patch

- Use more careful check for LHS expression,
- Make a bit more precise LHS expression tracking,
- Add comments,
- Add the test from discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp

Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1447,3 +1447,22 @@
   constexpr bool b = [a = S(), b = S()] { return a.p == b.p; }();
   static_assert(!b);
 }
+
+namespace PR45879 {
+struct Base {
+  int m;
+};
+struct Derived : Base {};
+constexpr int k = ((Base{} = Derived{}), 0);
+
+struct sub {
+  char data;
+};
+struct main2 {
+  constexpr main2() {
+member = {};
+  }
+  sub member;
+};
+constexpr main2 a{};
+} // namespace PR45879
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -1546,6 +1546,7 @@
 APValue::LValueBase Base;
 CharUnits Offset;
 SubobjectDesignator Designator;
+const Expr *LExpr = nullptr;
 bool IsNullPtr : 1;
 bool InvalidBase : 1;
 
@@ -1554,6 +1555,7 @@
 const CharUnits () const { return Offset; }
 SubobjectDesignator () { return Designator; }
 const SubobjectDesignator () const { return Designator;}
+const Expr *getExpr() const { return LExpr; }
 bool isNullPointer() const { return IsNullPtr;}
 
 unsigned getLValueCallIndex() const { return Base.getCallIndex(); }
@@ -1591,6 +1593,8 @@
   Offset = CharUnits::fromQuantity(0);
   InvalidBase = BInvalid;
   Designator = SubobjectDesignator(getType(B));
+  if (const Expr *E = B.dyn_cast())
+LExpr = E;
   IsNullPtr = false;
 }
 
@@ -5893,11 +5897,17 @@
 /// operator whose left-hand side might involve a union member access. If it
 /// does, implicitly start the lifetime of any accessed union elements per
 /// C++20 [class.union]5.
+/// Returns true if the assignment is OK and it may be processed further.
 static bool HandleUnionActiveMemberChange(EvalInfo , const Expr *LHSExpr,
   const LValue ) {
   if (LHS.InvalidBase || LHS.Designator.Invalid)
 return false;
 
+  // If no LHS expression was specified, assume the LHS cannot be an assignment
+  // to a union member.
+  if (!LHSExpr)
+return true;
+
   llvm::SmallVector, 4> UnionPathLengths;
   // C++ [class.union]p5:
   //   define the set S(E) of subexpressions of E as follows:
@@ -6101,7 +6111,7 @@
MD->getParent()->isUnion()))
   return false;
 if (Info.getLangOpts().CPlusPlus20 && MD->isTrivial() &&
-!HandleUnionActiveMemberChange(Info, Args[0], *This))
+!HandleUnionActiveMemberChange(Info, This->getExpr(), *This))
   return false;
 if (!handleAssignment(Info, Args[0], *This, MD->getThisType(),
   RHSValue))
@@ -8058,10 +8068,20 @@
 namespace {
 class LValueExprEvaluator
   : public LValueExprEvaluatorBase {
+  friend class LValueExprEvaluatorBase;
+  friend class ExprEvaluatorBase;
+  friend class ConstStmtVisitor;
+  friend class StmtVisitorBase;
 public:
   LValueExprEvaluator(EvalInfo , LValue , bool InvalidBaseOK) :
 LValueExprEvaluatorBaseTy(Info, Result, InvalidBaseOK) {}
 
+  bool Evaluate(const Expr *E) {
+Result.LExpr = E;
+return Visit(E);
+  }
+
+protected:
   bool VisitVarDecl(const Expr *E, const VarDecl *VD);
   bool VisitUnaryPreIncDec(const UnaryOperator *UO);
 
@@ -8123,7 +8143,7 @@
   assert(!E->isValueDependent());
   assert(E->isGLValue() || E->getType()->isFunctionType() ||
  E->getType()->isVoidType() || isa(E));
-  return LValueExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
+  return LValueExprEvaluator(Info, Result, InvalidBaseOK).Evaluate(E);
 }
 
 bool LValueExprEvaluator::VisitDeclRefExpr(const DeclRefExpr *E) {
@@ -8424,7 +8444,7 @@
 
   // C++17 onwards require that we evaluate the RHS first.
   APValue RHS;
-  if (!Evaluate(RHS, this->Info, CAO->getRHS())) {
+  if (!::Evaluate(RHS, this->Info, CAO->getRHS())) {
 if (!Info.noteFailure())
   return false;
 Success = false;
@@ -8448,7 +8468,7 @@
 
   // C++17 onwards require that we evaluate the RHS first.
   APValue NewVal;
-  if (!Evaluate(NewVal, this->Info, E->getRHS())) {
+  if (!::Evaluate(NewVal, this->Info, E->getRHS())) {
 if (!Info.noteFailure())
   return false;
 Success = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D103395#3105668 , @sepavloff wrote:

> Strange, I see that it cannot be compiled neither by gcc nor by clang: 
> https://godbolt.org/z/1dY9Gs6zM. Do I miss something?

Sorry, should've been more specific. Try in C++20 mode: 
https://godbolt.org/z/4v8b3nsET
I think the difference might be due to P1331R2 
, but I'm 
not sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

Thank you for feedback!

In D103395#3096177 , @tambre wrote:

> This breaks the following code:
>
>   struct sub
>   {
>   char data;
>   };
>   
>   struct main
>   {
>   constexpr main()
>   {
>   member = {};
>   }
>   
>   sub member;
>   };
>   
>   constexpr main a{};
>
> With:
>
>   fmt.cpp:16:16: error: constexpr variable 'a' must be initialized by a 
> constant expression
>   constexpr main a{};
>  ^~~
>   1 error generated.
>
> Clang trunk and GCC (Debian 11.2.0-10) handle it fine.
> Noticed in libfmt 
> .

Strange, I see that it cannot be compiled neither by gcc nor by clang: 
https://godbolt.org/z/1dY9Gs6zM. Do I miss something?




Comment at: clang/lib/AST/ExprConstant.cpp:1584-1585
   Designator = SubobjectDesignator(getType(B));
+  if (!LExpr)
+LExpr = B.dyn_cast();
   IsNullPtr = false;

aaron.ballman wrote:
> Should we be asserting that `LExpr` is null?
`LExpr` is initialized only once, when LValue starts evaluation. Later on the 
evaluation can proceed to subexpressions but the upper expression is sufficient 
to detect active union member change.



Comment at: clang/lib/AST/ExprConstant.cpp:8049
 
+  bool evaluate(const Expr *E) {
+Result.LExpr = E;

aaron.ballman wrote:
> It's not super clear to me when consumers should call `Evaluate()` instead of 
> calling `Visit()`. Some comments on the function may help make it more clear.
`Visit` methods are now made non-public, hopefully it can make usage more clear.



Comment at: clang/lib/AST/ExprConstant.cpp:8050
+  bool evaluate(const Expr *E) {
+Result.LExpr = E;
+return Visit(E);

aaron.ballman wrote:
> Should we be asserting that `Result.LExpr` is not already set?
`Result.LExpr` is updated while evaluator descend the evaluated tree, so it is 
normal to see it set. 



Comment at: clang/lib/AST/ExprConstant.cpp:8586
 
+  bool evaluate(const Expr *E) { return Visit(E); }
+

aaron.ballman wrote:
> Is there a reason we're not setting `Result.LExpr` here?
Actually This method was used in the initial implementation, now it can be 
removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 384386.
sepavloff added a comment.

Updated patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp

Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1447,3 +1447,11 @@
   constexpr bool b = [a = S(), b = S()] { return a.p == b.p; }();
   static_assert(!b);
 }
+
+namespace PR45879 {
+struct Base {
+  int m;
+};
+struct Derived : Base {};
+constexpr int k = ((Base{} = Derived{}), 0);
+} // namespace PR45879
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -1546,6 +1546,7 @@
 APValue::LValueBase Base;
 CharUnits Offset;
 SubobjectDesignator Designator;
+const Expr *LExpr = nullptr;
 bool IsNullPtr : 1;
 bool InvalidBase : 1;
 
@@ -1554,6 +1555,7 @@
 const CharUnits () const { return Offset; }
 SubobjectDesignator () { return Designator; }
 const SubobjectDesignator () const { return Designator;}
+const Expr *getExpr() const { return LExpr; }
 bool isNullPointer() const { return IsNullPtr;}
 
 unsigned getLValueCallIndex() const { return Base.getCallIndex(); }
@@ -1591,6 +1593,8 @@
   Offset = CharUnits::fromQuantity(0);
   InvalidBase = BInvalid;
   Designator = SubobjectDesignator(getType(B));
+  if (!LExpr)
+LExpr = B.dyn_cast();
   IsNullPtr = false;
 }
 
@@ -6100,8 +6104,11 @@
 if (!handleTrivialCopy(Info, MD->getParamDecl(0), Args[0], RHSValue,
MD->getParent()->isUnion()))
   return false;
+const Expr *LHS = This->getExpr();
+if (!LHS)
+  return false;
 if (Info.getLangOpts().CPlusPlus20 && MD->isTrivial() &&
-!HandleUnionActiveMemberChange(Info, Args[0], *This))
+!HandleUnionActiveMemberChange(Info, LHS, *This))
   return false;
 if (!handleAssignment(Info, Args[0], *This, MD->getThisType(),
   RHSValue))
@@ -8058,10 +8065,20 @@
 namespace {
 class LValueExprEvaluator
   : public LValueExprEvaluatorBase {
+  friend class LValueExprEvaluatorBase;
+  friend class ExprEvaluatorBase;
+  friend class ConstStmtVisitor;
+  friend class StmtVisitorBase;
 public:
   LValueExprEvaluator(EvalInfo , LValue , bool InvalidBaseOK) :
 LValueExprEvaluatorBaseTy(Info, Result, InvalidBaseOK) {}
 
+  bool Evaluate(const Expr *E) {
+Result.LExpr = E;
+return Visit(E);
+  }
+
+protected:
   bool VisitVarDecl(const Expr *E, const VarDecl *VD);
   bool VisitUnaryPreIncDec(const UnaryOperator *UO);
 
@@ -8123,7 +8140,7 @@
   assert(!E->isValueDependent());
   assert(E->isGLValue() || E->getType()->isFunctionType() ||
  E->getType()->isVoidType() || isa(E));
-  return LValueExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
+  return LValueExprEvaluator(Info, Result, InvalidBaseOK).Evaluate(E);
 }
 
 bool LValueExprEvaluator::VisitDeclRefExpr(const DeclRefExpr *E) {
@@ -8424,7 +8441,7 @@
 
   // C++17 onwards require that we evaluate the RHS first.
   APValue RHS;
-  if (!Evaluate(RHS, this->Info, CAO->getRHS())) {
+  if (!::Evaluate(RHS, this->Info, CAO->getRHS())) {
 if (!Info.noteFailure())
   return false;
 Success = false;
@@ -8448,7 +8465,7 @@
 
   // C++17 onwards require that we evaluate the RHS first.
   APValue NewVal;
-  if (!Evaluate(NewVal, this->Info, E->getRHS())) {
+  if (!::Evaluate(NewVal, this->Info, E->getRHS())) {
 if (!Info.noteFailure())
   return false;
 Success = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

This breaks the following code:

  struct sub
  {
char data;
  };
  
  struct main
  {
constexpr main()
{
member = {};
}
  
sub member;
  };
  
  constexpr main a{};

With:

  fmt.cpp:16:16: error: constexpr variable 'a' must be initialized by a 
constant expression
  constexpr main a{};
 ^~~
  1 error generated.

Clang trunk and GCC (Debian 11.2.0-10) handle it fine.
Noticed in libfmt 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

Trying to help get this review going again as it impacts libc++.




Comment at: clang/lib/AST/ExprConstant.cpp:1584-1585
   Designator = SubobjectDesignator(getType(B));
+  if (!LExpr)
+LExpr = B.dyn_cast();
   IsNullPtr = false;

Should we be asserting that `LExpr` is null?



Comment at: clang/lib/AST/ExprConstant.cpp:8049
 
+  bool evaluate(const Expr *E) {
+Result.LExpr = E;

It's not super clear to me when consumers should call `Evaluate()` instead of 
calling `Visit()`. Some comments on the function may help make it more clear.



Comment at: clang/lib/AST/ExprConstant.cpp:8050
+  bool evaluate(const Expr *E) {
+Result.LExpr = E;
+return Visit(E);

Should we be asserting that `Result.LExpr` is not already set?



Comment at: clang/lib/AST/ExprConstant.cpp:8586
 
+  bool evaluate(const Expr *E) { return Visit(E); }
+

Is there a reason we're not setting `Result.LExpr` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

Gentle ping. I can't review this, but if someone can, it would be awesome. 
https://bugs.llvm.org/show_bug.cgi?id=45879 is breaking some libc++ tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

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

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 357988.
sepavloff added a comment.

Rebased patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp


Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1447,3 +1447,11 @@
   constexpr bool b = [a = S(), b = S()] { return a.p == b.p; }();
   static_assert(!b);
 }
+
+namespace PR45879 {
+struct Base {
+  int m;
+};
+struct Derived : Base {};
+constexpr int k = ((Base{} = Derived{}), 0);
+} // namespace PR45879
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -1534,6 +1534,7 @@
 APValue::LValueBase Base;
 CharUnits Offset;
 SubobjectDesignator Designator;
+const Expr *LExpr = nullptr;
 bool IsNullPtr : 1;
 bool InvalidBase : 1;
 
@@ -1542,6 +1543,7 @@
 const CharUnits () const { return Offset; }
 SubobjectDesignator () { return Designator; }
 const SubobjectDesignator () const { return 
Designator;}
+const Expr *getExpr() const { return LExpr; }
 bool isNullPointer() const { return IsNullPtr;}
 
 unsigned getLValueCallIndex() const { return Base.getCallIndex(); }
@@ -1579,6 +1581,8 @@
   Offset = CharUnits::fromQuantity(0);
   InvalidBase = BInvalid;
   Designator = SubobjectDesignator(getType(B));
+  if (!LExpr)
+LExpr = B.dyn_cast();
   IsNullPtr = false;
 }
 
@@ -6077,8 +6081,11 @@
 if (!handleTrivialCopy(Info, MD->getParamDecl(0), Args[0], RHSValue,
MD->getParent()->isUnion()))
   return false;
+const Expr *LHS = This->getExpr();
+if (!LHS)
+  return false;
 if (Info.getLangOpts().CPlusPlus20 && MD->isTrivial() &&
-!HandleUnionActiveMemberChange(Info, Args[0], *This))
+!HandleUnionActiveMemberChange(Info, LHS, *This))
   return false;
 if (!handleAssignment(Info, Args[0], *This, MD->getThisType(),
   RHSValue))
@@ -8039,6 +8046,11 @@
   LValueExprEvaluator(EvalInfo , LValue , bool InvalidBaseOK) :
 LValueExprEvaluatorBaseTy(Info, Result, InvalidBaseOK) {}
 
+  bool evaluate(const Expr *E) {
+Result.LExpr = E;
+return Visit(E);
+  }
+
   bool VisitVarDecl(const Expr *E, const VarDecl *VD);
   bool VisitUnaryPreIncDec(const UnaryOperator *UO);
 
@@ -8100,7 +8112,7 @@
   assert(!E->isValueDependent());
   assert(E->isGLValue() || E->getType()->isFunctionType() ||
  E->getType()->isVoidType() || isa(E));
-  return LValueExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
+  return LValueExprEvaluator(Info, Result, InvalidBaseOK).evaluate(E);
 }
 
 bool LValueExprEvaluator::VisitDeclRefExpr(const DeclRefExpr *E) {
@@ -8571,6 +8583,8 @@
   : ExprEvaluatorBaseTy(info), Result(Result),
 InvalidBaseOK(InvalidBaseOK) {}
 
+  bool evaluate(const Expr *E) { return Visit(E); }
+
   bool Success(const APValue , const Expr *E) {
 Result.setFrom(Info.Ctx, V);
 return true;
@@ -8680,7 +8694,7 @@
 bool InvalidBaseOK) {
   assert(!E->isValueDependent());
   assert(E->isPRValue() && E->getType()->hasPointerRepresentation());
-  return PointerExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
+  return PointerExprEvaluator(Info, Result, InvalidBaseOK).evaluate(E);
 }
 
 bool PointerExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {


Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1447,3 +1447,11 @@
   constexpr bool b = [a = S(), b = S()] { return a.p == b.p; }();
   static_assert(!b);
 }
+
+namespace PR45879 {
+struct Base {
+  int m;
+};
+struct Derived : Base {};
+constexpr int k = ((Base{} = Derived{}), 0);
+} // namespace PR45879
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -1534,6 +1534,7 @@
 APValue::LValueBase Base;
 CharUnits Offset;
 SubobjectDesignator Designator;
+const Expr *LExpr = nullptr;
 bool IsNullPtr : 1;
 bool InvalidBase : 1;
 
@@ -1542,6 +1543,7 @@
 const CharUnits () const { return Offset; }
 SubobjectDesignator () { return Designator; }
 const SubobjectDesignator () const { return Designator;}
+const Expr *getExpr() const { return LExpr; }
 bool isNullPointer() const { return IsNullPtr;}
 
 unsigned getLValueCallIndex() const { return Base.getCallIndex(); 

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff created this revision.
sepavloff added a reviewer: rsmith.
sepavloff requested review of this revision.
Herald added a project: clang.

Class LValue keeps result of lvalue evaluation. In some analyses it is
also necessary to have access to the original expression. This change
adds new member to LValue to keep the expression and initializes this
member when an lvalue is evaluated using LValueExprEvaluator.

With this change LHS expression becomes available in
HandleUnionActiveMemberChange and it becomes possible to fix PR45879.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103395

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp


Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1447,3 +1447,11 @@
   constexpr bool b = [a = S(), b = S()] { return a.p == b.p; }();
   static_assert(!b);
 }
+
+namespace PR45879 {
+struct Base {
+  int m;
+};
+struct Derived : Base {};
+constexpr int k = ((Base{} = Derived{}), 0);
+} // namespace PR45879
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -1534,6 +1534,7 @@
 APValue::LValueBase Base;
 CharUnits Offset;
 SubobjectDesignator Designator;
+const Expr *LExpr = nullptr;
 bool IsNullPtr : 1;
 bool InvalidBase : 1;
 
@@ -1542,6 +1543,7 @@
 const CharUnits () const { return Offset; }
 SubobjectDesignator () { return Designator; }
 const SubobjectDesignator () const { return 
Designator;}
+const Expr *getExpr() const { return LExpr; }
 bool isNullPointer() const { return IsNullPtr;}
 
 unsigned getLValueCallIndex() const { return Base.getCallIndex(); }
@@ -1579,6 +1581,8 @@
   Offset = CharUnits::fromQuantity(0);
   InvalidBase = BInvalid;
   Designator = SubobjectDesignator(getType(B));
+  if (!LExpr)
+LExpr = B.dyn_cast();
   IsNullPtr = false;
 }
 
@@ -6077,8 +6081,11 @@
 if (!handleTrivialCopy(Info, MD->getParamDecl(0), Args[0], RHSValue,
MD->getParent()->isUnion()))
   return false;
+const Expr *LHS = This->getExpr();
+if (!LHS)
+  return false;
 if (Info.getLangOpts().CPlusPlus20 && MD->isTrivial() &&
-!HandleUnionActiveMemberChange(Info, Args[0], *This))
+!HandleUnionActiveMemberChange(Info, LHS, *This))
   return false;
 if (!handleAssignment(Info, Args[0], *This, MD->getThisType(),
   RHSValue))
@@ -8039,6 +8046,11 @@
   LValueExprEvaluator(EvalInfo , LValue , bool InvalidBaseOK) :
 LValueExprEvaluatorBaseTy(Info, Result, InvalidBaseOK) {}
 
+  bool evaluate(const Expr *E) {
+Result.LExpr = E;
+return Visit(E);
+  }
+
   bool VisitVarDecl(const Expr *E, const VarDecl *VD);
   bool VisitUnaryPreIncDec(const UnaryOperator *UO);
 
@@ -8100,7 +8112,7 @@
   assert(!E->isValueDependent());
   assert(E->isGLValue() || E->getType()->isFunctionType() ||
  E->getType()->isVoidType() || isa(E));
-  return LValueExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
+  return LValueExprEvaluator(Info, Result, InvalidBaseOK).evaluate(E);
 }
 
 bool LValueExprEvaluator::VisitDeclRefExpr(const DeclRefExpr *E) {
@@ -8571,6 +8583,8 @@
   : ExprEvaluatorBaseTy(info), Result(Result),
 InvalidBaseOK(InvalidBaseOK) {}
 
+  bool evaluate(const Expr *E) { return Visit(E); }
+
   bool Success(const APValue , const Expr *E) {
 Result.setFrom(Info.Ctx, V);
 return true;
@@ -8680,7 +8694,7 @@
 bool InvalidBaseOK) {
   assert(!E->isValueDependent());
   assert(E->isRValue() && E->getType()->hasPointerRepresentation());
-  return PointerExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
+  return PointerExprEvaluator(Info, Result, InvalidBaseOK).evaluate(E);
 }
 
 bool PointerExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {


Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1447,3 +1447,11 @@
   constexpr bool b = [a = S(), b = S()] { return a.p == b.p; }();
   static_assert(!b);
 }
+
+namespace PR45879 {
+struct Base {
+  int m;
+};
+struct Derived : Base {};
+constexpr int k = ((Base{} = Derived{}), 0);
+} // namespace PR45879
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -1534,6 +1534,7 @@
 APValue::LValueBase Base;
 CharUnits Offset;
 SubobjectDesignator Designator;
+const Expr *LExpr = nullptr;
 bool IsNullPtr : 1;