[clang][Interp] Add 'Invalid' opcode and use it for throw/asm stmts

[tbaeder]

Summary

As discussed the other day. This fixes the diagnostics for the test case in records.cpp.

[erichkeane]

I've unfortunately not caught up enough to comment on the approach, but 'inline assembly' has the same restriction, so you might find MsAsmStmt and GCCAsmStmt to be equally as easy 'wins'.

[aaron.ballman]

"Unsupported" is a bit of a loaded term -- it could mean "this operation is not supported, YET" or it could mean "this operation is not and will not be supported, EVER". Perhaps something more like "InvalidInConstantExpr" would be more descriptive?

clang/test/AST/Interp/unsupported.cpp

4–6

Can get rid of some whitespace.

[tbaeder]

In D150364#4335221, @aaron.ballman wrote:

"Unsupported" is a bit of a loaded term -- it could mean "this operation is not supported, YET" or it could mean "this operation is not and will not be supported, EVER". Perhaps something more like "InvalidInConstantExpr" would be more descriptive?

I guess it would be more descriptive, but it could still mean that it is "not yet valid in a constant expression", so I guess I don't see the upside of using a longer opcode name.

[aaron.ballman]

In D150364#4335261, @tbaeder wrote:

In D150364#4335221, @aaron.ballman wrote:

"Unsupported" is a bit of a loaded term -- it could mean "this operation is not supported, YET" or it could mean "this operation is not and will not be supported, EVER". Perhaps something more like "InvalidInConstantExpr" would be more descriptive?

I guess it would be more descriptive, but it could still mean that it is "not yet valid in a constant expression", so I guess I don't see the upside of using a longer opcode name.

I don't feel strongly; it's easy enough to rename if we think it's causing confusion. FWIW, my first thought was "Oh, we're planning to support throw expressions in constant expressions? Please don't tell WG21." I'm used to seeing "invalid" for things that are never valid and "unsupported" for things that aren't supported but might be someday. However, I also see we use "unsupported" in the same sense you're using it here in some of our diagnostics, so I'm fine with whatever you want to go with.

clang/test/AST/Interp/records.cpp

338

Oh interesting -- does the old constexpr interpreter think the destructor is called at the end of the block as opposed to at the end of the full expression with the temporary?

[tbaeder]

In D150364#4335282, @aaron.ballman wrote:

In D150364#4335261, @tbaeder wrote:

In D150364#4335221, @aaron.ballman wrote:

"Unsupported" is a bit of a loaded term -- it could mean "this operation is not supported, YET" or it could mean "this operation is not and will not be supported, EVER". Perhaps something more like "InvalidInConstantExpr" would be more descriptive?

I guess it would be more descriptive, but it could still mean that it is "not yet valid in a constant expression", so I guess I don't see the upside of using a longer opcode name.

I don't feel strongly; it's easy enough to rename if we think it's causing confusion. FWIW, my first thought was "Oh, we're planning to support throw expressions in constant expressions? Please don't tell WG21." I'm used to seeing "invalid" for things that are never valid and "unsupported" for things that aren't supported but might be someday. However, I also see we use "unsupported" in the same sense you're using it here in some of our diagnostics, so I'm fine with whatever you want to go with.

I didn't know about that distinction, renaming to invalid is fine with me.

[tbaeder]

clang/test/AST/Interp/records.cpp

338

I think it's the other way around and the new one does that (expected is the new one). Need to investigate that.

[aaron.ballman]

LGTM

clang/test/AST/Interp/records.cpp

338

Ah oops, you're right, I did have that backwards. That makes more sense, thanks. :-)

clang/lib/AST/Interp/ByteCodeExprGen.h

public:
   bool VisitExprWithCleanups(const ExprWithCleanups *E);
   bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *E);
   bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *E);
   bool VisitCXXTemporaryObjectExpr(const CXXTemporaryObjectExpr *E);
   bool VisitCompoundLiteralExpr(const CompoundLiteralExpr *E);
   bool VisitTypeTraitExpr(const TypeTraitExpr *E);
   bool VisitLambdaExpr(const LambdaExpr *E);
   bool VisitPredefinedExpr(const PredefinedExpr *E);
