[cfe-commits] r147026 - in /cfe/trunk: lib/AST/Expr.cpp test/Sema/static-init.c

[Richard Smith]

On Wed, December 21, 2011 02:55, Eli Friedman wrote:
> On Tue, Dec 20, 2011 at 6:39 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> Hi Eli,
>>
>> On Wed, December 21, 2011 00:43, Eli Friedman wrote:
>>
>>> Author: efriedma
>>> Date: Tue Dec 20 18:43:02 2011
>>> New Revision: 147026
>>>
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=147026&view=rev
>>> Log:
>>> Fix a case where Expr::isConstantInitializer would return true for an
>>> expression we can't support.  In a slightly amusing twist, the case in
>>> question was already in the clang regression tests marked as a valid
>>> construct.  <rdar://problem/10020074>
>>>
>>>
>>> Modified:
>>> cfe/trunk/lib/AST/Expr.cpp cfe/trunk/test/Sema/static-init.c
>>>
>>> Modified: cfe/trunk/lib/AST/Expr.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Expr.cpp?rev=147026&
>>> r1= 147025&r2=147026&view=diff
>>> ==========================================================================
>>> ====
>>>  --- cfe/trunk/lib/AST/Expr.cpp (original)
>>> +++ cfe/trunk/lib/AST/Expr.cpp Tue Dec 20 18:43:02 2011
>>> @@ -2550,17 +2550,30 @@
>>> if (getType()->isVectorType() && CE->getCastKind() == CK_BitCast) return
>>> CE->getSubExpr()->isConstantInitializer(Ctx, false);
>>>
>>>
>>>
>>> -    // Handle casts with a destination that's a struct or union; this
>>> -    // deals with both the gcc no-op struct cast extension and the
>>> -    // cast-to-union extension.
>>> -    if (getType()->isRecordType())
>>> +    // Handle misc casts we want to ignore.
>>> +    // FIXME: Is it really safe to ignore all these?
>>> +    if (CE->getCastKind() == CK_NoOp ||
>>> +        CE->getCastKind() == CK_LValueToRValue ||
>>> +        CE->getCastKind() == CK_ToUnion ||
>>> +        CE->getCastKind() == CK_ConstructorConversion)
>>> return CE->getSubExpr()->isConstantInitializer(Ctx, false);
>>>
>>> -    // Integer->integer casts can be handled here, which is important
>>> for -    // things like (int)(&&x-&&y).  Scary but true.
>>> -    if (getType()->isIntegerType() &&
>>> -        CE->getSubExpr()->getType()->isIntegerType())
>>> -      return CE->getSubExpr()->isConstantInitializer(Ctx, false);
>>> +    // Handle things like (int)(&&x-&&y). It's a bit nasty, but we
>>> support it. +    if (CE->getCastKind() == CK_IntegralCast) { +      const
>>> Expr *E = CE->getSubExpr()->IgnoreParenNoopCasts(Ctx);
>>> +      while (const CastExpr *InnerCE = dyn_cast<CastExpr>(E)) {
>>> +        if (InnerCE->getCastKind() != CK_IntegralCast)
>>> +          break;
>>> +        E = InnerCE->getSubExpr()->IgnoreParenNoopCasts(Ctx);
>>> +      }
>>> +
>>> +      if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
>>> +        if (BO->getOpcode() == BO_Sub &&
>>> +            isa<AddrLabelExpr>(BO->getLHS()->IgnoreParenNoopCasts(Ctx))
>>> &&
>>> +            isa<AddrLabelExpr>(BO->getRHS()->IgnoreParenNoopCasts(Ctx)))
>>> +          return true;
>>> +      }
>>> +    }
>>>
>>>
>>>
>>> break; }
>>>
>>>
>>> Modified: cfe/trunk/test/Sema/static-init.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/static-init.c?rev=
>>> 147
>>> 026&r1=147025&r2=147026&view=diff
>>> ==========================================================================
>>> ====
>>>  --- cfe/trunk/test/Sema/static-init.c (original)
>>> +++ cfe/trunk/test/Sema/static-init.c Tue Dec 20 18:43:02 2011
>>> @@ -19,5 +19,6 @@
>>> };
>>>
>>>
>>>
>>> union bar u[1]; -struct foo x = {(intptr_t) u}; // no-error +struct foo x
>>> = {(intptr_t) u}; // expected-error {{initializer element is not
>>> a compile-time constant}}
>>
>> A matching change to CGExprConstant is required, or we'll still emit IR
>> which can't be lowered in some cases (for instance, when building the same
>> test case in C++ mode).
>
> Err, great.  That will be a bit annoying to write.  It would make all
> the constant stuff a bit less complicated if Evaluate could handle all scalar
> expressions...

I agree, and we're very close. The cases we can't evaluate are:

 - difference of two address labels (would need a new APValue kind)
 - calls to trivial constructors (which are not constexpr)
 - vector bitcasts
 - cast to union

Supporting these in Evaluate would allow us to entirely ditch
isConstantInitializer and CGExprConstant's ConstExprEmitter, but... it
requires us to build an intermediate (currently APValue) representation for
all initializers, including large initializer lists (as appear in SPEC's
445.gobmk). That has a non-trivial performance and memory impact. I've been
working on this (including building a more memory-efficient constant value
representation), but it's not clear whether the performance penalty can
entirely be avoided.

- Richard