[clang][Interp] Implement pointer (de)ref operations and DeclRefExprs

[tbaeder]

Summary

I wanted to do only pointers here, but they are impossible to test without having some support for DeclRefExprs.

This also implements assignments because that was broken when implementing DeclRefExprs. Assignments were handled through LValueToRValue casts before.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

655

Handling the deref like this breaks assigning, e.g. int m = 10; int *p = &m; *p = 12; doesn't work. The LHS of the assignment ends up being *p, so just an int.

[erichkeane]

Just the 1 question for my info, Tests seem pretty complete for what is being looked at, but I'm hopeful someone else will take a look at this too. @shafik or @tahonermann want to take a pass so aaron doesn't have to?

clang/lib/AST/Interp/ByteCodeExprGen.cpp

227

Can you explain what this is doing for me?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

227

The Store operation pops the value to store from the stack, but leaves the pointer it stores the value to on the stack (it only peek()s the pointer). So after the emitStore, there's still the pointer we stored to on the stack, and we need to get rid of it if DiscardResult is true (that's the case for a simple i = 5; on a single line). Otherwise, we leave it no the stack for the caller to read from.

[shafik]

This mostly makes sense to me but I will let @tahonermann review it as well.

clang/test/AST/Interp/literals.cpp

40

What are these functions testing?

[tbaeder]

clang/test/AST/Interp/literals.cpp

40

Functions take a slightly different code path where actual bytecode is generated instead of the expression directly evaluated, so the functions test that this codepath works as well.

[tahonermann]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

625–626

I don't love that the Visit() calls are now repeated for most of the operations, but I don't have a better suggestion. I see that Visit() must not be called for `UO_Deref since dereference() will perform the visitation. I guess that case could be pulled out of the switch, but that isn't necessarily better. Consider this an unactionable comment.

674

Perhaps add:

else {
  assert(0 && "Unhandled declaration kind");
}

clang/test/AST/Interp/cxx20.cpp

3

Is -DREFERENCE needed here? I don't see REFERENCE referenced in the test. From the other test, I gather that it is intended to annotate differences from the "reference" implementation.

12

Perhaps add:

//static_assert(getMinux5() == -5, "");  TODO

clang/test/AST/Interp/literals.cpp

42

Perhaps add:

//static_assert(*getIntPointer() == 10, ""); TODO

46

Perhaps add:

//static_assert(gimme(5) == 5, ""); TODO

[tbaeder]

clang/test/AST/Interp/cxx20.cpp

3

Yes, exactly. I'll switch this one to use -verify=ref as well.

[tahonermann]

clang/test/AST/Interp/cxx20.cpp

3

Cool! I didn't know about -verify=ref!

clang/test/AST/Interp/literals.cpp

44

Should be == 10 for the second case.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

674

We actually hit this path for non-constexpr-conforming functions, so asserting doesn't work:

constexpr void foo(int a) {
  constexpr int b = a;
}

The initializer for b goes through evaluteAsInitializer() before the function foo is ever registered, so the parameter is not known. This is diagnosed by the current interpreter as well:

array.cpp:13:17: error: constexpr variable 'b' must be initialized by a constant expression
  constexpr int b = a;
                ^   ~
array.cpp:13:21: note: function parameter 'a' with unknown value cannot be used in a constant expression
  constexpr int b = a;
                    ^
array.cpp:12:24: note: declared here
constexpr void foo(int a) {
                       ^

Would be a good future test case, but right now the error message for the new interpreter is just "constexpr variable 'b' must be initialized by a constant expression".

[tahonermann]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

674

I see, interesting.

I imagine future work will be needed to support references to lambda/block captures, data members, and such? And the answer to those right now is, we're not there yet?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

674

Yes, almost nothing works right now. :)

[tahonermann]

Thanks, Timm. The changes look reasonable to me, so I'll accept. Given my unfamiliarity with this code though, it would be great if someone else also gave it a nod.

[erichkeane]

I'm happy as well.

clang/lib/AST/Interp/ByteCodeExprGen.h

public:
   // 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);
   bool VisitUnaryOperator(const UnaryOperator *E);
+  bool VisitDeclRefExpr(const DeclRefExpr *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

if (Optional<PrimType> T = classify(BO->getType())) {
     case BO_GE:
       return Discard(this->emitGE(*LT, BO));
     case BO_Sub:
       return Discard(this->emitSub(*T, BO));
     case BO_Add:
       return Discard(this->emitAdd(*T, BO));
     case BO_Mul:
       return Discard(this->emitMul(*T, BO));
+    case BO_Assign:
+      if (!this->emitStore(*T, BO))
+        return false;
+      return DiscardResult ? this->emitPopPtr(BO) : true;
     default:
       return this->bail(BO);
     }
   }
 
   return this->bail(BO);
 }
 
bool ByteCodeExprGen<Emitter>::VisitCXXNullPtrLiteralExpr(
   if (DiscardResult)
     return true;
 
   return this->emitNullPtr(E);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitUnaryOperator(const UnaryOperator *E) {
-  if (!this->Visit(E->getSubExpr()))
-    return false;
+  const Expr *SubExpr = E->getSubExpr();
 
   switch (E->getOpcode()) {
   case UO_PostInc: // x++
   case UO_PostDec: // x--
   case UO_PreInc:  // --x
   case UO_PreDec:  // ++x
     return false;
 
   case UO_LNot: // !x
+    if (!this->Visit(SubExpr))
+      return false;
     return this->emitInvBool(E);
   case UO_Minus: // -x
+    if (!this->Visit(SubExpr))
+      return false;
     if (Optional<PrimType> T = classify(E->getType()))
       return this->emitNeg(*T, E);
     return false;
   case UO_Plus:  // +x
-    return true; // noop
+    return this->Visit(SubExpr); // noop
 
   case UO_AddrOf: // &x
+    // We should already have a pointer when we get here.
+    return this->Visit(SubExpr);
+
   case UO_Deref:  // *x
+    return dereference(
+        SubExpr, DerefKind::Read,
+        [](PrimType) {
+          llvm_unreachable("Dereferencing requires a pointer");
+          return false;
+        },
+        [this, E](PrimType T) {
+          return DiscardResult ? this->emitPop(T, E) : true;
+        });
   case UO_Not:    // ~x
   case UO_Real:   // __real x
   case UO_Imag:   // __imag x
   case UO_Extension:
   case UO_Coawait:
     assert(false && "Unhandled opcode");
   }
 
   return false;
 }
 
 template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitDeclRefExpr(const DeclRefExpr *E) {
+  const auto *Decl = E->getDecl();
+
+  if (auto It = Locals.find(Decl); It != Locals.end()) {
+    const unsigned Offset = It->second.Offset;
+    return this->emitGetPtrLocal(Offset, E);
+  } else if (auto GlobalIndex = P.getGlobal(Decl)) {
+    return this->emitGetPtrGlobal(*GlobalIndex, E);
+  } else if (const auto *PVD = dyn_cast<ParmVarDecl>(Decl)) {
+    if (auto It = this->Params.find(PVD); It != this->Params.end())
+      return this->emitGetPtrParam(It->second, E);
+  }
+
+  return false;
+}
+
+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/test/AST/Interp/cxx20.cpp

+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++20 -verify %s
+// RUN: %clang_cc1 -std=c++20 -verify=ref %s
+
+
+// expected-no-diagnostics
+constexpr int getMinus5() {
+  int a = 10;
+  a = -5;
+  int *p = &a;
+  return *p;
+}
+//static_assert(getMinus5() == -5, "") TODO
+
+constexpr int assign() {
+  int m = 10;
+  int k = 12;
+
+  m = (k = 20);
+
+  return m;
+}
+//static_assert(assign() == 20, "");  TODO
+
+
+constexpr int pointerAssign() {
+  int m = 10;
+  int *p = &m;
+
+  *p = 12; // modifies m
+
+  return m;
+}
+//static_assert(pointerAssign() == 12, "");  TODO
+
+constexpr int pointerDeref() {
+  int m = 12;
+  int *p = &m;
+
+  return *p;
+}
+//static_assert(pointerDeref() == 12, ""); TODO
+
+constexpr int pointerAssign2() {
+  int m = 10;
+  int *p = &m;
+  int **pp = &p;
+
+  **pp = 12;
+
+  int v = **pp;
+
+  return v;
+}
+//static_assert(pointerAssign2() == 12, ""); TODO

clang/test/AST/Interp/literals.cpp

 static_assert(!!true, "");
 static_assert(!!true == !false, "");
 static_assert(true == 1, "");
 static_assert(false == 0, "");
 static_assert(!5 == false, "");
 static_assert(!0, "");
 static_assert(-true, "");
 static_assert(-false, ""); //expected-error{{failed}} ref-error{{failed}}
+
+constexpr int m = 10;
+constexpr const int *p = &m;
+static_assert(p != nullptr, "");
+static_assert(*p == 10, "");
+
+constexpr const int* getIntPointer() {
+  return &m;
+}
+//static_assert(getIntPointer() == &m, ""); TODO
+//static_assert(*getIntPointer() == 10, ""); TODO
+
+constexpr int gimme(int k) {
+  return k;
+}
+// static_assert(gimme(5) == 5, ""); TODO