+  bool VisitCXXThrowExpr(const CXXThrowExpr *E);
 
 protected:
   bool visitExpr(const Expr *E) override;
   bool visitDecl(const VarDecl *VD) override;
 
 protected:
   /// Emits scope cleanup instructions.
   void emitCleanup();

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitPredefinedExpr(const PredefinedExpr *E) {
   if (DiscardResult)
     return true;
 
   return this->visit(E->getFunctionName());
 }
 
+template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitCXXThrowExpr(const CXXThrowExpr *E) {
+  if (!this->discard(E->getSubExpr()))
+    return false;
+
+  return this->emitInvalid(E);
+}
+
 template <class Emitter> bool ByteCodeExprGen<Emitter>::discard(const Expr *E) {
   if (E->containsErrors())
     return false;
 
   OptionScope<Emitter> Scope(this, /*NewDiscardResult=*/true);
   return this->Visit(E);
 }
 

clang/lib/AST/Interp/ByteCodeStmtGen.h

private:
   bool visitDoStmt(const DoStmt *S);
   bool visitForStmt(const ForStmt *S);
   bool visitCXXForRangeStmt(const CXXForRangeStmt *S);
   bool visitBreakStmt(const BreakStmt *S);
   bool visitContinueStmt(const ContinueStmt *S);
   bool visitSwitchStmt(const SwitchStmt *S);
   bool visitCaseStmt(const CaseStmt *S);
   bool visitDefaultStmt(const DefaultStmt *S);
+  bool visitAsmStmt(const AsmStmt *S);
 
   bool emitLambdaStaticInvokerBody(const CXXMethodDecl *MD);
 
   /// Type of the expression returned by the function.
   std::optional<PrimType> ReturnType;
 
   /// Switch case mapping.
   CaseMap CaseLabels;

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

bool ByteCodeStmtGen<Emitter>::visitStmt(const Stmt *S) {
   case Stmt::ContinueStmtClass:
     return visitContinueStmt(cast<ContinueStmt>(S));
   case Stmt::SwitchStmtClass:
     return visitSwitchStmt(cast<SwitchStmt>(S));
   case Stmt::CaseStmtClass:
     return visitCaseStmt(cast<CaseStmt>(S));
   case Stmt::DefaultStmtClass:
     return visitDefaultStmt(cast<DefaultStmt>(S));
+  case Stmt::GCCAsmStmtClass:
+  case Stmt::MSAsmStmtClass:
+    return visitAsmStmt(cast<AsmStmt>(S));
   case Stmt::NullStmtClass:
     return true;
   default: {
     if (auto *Exp = dyn_cast<Expr>(S))
       return this->discard(Exp);
     return this->bail(S);
   }
   }

 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitDefaultStmt(const DefaultStmt *S) {
   this->emitLabel(*DefaultLabel);
   return this->visitStmt(S->getSubStmt());
 }
 
+template <class Emitter>
+bool ByteCodeStmtGen<Emitter>::visitAsmStmt(const AsmStmt *S) {
+  return this->emitInvalid(S);
+}
+
 namespace clang {
 namespace interp {
 
 template class ByteCodeStmtGen<ByteCodeEmitter>;
 
 } // namespace interp
 } // namespace clang

clang/lib/AST/Interp/Interp.h

 }
 
 inline bool GetFnPtr(InterpState &S, CodePtr OpPC, const Function *Func) {
   assert(Func);
   S.Stk.push<FunctionPointer>(Func);
   return true;
 }
 
