[clang][Interp] Check instance pointers before calling functions on them

[tbaeder]

Summary

Remove the double Call() implementation to reduce code duplication.
Then fix Function::getSource() so we can diagnose instance pointers being null.

As discussed in https://reviews.llvm.org/D134699 - this fixes the test case added there.
The output in this case is not 100% the same as the current constant interpreter however.

[tbaeder]

clang/lib/AST/Interp/Interp.h

1119

I think this is okay to do for now, but in the long run we need to check *all* the parameters passed to function calls...

[tbaeder]

clang/lib/AST/Interp/Function.cpp

39

While I think the comment here is correct, the decrement itself certainly isn't.

[aaron.ballman]

clang/lib/AST/Interp/Function.cpp

39

Which comment and which decrement? (Or is this a stale review comment?)

clang/lib/AST/Interp/Interp.h

1111

Use a unique_ptr for this?

[aaron.ballman]

LGTM

clang/lib/AST/Interp/EvalEmitter.cpp

 
 template <PrimType OpType> bool EvalEmitter::emitRet(const SourceInfo &Info) {
   if (!isActive())
     return true;
   using T = typename PrimConv<OpType>::T;
   return ReturnValue<T>(S.Stk.pop<T>(), Result);
 }
 
-template <PrimType OpType>
-bool EvalEmitter::emitCall(const Function *Func, const SourceInfo &Info) {
-
-  S.Current = new InterpFrame(S, Func, {});
-  // Result of call will be on the stack and needs to be handled by the caller.
-  return Interpret(S, Result);
-}
-
 bool EvalEmitter::emitCallVoid(const Function *Func, const SourceInfo &Info) {
   APValue VoidResult;
   InterpFrame *before = S.Current;
   (void)before;
   S.Current = new InterpFrame(S, Func, {});
   bool Success = Interpret(S, VoidResult);
   assert(VoidResult.isAbsent());
   assert(S.Current == before);

clang/lib/AST/Interp/Function.cpp

 
 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 {
+  assert(PC >= getCodeBegin() && "PC does not belong to this function");
+  assert(PC <= getCodeEnd() && "PC Does not belong to this function");
   unsigned Offset = PC - getCodeBegin();
   using Elem = std::pair<unsigned, SourceInfo>;
   auto It = llvm::lower_bound(SrcMap, Elem{Offset, {}}, llvm::less_first());
-  if (It == SrcMap.end() || It->first != Offset)
-    llvm::report_fatal_error("missing source location");
+  assert(It != SrcMap.end());
   return It->second;
 }
 
 bool Function::isVirtual() const {
   if (auto *M = dyn_cast<CXXMethodDecl>(F))
     return M->isVirtual();
   return false;
 }

clang/lib/AST/Interp/Interp.h

if (LHS.isSigned() && LHS.isMin() && RHS.isNegative() && RHS.isMinusOne()) {
     S.CCEDiag(Loc, diag::note_constexpr_overflow) << Trunc << E->getType();
     return false;
   }
   return true;
 }
 
 template <typename T> inline bool IsTrue(const T &V) { return !V.isZero(); }
 
+/// Interpreter entry point.
+bool Interpret(InterpState &S, APValue &Result);
+
 //===----------------------------------------------------------------------===//
 // Add, Sub, Mul
 //===----------------------------------------------------------------------===//
 
 template <typename T, bool (*OpFW)(T, T, unsigned, T *),
           template <typename U> class OpAP>
 bool AddSubMulHelper(InterpState &S, CodePtr OpPC, unsigned Bits, const T &LHS,
                      const T &RHS) {

 }
 
 inline bool ExpandPtr(InterpState &S, CodePtr OpPC) {
   const Pointer &Ptr = S.Stk.pop<Pointer>();
   S.Stk.push<Pointer>(Ptr.expand());
   return true;
 }
 
+template <PrimType Name, class T = typename PrimConv<Name>::T>
+static bool Call(InterpState &S, CodePtr &PC, const Function *Func) {
+  auto NewFrame = std::make_unique<InterpFrame>(S, Func, PC);
+  if (Func->hasThisPointer()) {
+    if (!CheckInvoke(S, PC, NewFrame->getThis())) {
+      return false;
+    }
+    // TODO: CheckCallable
+  }
+
+  InterpFrame *FrameBefore = S.Current;
+  S.Current = NewFrame.get();
+
+  APValue CallResult;
+  // Note that we cannot assert(CallResult.hasValue()) here since
+  // Ret() above only sets the APValue if the curent frame doesn't
+  // have a caller set.
+  if (Interpret(S, CallResult)) {
+    NewFrame.release(); // Frame was delete'd already.
+    assert(S.Current == FrameBefore);
+    return true;
+  }
+
+  // Interpreting the function failed somehow. Reset to
+  // previous state.
+  S.Current = FrameBefore;
+  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));
   } else {
     return OpPC.read<T>();
   }
 }
 
-/// Interpreter entry point.
-bool Interpret(InterpState &S, APValue &Result);
-
 } // namespace interp
 } // namespace clang
 
 #endif

clang/lib/AST/Interp/Interp.cpp

if (InterpFrame *Caller = S.Current->Caller) {
     delete S.Current;
     S.Current = nullptr;
     if (!ReturnValue<T>(Ret, Result))
       return false;
   }
   return true;
 }
 
-template <PrimType Name, class T = typename PrimConv<Name>::T>
-static bool Call(InterpState &S, CodePtr &PC, const Function *Func) {
-  S.Current = new InterpFrame(S, const_cast<Function *>(Func), PC);
-  APValue CallResult;
-  // Note that we cannot assert(CallResult.hasValue()) here since
-  // Ret() above only sets the APValue if the curent frame doesn't
-  // have a caller set.
-  return Interpret(S, CallResult);
-}
-
 static bool CallVoid(InterpState &S, CodePtr &PC, const Function *Func) {
   APValue VoidResult;
   S.Current = new InterpFrame(S, const_cast<Function *>(Func), PC);
   bool Success = Interpret(S, VoidResult);
   assert(VoidResult.isAbsent());
 
   return Success;
 }

clang/lib/AST/Interp/Opcodes.td

 // [] -> EXIT
 def NoRet : Opcode {}
 
 
 def Call : Opcode {
   let Args = [ArgFunction];
   let Types = [AllTypeClass];
   let ChangesPC = 1;
-  let HasCustomEval = 1;
   let HasGroup = 1;
 }
 
 def CallVoid : Opcode {
   let Args = [ArgFunction];
   let Types = [];
   let ChangesPC = 1;
   let HasCustomEval = 1;

clang/test/AST/Interp/records.cpp

 
 namespace thisPointer {
   struct S {
     constexpr int get12() { return 12; }
   };
 
   constexpr int foo() { // ref-error {{never produces a constant expression}}
     S *s = nullptr;
-    return s->get12(); // ref-note 2{{member call on dereferenced null pointer}}
+    return s->get12(); // ref-note 2{{member call on dereferenced null pointer}} \
+                       // expected-note {{member call on dereferenced null pointer}}
+
   }
-  // FIXME: The new interpreter doesn't reject this currently.
   static_assert(foo() == 12, ""); // ref-error {{not an integral constant expression}} \
-                                  // ref-note {{in call to 'foo()'}}
+                                  // ref-note {{in call to 'foo()'}} \
+                                  // expected-error {{not an integral constant expression}} \
+                                  // expected-note {{in call to 'foo()'}}
 };
 
 struct FourBoolPairs {
   BoolPair v[4] = {
     {false, false},
     {false,  true},
     {true,  false},
     {true,  true },