[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

649

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

219

Can you explain what this is doing for me?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

219

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

41

What are these functions testing?

[tbaeder]

clang/test/AST/Interp/literals.cpp

41

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

619–620

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.

673

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

43

Perhaps add:

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

47

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

45

Should be == 10 for the second case.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

673

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

673

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

673

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

bool ByteCodeExprGen<Emitter>::VisitCastExpr(const CastExpr *CE) {
 
   case CK_LValueToRValue: {
     return dereference(
         CE->getSubExpr(), DerefKind::Read,
         [](PrimType) {
           // Value loaded - nothing to do here.
           return true;
         },
-        [this, CE](PrimType T) {
+        [this, CE, SubExpr](PrimType T) {
+          // DeclRefExpr are modeled as pointers, so nothing to do.
+          if (isa<DeclRefExpr>(SubExpr->IgnoreImpCasts())) {
             // Pointer on stack - dereference it.
             if (!this->emitLoadPop(T, CE))
               return false;
             return DiscardResult ? this->emitPop(T, CE) : true;
+          }
+          return true;
         });
   }
 
   case CK_ArrayToPointerDecay:
   case CK_AtomicToNonAtomic:
   case CK_ConstructorConversion:
   case CK_FunctionToPointerDecay:
   case CK_NonAtomicToAtomic:
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;
+      // TODO: Assignments return the assigned value, so the pop() here
+      //   should proably just go away.
+      return this->emitPopPtr(BO);
     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("Referencing requires a pointer");
+          return false;
+        },
+        [this, E](PrimType T) {
+          // T: type we get here from classify() is the subexpr
+          // TODO: Is this right?
+          T = *classify(E->getType());
+          if (!this->emitLoadPop(T, E))
+            return false;
+          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:
     llvm_unreachable("Unhandled opcode");
   }
 
   return false;
 }
 
 template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitDeclRefExpr(const DeclRefExpr *E) {
+  if (auto It = Locals.find(E->getDecl()); It != Locals.end()) {
+    const unsigned Offset = It->second.Offset;
+    return this->emitGetPtrLocal(Offset, E);
+  } else if (auto GlobalIndex = P.getGlobal(E->getDecl())) {
+    return this->emitGetPtrGlobal(*GlobalIndex, E);
+  } else if (isa<ParmVarDecl>(E->getDecl())) {
+    // I'm pretty sure we should do something here, BUT
+    // when we're in evaluateAsRValue(), we don't have any parameters,
+    // so we can't actually use this->Params. This happens when
+    // a parameter is used in a return statement.
+    return false;
+  }
+
+  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 %s -DREFERENCE
+
+
+// expected-no-diagnostics
+constexpr int getMinus5() {
+  int a = 10;
+  a = -5;
+  int *p = &a;
+  return *p;
+}
+

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}}
+
+constexpr int m = 10;
+constexpr const int *p = &m;
+static_assert(p != nullptr, "");
+static_assert(*p == 10, "");
+
+constexpr const int* getIntPointer() {
+  return &m;
+}