+/// Just emit a diagnostic. The expression that caused emission of this
+/// op is not valid in a constant context.
+inline bool Invalid(InterpState &S, CodePtr OpPC) {
+  const SourceLocation &Loc = S.Current->getLocation(OpPC);
+  S.FFDiag(Loc, diag::note_invalid_subexpr_in_const_expr);
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 // Read opcode arguments
 //===----------------------------------------------------------------------===//
 
 template <typename T> inline T ReadArg(InterpState &S, CodePtr &OpPC) {
   if constexpr (std::is_pointer<T>::value) {
     uint32_t ID = OpPC.read<uint32_t>();
     return reinterpret_cast<T>(S.P.getNativePointer(ID));

clang/lib/AST/Interp/Opcodes.td

def Pop : Opcode {
   let HasGroup = 1;
 }
 
 // [Value] -> [Value, Value]
 def Dup : Opcode {
   let Types = [AllTypeClass];
   let HasGroup = 1;
 }
+
+// [] -> []
+def Invalid : Opcode {}

clang/test/AST/Interp/records.cpp

#if __cplusplus >= 202002L
   static_assert(T(0) == 1);
   static_assert(T(1) == 2);
   static_assert(T(2) == 3);
 
   // Invalid destructor.
   struct S {
     constexpr S() {}
     constexpr ~S() noexcept(false) { throw 12; } // expected-error {{cannot use 'throw'}} \
-                                                 // expected-note {{declared here}} \
+                                                 // expected-error {{never produces a constant expression}} \
+                                                 // expected-note 2{{subexpression not valid}} \
                                                  // ref-error {{cannot use 'throw'}} \
                                                  // ref-error {{never produces a constant expression}} \
                                                  // ref-note 2{{subexpression not valid}}
   };
 
   constexpr int f() {
     S{}; // ref-note {{in call to 'S{}.~S()'}}
-    return 12; // expected-note {{undefined function '~S'}}
+    /// FIXME: Wrong source location below.
+    return 12; // expected-note {{in call to '&S{}->~S()'}}
   }
   static_assert(f() == 12); // expected-error {{not an integral constant expression}} \
                             // expected-note {{in call to 'f()'}} \
                             // ref-error {{not an integral constant expression}} \
                             // ref-note {{in call to 'f()'}}
 
 
 #endif

clang/test/AST/Interp/unsupported.cpp

+// RUN: %clang_cc1 -fcxx-exceptions -std=c++20 -fexperimental-new-constant-interpreter -verify %s
+// RUN: %clang_cc1 -fcxx-exceptions -std=c++20 -verify=ref %s
+
+namespace Throw {
+
+  constexpr int ConditionalThrow(bool t) {
+    if (t)
+      throw 4; // expected-note {{subexpression not valid in a constant expression}} \
+               // ref-note {{subexpression not valid in a constant expression}}
+
+    return 0;
+  }
+
+  static_assert(ConditionalThrow(false) == 0, "");
+  static_assert(ConditionalThrow(true) == 0, ""); // expected-error {{not an integral constant expression}} \
+                                                  // expected-note {{in call to 'ConditionalThrow(true)'}} \
+                                                  // ref-error {{not an integral constant expression}} \
+                                                  // ref-note {{in call to 'ConditionalThrow(true)'}}
+
+  constexpr int Throw() { // expected-error {{never produces a constant expression}} \
+                          // ref-error {{never produces a constant expression}}
+    throw 5; // expected-note {{subexpression not valid in a constant expression}} \
+             // ref-note {{subexpression not valid in a constant expression}}
+    return 0;
+  }
+}
+
+namespace Asm {
+  constexpr int ConditionalAsm(bool t) {
+    if (t)
+      asm(""); // expected-note {{subexpression not valid in a constant expression}} \
+               // ref-note {{subexpression not valid in a constant expression}}
+
+    return 0;
+  }
+  static_assert(ConditionalAsm(false) == 0, "");
+  static_assert(ConditionalAsm(true) == 0, ""); // expected-error {{not an integral constant expression}} \
+                                                // expected-note {{in call to 'ConditionalAsm(true)'}} \
+                                                // ref-error {{not an integral constant expression}} \
+                                                // ref-note {{in call to 'ConditionalAsm(true)'}}
+
+
+  constexpr int Asm() { // expected-error {{never produces a constant expression}} \
+                        // ref-error {{never produces a constant expression}}
+    __asm volatile(""); // expected-note {{subexpression not valid in a constant expression}} \
+                        // ref-note {{subexpression not valid in a constant expression}}
+    return 0;
+  }
+}