PR45879: Keep evaluated expression in LValue object

[sepavloff]

Summary

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.

[sepavloff]

Rebased patch

[ldionne]

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.

[aaron.ballman]

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

clang/lib/AST/ExprConstant.cpp

1584–1585

Should we be asserting that LExpr is null?

8049

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.

8050

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

8586

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

[tambre]

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.

[sepavloff]

Updated patch

[sepavloff]

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?

clang/lib/AST/ExprConstant.cpp

1584–1585

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.

8049

Visit methods are now made non-public, hopefully it can make usage more clear.

8050

Result.LExpr is updated while evaluator descend the evaluated tree, so it is normal to see it set.

8586

Actually This method was used in the initial implementation, now it can be removed.

[tambre]

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.

[sepavloff]

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.

[sepavloff]

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.

[sepavloff]

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.

[ldionne]

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

[rsmith]

clang/lib/AST/ExprConstant.cpp

1537

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...

[rsmith]

Alternative fix landed in rG30baa5d2a450d5e302d8cba3fc7a26a59d4b7ae1.

[sepavloff]

@rsmith, thank you!

[ldionne]

@rsmith Thanks a bunch!

And thanks @sepavloff for trying out this approach too.

clang/lib/AST/ExprConstant.cpp

void setFrom(const APValue &v) {
       }
     }
   };
 
   struct LValue {
     APValue::LValueBase Base;
     CharUnits Offset;
     SubobjectDesignator Designator;
+    const Expr *LExpr = nullptr;
     bool IsNullPtr : 1;
     bool InvalidBase : 1;
 
     const APValue::LValueBase getLValueBase() const { return Base; }
     CharUnits &getLValueOffset() { return Offset; }
     const CharUnits &getLValueOffset() const { return Offset; }
     SubobjectDesignator &getLValueDesignator() { return Designator; }
     const SubobjectDesignator &getLValueDesignator() const { return Designator;}
+    const Expr *getExpr() const { return LExpr; }
     bool isNullPointer() const { return IsNullPtr;}
 
     unsigned getLValueCallIndex() const { return Base.getCallIndex(); }
     unsigned getLValueVersion() const { return Base.getVersion(); }
 
     void moveInto(APValue &V) const {
       if (Designator.Invalid)
         V = APValue(Base, Offset, APValue::NoLValuePath(), IsNullPtr);
#ifndef NDEBUG
                "Unexpected type of invalid base");
       }
 #endif
 
       Base = B;
       Offset = CharUnits::fromQuantity(0);
       InvalidBase = BInvalid;
       Designator = SubobjectDesignator(getType(B));
+      if (!LExpr)
+        LExpr = B.dyn_cast<const Expr *>();
       IsNullPtr = false;
     }
 
     void setNull(ASTContext &Ctx, QualType PointerTy) {
       Base = (const ValueDecl *)nullptr;
       Offset =
           CharUnits::fromQuantity(Ctx.getTargetNullPointerValue(PointerTy));
       InvalidBase = false;
if (MD && MD->isDefaulted() &&
        (MD->isTrivial() &&
         isReadByLvalueToRvalueConversion(MD->getParent())))) {
     assert(This &&
            (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()));
     APValue RHSValue;
     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))
       return false;
     This->moveInto(Result);
     return true;
   } else if (MD && isLambdaCallOperator(MD)) {
     // We're in a lambda; determine the lambda capture field maps unless we're

 //===----------------------------------------------------------------------===//
 namespace {
 class LValueExprEvaluator
   : public LValueExprEvaluatorBase<LValueExprEvaluator> {
 public:
   LValueExprEvaluator(EvalInfo &Info, LValue &Result, 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);
 
   bool VisitDeclRefExpr(const DeclRefExpr *E);
   bool VisitPredefinedExpr(const PredefinedExpr *E) { return Success(E); }
   bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *E);
   bool VisitCompoundLiteralExpr(const CompoundLiteralExpr *E);
   bool VisitMemberExpr(const MemberExpr *E);

 ///  * function designators in C, and
 ///  * "extern void" objects
 ///  * @selector() expressions in Objective-C
 static bool EvaluateLValue(const Expr *E, LValue &Result, EvalInfo &Info,
                            bool InvalidBaseOK) {
   assert(!E->isValueDependent());
   assert(E->isGLValue() || E->getType()->isFunctionType() ||
          E->getType()->isVoidType() || isa<ObjCSelectorExpr>(E));
-  return LValueExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
+  return LValueExprEvaluator(Info, Result, InvalidBaseOK).evaluate(E);
 }
 
 bool LValueExprEvaluator::VisitDeclRefExpr(const DeclRefExpr *E) {
   const NamedDecl *D = E->getDecl();
   if (isa<FunctionDecl, MSGuidDecl, TemplateParamObjectDecl>(D))
     return Success(cast<ValueDecl>(D));
   if (const VarDecl *VD = dyn_cast<VarDecl>(D))
     return VisitVarDecl(E, VD);
class PointerExprEvaluator
 
   bool visitNonBuiltinCallExpr(const CallExpr *E);
 public:
 
   PointerExprEvaluator(EvalInfo &info, LValue &Result, bool InvalidBaseOK)
       : ExprEvaluatorBaseTy(info), Result(Result),
         InvalidBaseOK(InvalidBaseOK) {}
 
+  bool evaluate(const Expr *E) { return Visit(E); }
+
   bool Success(const APValue &V, const Expr *E) {
     Result.setFrom(Info.Ctx, V);
     return true;
   }
   bool ZeroInitialization(const Expr *E) {
     Result.setNull(Info.Ctx, E->getType());
     return true;
   }
public:
   // FIXME: Missing: @protocol, @selector
 };
 } // end anonymous namespace
 
 static bool EvaluatePointer(const Expr* E, LValue& Result, EvalInfo &Info,
                             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) {
   if (E->getOpcode() != BO_Add &&
       E->getOpcode() != BO_Sub)
     return ExprEvaluatorBaseTy::VisitBinaryOperator(E);
 
   const Expr *PExp = E->getLHS();

clang/test/SemaCXX/constant-expression-cxx2a.cpp

namespace PR48582 {
   struct S {
     void *p = this;
     constexpr S() {}
     constexpr S(const S&) {}
   };
   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