nickdesaulniers added inline comments.
================ Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. - return CGM.GetConstantArrayFromStringLiteral(E); + return E->isLValue() ? + CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() : ---------------- efriedma wrote: > nickdesaulniers wrote: > > efriedma wrote: > > > efriedma wrote: > > > > efriedma wrote: > > > > > nickdesaulniers wrote: > > > > > > efriedma wrote: > > > > > > > nickdesaulniers wrote: > > > > > > > > nickdesaulniers wrote: > > > > > > > > > efriedma wrote: > > > > > > > > > > nickdesaulniers wrote: > > > > > > > > > > > efriedma wrote: > > > > > > > > > > > > Maybe we should have a separate > > > > > > > > > > > > ConstExprEmitterLValue... trying to handle both LValues > > > > > > > > > > > > and RValues on the same codepath has been problematic > > > > > > > > > > > > in the past. It's very easy for code to get confused > > > > > > > > > > > > what it's actually trying to emit. > > > > > > > > > > > So we'd have a `ConstExprEmitterLValue` class with some > > > > > > > > > > > visitor methods, and a `ConstExprEmitterRValue` with > > > > > > > > > > > other methods implemented? > > > > > > > > > > Something like that. > > > > > > > > > > > > > > > > > > > > Actually thinking about it a bit more, not sure you need to > > > > > > > > > > actually implement ConstExprEmitterLValue for now. You > > > > > > > > > > might just be able to ensure we don't ever call > > > > > > > > > > ConstExprEmitter with an lvalue. The current > > > > > > > > > > ConstExprEmitter doesn't expect lvalues, and shouldn't call > > > > > > > > > > itself with lvalues. (We bail on explicit LValueToRValue > > > > > > > > > > conversions.) And Evaluate() shouldn't actually evaluate > > > > > > > > > > the contents of an lvalue if it isn't dereferenced, so > > > > > > > > > > there hopefully aren't any performance issues using that > > > > > > > > > > codepath. > > > > > > > > > > > > > > > > > > > > In terms of implementation, I guess that's basically > > > > > > > > > > restoring the destType->isReferenceType() that got removed? > > > > > > > > > > (I know I suggested it, but I wasn't really thinking about > > > > > > > > > > it...) > > > > > > > > > One thing I think we may need to add to `ConstExprEmitter` is > > > > > > > > > the ability to evaluate `CallExpr`s based on certain test > > > > > > > > > case failures...does that seem right? > > > > > > > > See also the calls to `constexpr f()` in > > > > > > > > clang/test/CodeGenCXX/const-init-cxx1y.cpp > > > > > > > The only things I know of that Evaluate() can't handle are > > > > > > > CK_ToUnion, CK_ReinterpretMemberPointer, and > > > > > > > DesignatedInitUpdateExpr. DesignatedInitUpdateExpr is related to > > > > > > > the failures in test/CodeGenCXX/designated-init.cpp; I don't > > > > > > > think the others show up in any of the testcases you've > > > > > > > mentioned. (CK_ToUnion can't appear in C++ code. > > > > > > > CK_ReinterpretMemberPointer is a `reinterpret_cast<T>` where T is > > > > > > > a member pointer type.) > > > > > > > > > > > > > > Given none of those constructs show up in const-init-cxx1y.cpp, > > > > > > > the only reason for it to fail is if we aren't correctly falling > > > > > > > back for a construct the current code doesn't know how to handle. > > > > > > > You shouldn't need to implement any new constructs. > > > > > > in clang/test/CodeGenCXX/designated-init.cpp we have: > > > > > > ``` > > > > > > >> 22 namespace ModifyStaticTemporary { > > > > > > >> > > > > > > 23 struct A { int &&temporary; int x; }; > > > > > > > > > > > > 24 constexpr int f(int &r) { r *= 9; return r - 12; } > > > > > > > > > > > > 25 A a = { 6, f(a.temporary) }; > > > > > > ``` > > > > > > In the AST, that looks like: > > > > > > ``` > > > > > > | |-VarDecl 0x562b77df39b0 <line:25:3, col:29> col:5 used a > > > > > > 'A':'ModifyStaticTemporary::A' cinit > > > > > > | | `-ExprWithCleanups 0x562b77df3c68 <col:9, col:29> > > > > > > 'A':'ModifyStaticTemporary::A' > > > > > > | | `-InitListExpr 0x562b77df3bb8 <col:9, col:29> > > > > > > 'A':'ModifyStaticTemporary::A' > > > > > > | | |-MaterializeTemporaryExpr 0x562b77df3c08 <col:11> 'int' > > > > > > xvalue extended by Var 0x562b77df39b0 'a' > > > > > > 'A':'ModifyStaticTemporary::A' > > > > > > | | | `-IntegerLiteral 0x562b77df3a18 <col:11> 'int' 6 > > > > > > | | `-CallExpr 0x562b77df3b30 <col:14, col:27> 'int' > > > > > > | | |-ImplicitCastExpr 0x562b77df3b18 <col:14> 'int (*)(int > > > > > > &)' <FunctionToPointerDecay> > > > > > > | | | `-DeclRefExpr 0x562b77df3ad0 <col:14> 'int (int &)' > > > > > > lvalue Function 0x562b77df37a0 'f' 'int (int &)' > > > > > > | | `-MemberExpr 0x562b77df3aa0 <col:16, col:18> 'int' lvalue > > > > > > .temporary 0x562b77df35c0 > > > > > > | | `-DeclRefExpr 0x562b77df3a80 <col:16> > > > > > > 'A':'ModifyStaticTemporary::A' lvalue Var 0x562b77df39b0 'a' > > > > > > 'A':'ModifyStaticTemporary::A' > > > > > > ``` > > > > > > (So, indeed no `DesignatedInitUpdateExpr`) but the call to the > > > > > > `constexpr` `f()` updates the reference (to `54`). If I remove the > > > > > > visitor for `MaterializeTemporaryExpr`, we fail to evaluate `f` and > > > > > > end up emitting `6` rather than `54`. Doesn't that mean that the > > > > > > fast path (`ConstExprEmitter`) needs to be able to evaluate > > > > > > `CallExpr`? > > > > > > > > > > > > Or should `VisitInitListExpr` bail if any of the inits > > > > > > `isa<MaterializeTemporaryExpr>` (or perhaps `isa<CallExpr>`)? > > > > > There are a few related cases here. > > > > > > > > > > Case number one is when you have something like `int z(); A a = { > > > > > z(), z() };`. There's no constant evaluation going on: you just emit > > > > > two zero-initialized variables, and the runtime init initializes both > > > > > of them. > > > > > > > > > > Case number two is when everything is obviously constant: something > > > > > like `A a = { 1, 2 };` > > > > > > > > > > Case number three is when there are simple side-effects, and the > > > > > standard requires we evaluate them at compile-time. Something like > > > > > `A a = { 1, ++a.temporary };`. In this case, we need to ensure that > > > > > we use Evaluate() to compute the value of both the temporary and the > > > > > variable. The literal "1" is not the correct value to use. > > > > > CodeGenModule::GetAddrOfGlobalTemporary is supposed to ensure we use > > > > > the value from the evaluation of the variable as a whole (see comment > > > > > "If the initializer of the extending declaration"). > > > > > > > > > > Case number four is when we can't constant-evaluate a variable as a > > > > > whole, but we do evaluate some of the temporaries involved. > > > > > Something like `int z(); A a = { 1, a.temporary += z() };` In this > > > > > case, we constant-evaluate the temporary using the initial value, > > > > > then emit runtime initialization to finish computing the value of the > > > > > variable as a whole. > > > > > > > > > > You example should fall under case three. Both the temporary and the > > > > > variable should be evaluated by Evaluate(). > > > > > > > > > > I'm not sure how the code ends up emitting the value 6, but hopefully > > > > > that helps? > > > > Oh, I think I see what's happening; the code that looks for the > > > > temporary in GetAddrOfGlobalTemporary isn't reliable if the whole > > > > variable isn't evaluated first. It ends up pulling out the result of a > > > > partial evaluation, or something like that. > > > > > > > > Making EmitArrayInitialization/EmitRecordInitialization bail if they > > > > see a MaterializeTemporaryExpr should deal with the issue, I think? > > > > Not sure if you'd need to recursively visit all the initializers (I > > > > don't remember what constructs allow lifetime extension off the top of > > > > my head). > > > To be more precise, what happens is that calling EvaluateAsLValue on the > > > MaterializeTemporaryExpr actually *corrupts* the computed value of the > > > temporary: the complete variable is evaluated earlier for other reasons, > > > then EvaluateAsLValue overwrites the correct value we computed earlier > > > with the wrong value. > > > In this case, we need to ensure that we use Evaluate() to compute the > > > value of both the temporary and the variable. > > > > Just triple checking, `Evaluate` is the "slow path" (i.e. > > `VarDecl::evaluateValue`, `Expr::EvaluateAsLValue`, and > > `Expr::EvaluateAsRValue`? > > > > > To be more precise, what happens is that calling EvaluateAsLValue on the > > > MaterializeTemporaryExpr actually *corrupts* the computed value of the > > > temporary > > > > So the slow path gets it wrong? But prior to this patch, that's was used > > first before ConstExprEmitter? (Maybe I should add more comments about > > fast vs slow path) > > > > --- > > > > > Not sure if you'd need to recursively visit all the initializers (I don't > > > remember what constructs allow lifetime extension off the top of my head). > > > > I think I would; the last test case currently failing is > > clang/test/CodeGenCXX/atomicinit.cpp: > > ``` > > struct X { > > constexpr X(int n) : n(n) {} > > short n; > > char c = 6; > > }; > > > > struct Y { > > _Atomic(X) a; > > _Atomic(int) b; > > }; > > Y y = { X(4), 5 }; > > ``` > > The AST for `y` looks like: > > ``` > > `-VarDecl 0x562bba0cad00 <line:11:1, col:17> col:3 y 'Y':'Y' cinit > > `-ExprWithCleanups 0x562bba0e9f28 <col:7, col:17> 'Y':'Y' > > `-InitListExpr 0x562bba0e9c20 <col:7, col:17> 'Y':'Y' > > |-ImplicitCastExpr 0x562bba0e9ef8 <col:9, col:12> '_Atomic(X)' > > <NonAtomicToAtomic> > > | `-ImplicitCastExpr 0x562bba0e9ee0 <col:9, col:12> 'X':'X' > > <ConstructorConversion> > > | `-CXXConstructExpr 0x562bba0e9eb0 <col:9, col:12> 'X':'X' 'void > > (X &&) noexcept' elidable > > | `-MaterializeTemporaryExpr 0x562bba0e9c70 <col:9, col:12> > > 'X':'X' xvalue > > | `-CXXFunctionalCastExpr 0x562bba0e9b88 <col:9, col:12> > > 'X':'X' functional cast to X <ConstructorConversion> > > | `-CXXConstructExpr 0x562bba0e9a10 <col:9, col:12> 'X':'X' > > 'void (int)' > > | `-IntegerLiteral 0x562bba0cadc0 <col:11> 'int' 4 > > `-ImplicitCastExpr 0x562bba0e9f10 <col:15> '_Atomic(int)' > > <NonAtomicToAtomic> > > `-IntegerLiteral 0x562bba0e9bb0 <col:15> 'int' 5 > > ``` > > so we'd need to peek through the casts to find that there was a > > `MaterializeTemporaryExpr` in there, then bail? > > Just triple checking, Evaluate is the "slow path" (i.e. > > VarDecl::evaluateValue, Expr::EvaluateAsLValue, and Expr::EvaluateAsRValue? > > Yes. > > >> To be more precise, what happens is that calling EvaluateAsLValue on the > >> MaterializeTemporaryExpr actually *corrupts* the computed value of the > >> temporary > > So the slow path gets it wrong? But prior to this patch, that's was used > > first before ConstExprEmitter? (Maybe I should add more comments about fast > > vs slow path) > > The EvaluateAsLValue bug only shows up if you EvaluateAsLValue pieces of the > initializer. If you use the slow path first, we never EvaluateAsLValue > pieces of the initializer; we just evaluate the whole variable initializer in > one evaluation. (Depending on the construct, we may have to evaluate it > during semantic analysis; if we do, the evaluation is cached.) > > -------- > > > I think I would; the last test case currently failing is > > clang/test/CodeGenCXX/atomicinit.cpp: > > I don't think that's the same issue? There, the MaterializeTemporaryExpr is > immediately passed to a CXXConstructExpr, which returns an rvalue, so the > final IR shouldn't actually reference the temporary. It looks like the issue > is that VisitCXXConstructExpr is broken; it tries to look through a trivial > move constructor, but the the operand of a move constructor is an lvalue, so > the recursive visit doesn't work correctly. The following crashes even > without your patch: > > ``` > struct X { > constexpr X(int n) : n(n) {} > short n; > char c = 6; > }; > struct Y { > _Atomic(X) a; > int b; > }; > int z; > Y y = { X(4), z }; > ``` > > You can probably just kill off the VisitCXXConstructExpr codepath... or if > you want to try to repair it, I guess you can teach it to specifically handle > only trivial constructors where the operand is a MaterializeTemporaryExpr. > You can probably just kill off the VisitCXXConstructExpr codepath.. If I delete `ConstExprEmitter::VisitCXXConstructExpr` (the "fast path") then wont the slow path then fail in the same way as your example above that crashes even without my patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/ https://reviews.llvm.org/D151587 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits