[clang][Interp] Implement bool and nullptr literal expressions

[tbaeder]

Summary

This is split out from https://reviews.llvm.org/D131888

As a start, implement handling of boolean and nullptr expressions. The patch includes a bug fix to readArg() advance the code pointer and a few other small changes I did while looking at the code. I can pull those out as well but I don't want to open too many review tickets.

[erichkeane]

clang/lib/AST/Interp/Disasm.cpp

26

Are you sure this isn't intentional that this is passed by value? With a name like CodePtr, it certainly SOUNDS like it means to be passed by value.

clang/lib/AST/Interp/Function.h

69

is this an unrelated NFC change? In the future, please keep this out of patches that do stuff.

clang/lib/AST/Interp/Source.h

64

Ah, this DOES just wrap a pointer, passing by value is likely the right idea here.

clang/test/AST/Interp/literals.cpp

15

can you also do a 'getNullptr'? Also, a static-assert for all 3 of the functions as well.

[tbaeder]

clang/lib/AST/Interp/Disasm.cpp

26

CodePtr::read() advances the pointer that CodePtr wraps , so copying just to call read() on it makes little sense. E.g. in https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Interp/Interp.cpp#L401-L412, ReadArg must advance the CodePtr, otherwise two consecutive ReadArg calls will read the same thing.

clang/lib/AST/Interp/Function.h

69

Okay.

clang/test/AST/Interp/literals.cpp

15

I can do a getNull(), but I can't call any of the functions yet, function calls aren't implemented yet.

[erichkeane]

clang/lib/AST/Interp/Disasm.cpp

26

my point is; With a struct this small, the 'copy' is cheaper than a pass-by-ref. However, if the pass-by-value causes the ReadArg to read the same thing, why was this not a problem before? More importantly, what in this patch makes this change necessary?

clang/test/AST/Interp/literals.cpp

15

ah, thanks for that clarificaiton. When doing the function-calls, please see if you can add that here as well.

[tbaeder]

clang/lib/AST/Interp/Disasm.cpp

26

Nothing in this patch, if you e.g. take this code:

constexpr int func() { return 5; }

and run it with clang++ -std=c++20 -fexperimental-new-constant-interpreter, the constant interpreter runs into an assertion while evaluating the bytecode for that function. Switching ReadArg to take the codeptr by reference fixes that. So I could probably postpone the change to a later patch but it's one of the first problems I ran into while working on the codebase.

[erichkeane]

clang/lib/AST/Interp/Disasm.cpp

26

I see, thanks for the explanation. I think it is fine now, it just wasn't clear that this was covered by a test or necessary.

[aaron.ballman]

LGTM as well; should we add a release note for this? I suspect not because it's improving an experimental feature that's not really ready for prime time yet, but I don't know how others feel.

[erichkeane]

In D131942#3728974, @aaron.ballman wrote:

LGTM as well; should we add a release note for this? I suspect not because it's improving an experimental feature that's not really ready for prime time yet, but I don't know how others feel.

That was my thought on it, though thinking further, I wonder if there is value to it to show 'signs of life' in the release notes?

[aaron.ballman]

In D131942#3728994, @erichkeane wrote:

In D131942#3728974, @aaron.ballman wrote:

LGTM as well; should we add a release note for this? I suspect not because it's improving an experimental feature that's not really ready for prime time yet, but I don't know how others feel.

That was my thought on it, though thinking further, I wonder if there is value to it to show 'signs of life' in the release notes?

@tbaeder -- do you have plans to continue to add support to the experimental constant expression evaluator?

If this is code that we have people actively working on with plans to bring it to completion, then I think showing signs of life in the release notes is a good idea. If we don't have anyone who is planning to bring this through to completion, I think we should seriously considering pulling it out of the tree rather than trying to maintain parallel constant expression evaluators. (We wouldn't be pulling it out of the tree because we don't like the design, so if someone wanted to resurrect it in the future because they planned to complete it, we'd gladly welcome it.)

[tbaeder]

In D131942#3729057, @aaron.ballman wrote:

In D131942#3728994, @erichkeane wrote:

In D131942#3728974, @aaron.ballman wrote:

LGTM as well; should we add a release note for this? I suspect not because it's improving an experimental feature that's not really ready for prime time yet, but I don't know how others feel.

That was my thought on it, though thinking further, I wonder if there is value to it to show 'signs of life' in the release notes?

@tbaeder -- do you have plans to continue to add support to the experimental constant expression evaluator?

If this is code that we have people actively working on with plans to bring it to completion, then I think showing signs of life in the release notes is a good idea. If we don't have anyone who is planning to bring this through to completion, I think we should seriously considering pulling it out of the tree rather than trying to maintain parallel constant expression evaluators. (We wouldn't be pulling it out of the tree because we don't like the design, so if someone wanted to resurrect it in the future because they planned to complete it, we'd gladly welcome it.)

I certainly plan to keep working on it. I have a few more such patches locally that you'll have pleasure reviewing I'm sure :). It think adding a small changelog note is fine of course.

[aaron.ballman]

In D131942#3729379, @tbaeder wrote:

In D131942#3729057, @aaron.ballman wrote:

In D131942#3728994, @erichkeane wrote:

In D131942#3728974, @aaron.ballman wrote:

LGTM as well; should we add a release note for this? I suspect not because it's improving an experimental feature that's not really ready for prime time yet, but I don't know how others feel.

That was my thought on it, though thinking further, I wonder if there is value to it to show 'signs of life' in the release notes?

@tbaeder -- do you have plans to continue to add support to the experimental constant expression evaluator?

If this is code that we have people actively working on with plans to bring it to completion, then I think showing signs of life in the release notes is a good idea. If we don't have anyone who is planning to bring this through to completion, I think we should seriously considering pulling it out of the tree rather than trying to maintain parallel constant expression evaluators. (We wouldn't be pulling it out of the tree because we don't like the design, so if someone wanted to resurrect it in the future because they planned to complete it, we'd gladly welcome it.)

I certainly plan to keep working on it. I have a few more such patches locally that you'll have pleasure reviewing I'm sure :). It think adding a small changelog note is fine of course.

Fantastic! As long as it's heading in the right direction rather than getting more and more stale, I'm happy to keep it in-tree. I'm fine either way on the release note.

clang/lib/AST/Interp/ByteCodeExprGen.h

public:
   ByteCodeExprGen(Context &Ctx, Program &P, Tys &&... Args)
       : Emitter(Ctx, P, Args...), Ctx(Ctx), P(P) {}
 
   // Expression visitors - result returned on stack.
   bool VisitCastExpr(const CastExpr *E);
   bool VisitIntegerLiteral(const IntegerLiteral *E);
   bool VisitParenExpr(const ParenExpr *E);
   bool VisitBinaryOperator(const BinaryOperator *E);
+  bool VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E);
+  bool VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *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

bool ByteCodeExprGen<Emitter>::VisitCastExpr(const CastExpr *CE) {
 
   case CK_ArrayToPointerDecay:
   case CK_AtomicToNonAtomic:
   case CK_ConstructorConversion:
   case CK_FunctionToPointerDecay:
   case CK_NonAtomicToAtomic:
   case CK_NoOp:
   case CK_UserDefinedConversion:
+  case CK_NullToPointer:
     return this->Visit(SubExpr);
 
   case CK_ToVoid:
     return discard(SubExpr);
 
   default: {
     // TODO: implement other casts.
     return this->bail(CE);
if (Optional<PrimType> T = classify(VD->getType())) {
       return this->emitRetValue(VD);
     }
   }
 
   return this->bail(VD);
 }
 
 template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitCXXBoolLiteralExpr(
+    const CXXBoolLiteralExpr *E) {
+  if (DiscardResult)
+    return true;
+
+  return this->emitConstBool(E->getValue(), E);
+}
+
+template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitCXXNullPtrLiteralExpr(
+    const CXXNullPtrLiteralExpr *E) {
+  if (DiscardResult)
+    return true;
+
+  return this->emitNullPtr(E);
+}
+
+template <class Emitter>
 void ByteCodeExprGen<Emitter>::emitCleanup() {
   for (VariableScope<Emitter> *C = VarScope; C; C = C->getParent())
     C->emitDestruction();
 }
 
 namespace clang {
 namespace interp {
 
 template class ByteCodeExprGen<ByteCodeEmitter>;
 template class ByteCodeExprGen<EvalEmitter>;
 
 } // namespace interp
 } // namespace clang

clang/lib/AST/Interp/Disasm.cpp

 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Format.h"
 
 using namespace clang;
 using namespace clang::interp;
 
 template <typename T>
 inline std::enable_if_t<!std::is_pointer<T>::value, T> ReadArg(Program &P,
-                                                               CodePtr OpPC) {
+                                                               CodePtr &OpPC) {
   return OpPC.read<T>();
 }
 
 template <typename T>
 inline std::enable_if_t<std::is_pointer<T>::value, T> ReadArg(Program &P,
-                                                              CodePtr OpPC) {
+                                                              CodePtr &OpPC) {
   uint32_t ID = OpPC.read<uint32_t>();
   return reinterpret_cast<T>(P.getNativePointer(ID));
 }
 
 LLVM_DUMP_METHOD void Function::dump() const { dump(llvm::errs()); }
 
 LLVM_DUMP_METHOD void Function::dump(llvm::raw_ostream &OS) const {
   if (F) {

clang/lib/AST/Interp/Function.h

public:
   using ParamDescriptor = std::pair<PrimType, Descriptor *>;
 
   /// Returns the size of the function's local stack.
   unsigned getFrameSize() const { return FrameSize; }
   /// Returns the size of the argument stackx
   unsigned getArgSize() const { return ArgSize; }
 
   /// Returns a pointer to the start of the code.
-  CodePtr getCodeBegin() const;
+  CodePtr getCodeBegin() const { return Code.data(); }
   /// Returns a pointer to the end of the code.
-  CodePtr getCodeEnd() const;
+  CodePtr getCodeEnd() const { return Code.data() + Code.size(); }
 
   /// Returns the original FunctionDecl.
   const FunctionDecl *getDecl() const { return F; }
 
+  /// Returns the name of the function decl this code
+  /// was generated for.
+  const std::string getName() const { return F->getNameInfo().getAsString(); }
+
   /// Returns the location.
   SourceLocation getLoc() const { return Loc; }
 
   /// Returns a parameter descriptor.
   ParamDescriptor getParamDescriptor(unsigned Offset) const;
 
   /// Checks if the first argument is a RVO pointer.
   bool hasRVO() const { return ParamTypes.size() != Params.size(); }

clang/lib/AST/Interp/Function.cpp

 using namespace clang::interp;
 
 Function::Function(Program &P, const FunctionDecl *F, unsigned ArgSize,
                    llvm::SmallVector<PrimType, 8> &&ParamTypes,
                    llvm::DenseMap<unsigned, ParamDescriptor> &&Params)
     : P(P), Loc(F->getBeginLoc()), F(F), ArgSize(ArgSize),
       ParamTypes(std::move(ParamTypes)), Params(std::move(Params)) {}
 
-CodePtr Function::getCodeBegin() const { return Code.data(); }
-
-CodePtr Function::getCodeEnd() const { return Code.data() + Code.size(); }
-
 Function::ParamDescriptor Function::getParamDescriptor(unsigned Offset) const {
   auto It = Params.find(Offset);
   assert(It != Params.end() && "Invalid parameter offset");
   return It->second;
 }
 
 SourceInfo Function::getSource(CodePtr PC) const {
   unsigned Offset = PC - getCodeBegin();

clang/lib/AST/Interp/Interp.h

 }
 
 //===----------------------------------------------------------------------===//
 // Read opcode arguments
 //===----------------------------------------------------------------------===//
 
 template <typename T>
 inline std::enable_if_t<!std::is_pointer<T>::value, T> ReadArg(InterpState &S,
-                                                               CodePtr OpPC) {
+                                                               CodePtr &OpPC) {
   return OpPC.read<T>();
 }
 
 template <typename T>
 inline std::enable_if_t<std::is_pointer<T>::value, T> ReadArg(InterpState &S,
-                                                              CodePtr OpPC) {
+                                                              CodePtr &OpPC) {
   uint32_t ID = OpPC.read<uint32_t>();
   return reinterpret_cast<T>(S.P.getNativePointer(ID));
 }
 
 /// Interpreter entry point.
 bool Interpret(InterpState &S, APValue &Result);
 
 } // namespace interp
 } // namespace clang
 
 #endif

clang/lib/AST/Interp/Interp.cpp

bool CheckPure(InterpState &S, CodePtr OpPC, const CXXMethodDecl *MD) {
   if (!MD->isPure())
     return true;
   const SourceInfo &E = S.Current->getSource(OpPC);
   S.FFDiag(E, diag::note_constexpr_pure_virtual_call, 1) << MD;
   S.Note(MD->getLocation(), diag::note_declared_at);
   return false;
 }
 bool Interpret(InterpState &S, APValue &Result) {
+  assert(!S.Current->isRoot());
   CodePtr PC = S.Current->getPC();
 
+  // Empty program.
+  if (!PC)
+    return true;
+
   for (;;) {
     auto Op = PC.read<Opcode>();
     CodePtr OpPC = PC;
 
     switch (Op) {
 #define GET_INTERP
 #include "Opcodes.inc"
 #undef GET_INTERP
     }
   }
 }
 
 } // namespace interp
 } // namespace clang

clang/lib/AST/Interp/Source.h

 #include "clang/AST/Stmt.h"
 #include "llvm/Support/Endian.h"
 
 namespace clang {
 namespace interp {
 class Function;
 
 /// Pointer into the code segment.
-class CodePtr {
+class CodePtr final {
 public:
   CodePtr() : Ptr(nullptr) {}
 
   CodePtr &operator+=(int32_t Offset) {
     Ptr += Offset;
     return *this;
   }
 
   int32_t operator-(const CodePtr &RHS) const {
     assert(Ptr != nullptr && RHS.Ptr != nullptr && "Invalid code pointer");
     return Ptr - RHS.Ptr;
   }
 
   CodePtr operator-(size_t RHS) const {
     assert(Ptr != nullptr && "Invalid code pointer");
     return CodePtr(Ptr - RHS);
   }
 
   bool operator!=(const CodePtr &RHS) const { return Ptr != RHS.Ptr; }
 
+  operator bool() const { return Ptr; }
+
   /// Reads data and advances the pointer.
   template <typename T> std::enable_if_t<!std::is_pointer<T>::value, T> read() {
     using namespace llvm::support;
     T Value = endian::read<T, endianness::native, 1>(Ptr);
     Ptr += sizeof(T);
     return Value;
   }
 
 private:
   /// Constructor used by Function to generate pointers.
   CodePtr(const char *Ptr) : Ptr(Ptr) {}
 
 private:
   friend class Function;
 
   /// Pointer into the code owned by a function.
   const char *Ptr;
 };
 
 /// Describes the statement/declaration an opcode was generated from.
-class SourceInfo {
+class SourceInfo final {
 public:
   SourceInfo() {}
   SourceInfo(const Stmt *E) : Source(E) {}
   SourceInfo(const Decl *D) : Source(D) {}
 
   SourceLocation getLoc() const;
 
   const Stmt *asStmt() const { return Source.dyn_cast<const Stmt *>(); }

clang/test/AST/Interp/literals.cpp

+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++11 -verify %s
+
+static_assert(true, "");
+static_assert(false, ""); // expected-error{{failed}}
+static_assert(nullptr == nullptr, "");
+static_assert(1 == 1, "");
+static_assert(1 == 3, ""); // expected-error{{failed}}
+
+constexpr int number = 10;
+static_assert(number == 10, "");
+static_assert(number != 10, ""); // expected-error{{failed}}
+
+constexpr bool getTrue() { return true; }
+constexpr bool getFalse() { return false; }
+constexpr void* getNull() { return nullptr; }