efriedma 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() :
----------------
nickdesaulniers wrote:
> 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?
I can't parse the question. What do you think is causing the _Atomic case to
crash?
I tried looking a bit more; my example might not actually be related to the
CXXConstructExpr. There's something weird going on with the NonAtomicToAtomic
cast.
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
