[Clang] Propagate const context info when emitting compound literal

[stuij]

Summary

This patch fixes a crash when trying to emit a constant compound literal.

For C++ Clang evaluates either casts or binary operations at translation time,
but doesn't pass on the InConstantContext information that was inferred when
parsing the statement. Because of this, strict FP evaluation (-ftrapping-math)
which shouldn't be in effect yet, then causes checkFloatingpointResult to return
false, which in tryEmitGlobalCompoundLiteral will trigger an assert that the
compound literal wasn't constant.

The discussion here around 'manifestly constant evaluated contexts' was very
helpful to me when trying to understand what LLVM's position is on what
evaluation context should be in effect, together with the explanatory text in
that patch itself:
https://reviews.llvm.org/D87528

[simon_tatham]

The clang code change looks reasonable to me, but I'm not the most expert in that area.

The intended result certainly seems sensible, because the C90-compatible version of that code without a constant literal is happy to do double→float conversion at compile time without complaining about side effects:

static const float separate_array[1] = { 0.1, };
struct { const float *floats; } without_compound_literal = { separate_array };

and so it makes sense that the version with a compound literal should be treated no differently.

Nit in the commit message:

which should be in effect yet

Surely "which shouldn't be in effect yet"?

clang/test/CodeGen/const-init.c

1–3

I think some kind of a comment would be useful saying what this option is doing there -- at least, which one of the tests further down the file it's supposed to apply to. Otherwise I could easily imagine someone throwing it out again, and since the test would pass anyway, not noticing that it's no longer testing what it's meant to test.

[stuij]

addressed review comments

[stuij]

also added a comment above the runline. Initially thought it wouldn't be necessary as we're now doing a check, but I do agree that it's more clear to be specific about the cmdline arg.

[vhscampos]

clang/lib/CodeGen/CGExprConstant.cpp

2215–2216

This constructor has the second parameter optional anyway. I suggest you omit the nullptr here.

[vhscampos]

clang/lib/CodeGen/CGExprConstant.cpp

2215–2216

To clarify, when I say optional, I mean it has a default value with is already nullptr

[stuij]

addressed review comment

[stuij]

clang/lib/CodeGen/CGExprConstant.cpp

2215–2216

done. thanks!

clang/test/CodeGen/const-init.c

1–3

thanks, yes is done. I also added a check on generation of the float value.

[DavidSpickett]

clang/test/CodeGen/const-init.c

2

Drive by comment, what is "the right thing" here? Without me having to git blame this file if I'm looking at this months from now.

You could add a comment down next to the test like "clang should evaluate this in a constant context, so floating point mode should have no effect". Which is still a bit vague but better than "clang should not crash if we do this".

Also are there any other floating point modes that have/had this same problem? They should be tested too if so.

[rjmccall]

This seems reasonable.

[stuij]

addressed review comment

[stuij]

clang/test/CodeGen/const-init.c

2

Yea, that does make the intent clearer. Added.

[DavidSpickett]

My comment was addressed, LGTM.

clang/lib/CodeGen/CGExprConstant.cpp

bool ConstStructBuilder::UpdateStruct(ConstantEmitter &Emitter,
   return ConstStructBuilder(Emitter, Const, Offset)
       .Build(Updater, /*AllowOverwrite*/ true);
 }
 
 //===----------------------------------------------------------------------===//
 //                             ConstExprEmitter
 //===----------------------------------------------------------------------===//
 
