[clang][Interp] Record initialization via conditional operator

[tbaeder]

Summary

Factor this out into a visitConditional() that calls the given visitor function.

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

911

This meant to be here?

912

Is this condition supposed to be just a 'visit', or should this be using the functor?

clang/lib/AST/Interp/ByteCodeExprGen.h

185

I'd probably rather make this a template taking a functor, rather than bringing in the atrocities that come with std::function.

clang/test/AST/Interp/records.cpp

372

Did this not work before? I don't see what part of teh code changes makes this work?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

911

Nope! :(

912

The visit is correct here, the functor is just for the true/false expr.

clang/lib/AST/Interp/ByteCodeExprGen.h

185

Alright, will look into that

clang/test/AST/Interp/records.cpp

372

We call visitInitializer also when the return value is constructed in the calling function, so the return statement in getS() exercises the code added.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

185

Done. Not sure if this is exactly how you meant it to look. Feels a bit inelegant.

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.h

185

This actually probably needs to be a non-member now, since its only used in the .cpp.

while I DO dislike that it was using std::function (probably because of captures in the lambdas?), I now suspect this is worse if we can't have it in the header, right?

[tbaeder]

Ping

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.h

184–185

The template definition isn't available within the header file, so this is fragile (impossible to instantiate from anything but ByteCodeExprGen.cpp).

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

184–185

What's the alternative? If I make it a static function in ByteCodeExprGen.cpp, I'd have to make a lot of members of ByteCodeEmitter etc. public which is not a very clean solution. Moving the definition into the header file isn't very nice either, all the others are in the source file.

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.h

184–185

What about making it a static template in ByteCodeExprGen.cpp, but make it a 'friend' here? I think that would work, wouldn't it? Something like: https://godbolt.org/z/ofYbGvfYa

You unfortunately cannot make it static, but as it is a template, at least it is inline. WDYT?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

184–185

I tried that but I keep running into linker problems:

mold: error: undefined symbol: lib/libclangAST.a(ByteCodeExprGen.cpp.o): bool clang::interp::visitConditional<clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>::VisitAbstractConditionalOperator(clang::AbstractConditionalOperator const*)::{lambda(clang::Expr const*)#1}>(clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>*, clang::AbstractConditionalOperator const*, clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>::VisitAbstractConditionalOperator(clang::AbstractConditionalOperator const*)::{lambda(clang::Expr const*)#1})

(and another one for EvalEmitter). But I can't investigate more before Wednesday.

[tbaeder]

Screw it, just use a llvm::function_ref.

[aaron.ballman]

I'm fine with this approach.

clang/lib/AST/Interp/ByteCodeExprGen.h

if (!this->emitThis(I))
       return false;
 
     if (!visitInitializer(I))
       return false;
 
     return this->emitPopPtr(I);
   }
 
+  bool visitConditional(const AbstractConditionalOperator *E,
+                        llvm::function_ref<bool(const Expr *)> V);
+
   /// Creates a local primitive value.
   unsigned allocateLocalPrimitive(DeclTy &&Decl, PrimType Ty, bool IsMutable,
                                   bool IsExtended = false);
 
   /// Allocates a space storing a local given its type.
   std::optional<unsigned> allocateLocal(DeclTy &&Decl, bool IsExtended = false);
 
 private:

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
   return this->visit(E->getSourceExpr());
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitAbstractConditionalOperator(
     const AbstractConditionalOperator *E) {
-  const Expr *Condition = E->getCond();
-  const Expr *TrueExpr = E->getTrueExpr();
-  const Expr *FalseExpr = E->getFalseExpr();
-
-  LabelTy LabelEnd = this->getLabel();   // Label after the operator.
-  LabelTy LabelFalse = this->getLabel(); // Label for the false expr.
-
-  if (!this->visit(Condition))
-    return false;
-  if (!this->jumpFalse(LabelFalse))
-    return false;
-
-  if (!this->visit(TrueExpr))
-    return false;
-  if (!this->jump(LabelEnd))
-    return false;
-
-  this->emitLabel(LabelFalse);
-
-  if (!this->visit(FalseExpr))
-    return false;
-
-  this->fallthrough(LabelEnd);
-  this->emitLabel(LabelEnd);
-
-  return true;
+  return this->visitConditional(
+      E, [this](const Expr *E) { return this->visit(E); });
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitStringLiteral(const StringLiteral *E) {
   unsigned StringIndex = P.createGlobalString(E);
   return this->emitGetPtrGlobal(StringIndex, E);
 }
 

 bool ByteCodeExprGen<Emitter>::visitBool(const Expr *E) {
   if (std::optional<PrimType> T = classify(E->getType())) {
     return visit(E);
   } else {
     return this->bail(E);
   }
 }
 
+/// Visit a conditional operator, i.e. `A ? B : C`.
+/// \V determines what function to call for the B and C expressions.
+template <class Emitter>
+bool ByteCodeExprGen<Emitter>::visitConditional(
+    const AbstractConditionalOperator *E,
+    llvm::function_ref<bool(const Expr *)> V) {
+
+  const Expr *Condition = E->getCond();
+  const Expr *TrueExpr = E->getTrueExpr();
+  const Expr *FalseExpr = E->getFalseExpr();
+
+  LabelTy LabelEnd = this->getLabel();   // Label after the operator.
+  LabelTy LabelFalse = this->getLabel(); // Label for the false expr.
+
+  if (!this->visit(Condition))
+    return false;
+  if (!this->jumpFalse(LabelFalse))
+    return false;
+
+  if (!V(TrueExpr))
+    return false;
+  if (!this->jump(LabelEnd))
+    return false;
+
+  this->emitLabel(LabelFalse);
+
+  if (!V(FalseExpr))
+    return false;
+
+  this->fallthrough(LabelEnd);
+  this->emitLabel(LabelEnd);
+
+  return true;
+}
+
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitZeroInitializer(PrimType T, const Expr *E) {
   switch (T) {
   case PT_Bool:
     return this->emitZeroBool(E);
   case PT_Sint8:
     return this->emitZeroSint8(E);
   case PT_Uint8:
if (const auto CtorExpr = dyn_cast<CXXConstructExpr>(Initializer)) {
 
     return this->visit(CE);
   } else if (const auto *DIE = dyn_cast<CXXDefaultInitExpr>(Initializer)) {
     return this->visitInitializer(DIE->getExpr());
   } else if (const auto *CE = dyn_cast<CastExpr>(Initializer)) {
     return this->visitInitializer(CE->getSubExpr());
   } else if (const auto *CE = dyn_cast<CXXBindTemporaryExpr>(Initializer)) {
     return this->visitInitializer(CE->getSubExpr());
+  } else if (const auto *ACO =
+                 dyn_cast<AbstractConditionalOperator>(Initializer)) {
+    return this->visitConditional(
+        ACO, [this](const Expr *E) { return this->visitRecordInitializer(E); });
   }
 
   return false;
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitInitializer(const Expr *Initializer) {
   QualType InitializerType = Initializer->getType();

clang/test/AST/Interp/constexpr-nqueens.cpp

struct Board {
   uint64_t State;
   bool Failed;
 
   constexpr Board() : Failed(false) {}
   constexpr Board(const Board &O) : Failed(O.Failed) {}
   constexpr Board(uint64_t State, bool Failed = false) :
     Failed(Failed) {}
   constexpr Board addQueen(int Row, int Col) const {
-    return Board(State | ((uint64_t)Row << (Col * 4))); // ref-note {{read of uninitialized object}}
+    return Board(State | ((uint64_t)Row << (Col * 4))); // ref-note {{read of uninitialized object}} \
+                                                        // expected-note {{read of object outside its lifetime}}
   }
   constexpr int getQueenRow(int Col) const {
     return (State >> (Col * 4)) & 0xf;
   }
   constexpr bool ok(int Row, int Col) const {
     return okRecurse(Row, Col, 0);
   }
   constexpr bool okRecurse(int Row, int Col, int CheckCol) const {

 constexpr Board buildBoardScan(int N, int Col, int Row, const Board &B);
 constexpr Board tryBoard(const Board &Try,
                          int N, int Col, int Row, const Board &B) {
   return Try.Failed ? buildBoardScan(N, Col, Row, B) : Try;
 }
 constexpr Board buildBoardScan(int N, int Col, int Row, const Board &B) {
   return Row == N ? Board(0, true) :
          B.ok(Row, Col) ?
-         tryBoard(buildBoardRecurse(N, Col + 1, B.addQueen(Row, Col)), // ref-note {{in call to '&Board()->addQueen(0, 0)}}
+         tryBoard(buildBoardRecurse(N, Col + 1, B.addQueen(Row, Col)), // ref-note {{in call to '&Board()->addQueen(0, 0)}} \
+                                                                       // expected-note {{in call to '&Board()->addQueen(0, 0)}}
                   N, Col, Row+1, B) :
          buildBoardScan(N, Col, Row + 1, B);
 }
 constexpr Board buildBoardRecurse(int N, int Col, const Board &B) {
-  return Col == N ? B : buildBoardScan(N, Col, 0, B); // ref-note {{in call to 'buildBoardScan(8, 0, 0, Board())'}}
+  return Col == N ? B : buildBoardScan(N, Col, 0, B); // ref-note {{in call to 'buildBoardScan(8, 0, 0, Board())'}} \
+                                                      // expected-note {{in call to 'buildBoardScan(8, 0, 0, Board())'}}
+
 }
 constexpr Board buildBoard(int N) {
-  return buildBoardRecurse(N, 0, Board()); // ref-note {{in call to 'buildBoardRecurse(8, 0, Board())'}}
+  return buildBoardRecurse(N, 0, Board()); // ref-note {{in call to 'buildBoardRecurse(8, 0, Board())'}} \
+                                           // expected-note {{in call to 'buildBoardRecurse(8, 0, Board())'}}
 }
 
 constexpr Board q8 = buildBoard(8); // ref-error {{must be initialized by a constant expression}} \
                                     // ref-note {{in call to 'buildBoard(8)'}} \
-                                    // expected-error {{must be initialized by a constant expression}}
+                                    // expected-error {{must be initialized by a constant expression}} \
+                                    // expected-note {{in call to 'buildBoard(8)'}}

clang/test/AST/Interp/records.cpp

constexpr YetAnotherDerived Oops(0); // ref-error {{must be initialized by a constant expression}} \
                                        // expected-note {{constructor inherited from base class 'YetAnotherBase' cannot be used in a constant expression}}
 };
 
 namespace EmptyCtor {
   struct piecewise_construct_t { explicit piecewise_construct_t() = default; };
   constexpr piecewise_construct_t piecewise_construct =
     piecewise_construct_t();
 };
+
+namespace ConditionalInit {
+  struct S { int a; };
+
+  constexpr S getS(bool b) {
+    return b ? S{12} : S{13};
+  }
+
+  static_assert(getS(true).a == 12, "");
+  static_assert(getS(false).a == 13, "");
+};