-static ConstantAddress tryEmitGlobalCompoundLiteral(CodeGenModule &CGM,
-                                                    CodeGenFunction *CGF,
-                                              const CompoundLiteralExpr *E) {
+static ConstantAddress
+tryEmitGlobalCompoundLiteral(ConstantEmitter &emitter,
+  CodeGenModule &CGM = emitter.CGM;
   CharUnits Align = CGM.getContext().getTypeAlignInChars(E->getType());
   if (llvm::GlobalVariable *Addr =
           CGM.getAddrOfConstantCompoundLiteralIfEmitted(E))
     return ConstantAddress(Addr, Addr->getValueType(), Align);
 
   LangAS addressSpace = E->getType().getAddressSpace();
-
-  ConstantEmitter emitter(CGM, CGF);
   llvm::Constant *C = emitter.tryEmitForInitializer(E->getInitializer(),
                                                     addressSpace, E->getType());
   if (!C) {
     assert(!E->isFileScope() &&
            "file-scope compound literal did not have constant initializer!");
     return ConstantAddress::invalid();
   }
 

 ConstantLValueEmitter::VisitConstantExpr(const ConstantExpr *E) {
   if (llvm::Constant *Result = Emitter.tryEmitConstantExpr(E))
     return Result;
   return Visit(E->getSubExpr());
 }
 
 ConstantLValue
 ConstantLValueEmitter::VisitCompoundLiteralExpr(const CompoundLiteralExpr *E) {
-  return tryEmitGlobalCompoundLiteral(CGM, Emitter.CGF, E);
+  ConstantEmitter CompoundLiteralEmitter(CGM, Emitter.CGF);
+  CompoundLiteralEmitter.setInConstantContext(Emitter.isInConstantContext());
+  return tryEmitGlobalCompoundLiteral(CompoundLiteralEmitter, E);
 }
 
 ConstantLValue
 ConstantLValueEmitter::VisitStringLiteral(const StringLiteral *E) {
   return CGM.GetAddrOfConstantStringFromLiteral(E);
 }
 
 ConstantLValue
void CodeGenModule::setAddrOfConstantCompoundLiteral(
   bool Ok = EmittedCompoundLiterals.insert(std::make_pair(CLE, GV)).second;
   (void)Ok;
   assert(Ok && "CLE has already been emitted!");
 }
 
 ConstantAddress
 CodeGenModule::GetAddrOfConstantCompoundLiteral(const CompoundLiteralExpr *E) {
   assert(E->isFileScope() && "not a file-scope compound literal expr");
-  return tryEmitGlobalCompoundLiteral(*this, nullptr, E);
+  ConstantEmitter emitter(*this);
+  return tryEmitGlobalCompoundLiteral(emitter, E);
 }
 
 llvm::Constant *
 CodeGenModule::getMemberPointerConstant(const UnaryOperator *uo) {
   // Member pointer constants always have a very particular form.
   const MemberPointerType *type = cast<MemberPointerType>(uo->getType());
   const ValueDecl *decl = cast<DeclRefExpr>(uo->getSubExpr())->getDecl();
 

clang/lib/CodeGen/ConstantEmitter.h

public:
 
   ~ConstantEmitter();
 
   /// Is the current emission context abstract?
   bool isAbstract() const {
     return Abstract;
   }
 
+  bool isInConstantContext() const { return InConstantContext; }
+  void setInConstantContext(bool var) { InConstantContext = var; }
+
   /// Try to emit the initiaizer of the given declaration as an abstract
   /// constant.  If this succeeds, the emission must be finalized.
   llvm::Constant *tryEmitForInitializer(const VarDecl &D);
   llvm::Constant *tryEmitForInitializer(const Expr *E, LangAS destAddrSpace,
                                         QualType destType);
   llvm::Constant *emitForInitializer(const APValue &value, LangAS destAddrSpace,
                                      QualType destType);
 

clang/test/CodeGen/const-init.c

-// RUN: %clang_cc1 -no-opaque-pointers -triple i386-pc-linux-gnu -ffreestanding -Wno-pointer-to-int-cast -Wno-int-conversion -emit-llvm -o - %s | FileCheck %s
+// setting strict FP behaviour in the run line below tests that the compiler
+// does the right thing for global compound literals (compoundliteral test)
+// RUN: %clang_cc1 -no-opaque-pointers -triple i386-pc-linux-gnu -ffreestanding -Wno-pointer-to-int-cast -Wno-int-conversion -ffp-exception-behavior=strict -emit-llvm -o - %s | FileCheck %s
 
 #include <stdint.h>
 
 // Brace-enclosed string array initializers
 char a[] = { "asdf" };
 // CHECK: @a ={{.*}} global [5 x i8] c"asdf\00"
 
 char a2[2][5] = { "asdf" };
#pragma pack(4)
   static struct {
     short a;
     long x;
     short z;
   } a = {23122, -12312731, -312};
 #pragma pack()
   // CHECK: @g31.a = internal global %struct.anon.2 { i16 23122, i32 -12312731, i16 -312 }, align 4
 }
+
+// Clang should evaluate this in constant context, so floating point mode should
+// have no effect.
+// CHECK: @.compoundliteral = internal global [1 x float] [float 0x3FB99999A0000000], align 4
+struct { const float *floats; } compoundliteral = {
+  (float[1]) { 0.1, },
+};