[clang][bytecode] Classify function pointers as PT_Ptr (llvm#135026)

The Pointer class already has the capability to be a function pointer,
but we still classifed function pointers as PT_FnPtr/FunctionPointer.
This means when converting from a Pointer to a FunctionPointer, we lost
the information of what the original Pointer pointed to.

Author: tbaederr

clang/lib/AST/ByteCode/Compiler.cpp

@@ -463,6 +463,8 @@ bool Compiler<Emitter>::VisitCastExpr(const CastExpr *CE) {
 
       if (!this->visit(SubExpr))
         return false;
+      if (CE->getType()->isFunctionPointerType())
+        return true;
       if (FromT == PT_Ptr)
         return this->emitPtrPtrCast(SubExprTy->isVoidPointerType(), CE);
       return true;
@@ -4921,10 +4923,10 @@ bool Compiler<Emitter>::VisitCallExpr(const CallExpr *E) {
   } else if (!FuncDecl) {
     const Expr *Callee = E->getCallee();
     CalleeOffset =
-        this->allocateLocalPrimitive(Callee, PT_FnPtr, /*IsConst=*/true);
+        this->allocateLocalPrimitive(Callee, PT_Ptr, /*IsConst=*/true);
     if (!this->visit(Callee))
       return false;
-    if (!this->emitSetLocal(PT_FnPtr, *CalleeOffset, E))
+    if (!this->emitSetLocal(PT_Ptr, *CalleeOffset, E))
       return false;
   }
 
@@ -5011,7 +5013,7 @@ bool Compiler<Emitter>::VisitCallExpr(const CallExpr *E) {
       if (!this->emitGetMemberPtrDecl(E))
         return false;
     } else {
-      if (!this->emitGetLocal(PT_FnPtr, *CalleeOffset, E))
+      if (!this->emitGetLocal(PT_Ptr, *CalleeOffset, E))
         return false;
     }
     if (!this->emitCallPtr(ArgSize, E, E))

clang/lib/AST/ByteCode/Context.cpp

@@ -189,7 +189,7 @@ std::optional<PrimType> Context::classify(QualType T) const {
 
   if (T->isFunctionPointerType() || T->isFunctionReferenceType() ||
       T->isFunctionType() || T->isBlockPointerType())
-    return PT_FnPtr;
+    return PT_Ptr;
 
   if (T->isPointerOrReferenceType() || T->isObjCObjectPointerType())
     return PT_Ptr;

clang/lib/AST/ByteCode/Context.h

@@ -78,11 +78,8 @@ class Context final {
   /// Classifies an expression.
   std::optional<PrimType> classify(const Expr *E) const {
     assert(E);
-    if (E->isGLValue()) {
-      if (E->getType()->isFunctionType())
-        return PT_FnPtr;
+    if (E->isGLValue())
       return PT_Ptr;
-    }
 
     return classify(E->getType());
   }

clang/lib/AST/ByteCode/EvalEmitter.cpp

@@ -169,6 +169,11 @@ template <> bool EvalEmitter::emitRet<PT_Ptr>(const SourceInfo &Info) {
 
   const Pointer &Ptr = S.Stk.pop<Pointer>();
 
+  if (Ptr.isFunctionPointer()) {
+    EvalResult.setValue(Ptr.toAPValue(Ctx.getASTContext()));
+    return true;
+  }
+
   if (!EvalResult.checkReturnValue(S, Ctx, Ptr, Info))
     return false;
   if (CheckFullyInitialized && !EvalResult.checkFullyInitialized(S, Ptr))

clang/lib/AST/ByteCode/Interp.cpp

@@ -90,9 +90,6 @@ static bool BCP(InterpState &S, CodePtr &RealPC, int32_t Offset, PrimType PT) {
       assert(S.Stk.size() == StackSizeBefore);
       S.Stk.push<Integral<32, true>>(
           Integral<32, true>::from(CheckBCPResult(S, Ptr)));
-    } else if (PT == PT_FnPtr) {
-      S.Stk.discard<FunctionPointer>();
-      S.Stk.push<Integral<32, true>>(Integral<32, true>::from(0));
     } else {
       // Pop the result from the stack and return success.
       TYPE_SWITCH(PT, S.Stk.pop<T>(););
@@ -318,6 +315,8 @@ bool CheckBCPResult(InterpState &S, const Pointer &Ptr) {
     return false;
   if (Ptr.isZero())
     return true;
+  if (Ptr.isFunctionPointer())
+    return false;
   if (Ptr.isIntegralPointer())
     return true;
   if (Ptr.isTypeidPointer())
@@ -1621,20 +1620,24 @@ bool CallBI(InterpState &S, CodePtr OpPC, const Function *Func,
 
 bool CallPtr(InterpState &S, CodePtr OpPC, uint32_t ArgSize,
              const CallExpr *CE) {
-  const FunctionPointer &FuncPtr = S.Stk.pop<FunctionPointer>();
+  const Pointer &Ptr = S.Stk.pop<Pointer>();
 
-  const Function *F = FuncPtr.getFunction();
-  if (!F) {
+  if (Ptr.isZero()) {
     const auto *E = cast<CallExpr>(S.Current->getExpr(OpPC));
     S.FFDiag(E, diag::note_constexpr_null_callee)
         << const_cast<Expr *>(E->getCallee()) << E->getSourceRange();
     return false;
   }
 
-  if (!FuncPtr.isValid() || !F->getDecl())
+  if (!Ptr.isFunctionPointer())
     return Invalid(S, OpPC);
 
+  const FunctionPointer &FuncPtr = Ptr.asFunctionPointer();
+  const Function *F = FuncPtr.getFunction();
   assert(F);
+  // Don't allow calling block pointers.
+  if (!F->getDecl())
+    return Invalid(S, OpPC);
 
   // This happens when the call expression has been cast to
   // something else, but we don't support that.
@@ -1774,6 +1777,8 @@ bool CheckPointerToIntegralCast(InterpState &S, CodePtr OpPC,
                                 const Pointer &Ptr, unsigned BitWidth) {
   if (Ptr.isDummy())
     return false;
+  if (Ptr.isFunctionPointer())
+    return true;
 
   const SourceInfo &E = S.Current->getSource(OpPC);
   S.CCEDiag(E, diag::note_constexpr_invalid_cast)

clang/lib/AST/ByteCode/Interp.h

@@ -979,20 +979,6 @@ bool CmpHelperEQ(InterpState &S, CodePtr OpPC, CompareFn Fn) {
   return CmpHelper<T>(S, OpPC, Fn);
 }
 
-/// Function pointers cannot be compared in an ordered way.
-template <>
-inline bool CmpHelper<FunctionPointer>(InterpState &S, CodePtr OpPC,
-                                       CompareFn Fn) {
-  const auto &RHS = S.Stk.pop<FunctionPointer>();
-  const auto &LHS = S.Stk.pop<FunctionPointer>();
-
-  const SourceInfo &Loc = S.Current->getSource(OpPC);
-  S.FFDiag(Loc, diag::note_constexpr_pointer_comparison_unspecified)
-      << LHS.toDiagnosticString(S.getASTContext())
-      << RHS.toDiagnosticString(S.getASTContext());
-  return false;
-}
-
 template <>
 inline bool CmpHelperEQ<FunctionPointer>(InterpState &S, CodePtr OpPC,
                                          CompareFn Fn) {
@@ -1019,18 +1005,27 @@ inline bool CmpHelper<Pointer>(InterpState &S, CodePtr OpPC, CompareFn Fn) {
   const Pointer &RHS = S.Stk.pop<Pointer>();
   const Pointer &LHS = S.Stk.pop<Pointer>();
 
+  // Function pointers cannot be compared in an ordered way.
+  if (LHS.isFunctionPointer() || RHS.isFunctionPointer()) {
+    const SourceInfo &Loc = S.Current->getSource(OpPC);
+    S.FFDiag(Loc, diag::note_constexpr_pointer_comparison_unspecified)
+        << LHS.toDiagnosticString(S.getASTContext())
+        << RHS.toDiagnosticString(S.getASTContext());
+    return false;
+  }
+
   if (!Pointer::hasSameBase(LHS, RHS)) {
     const SourceInfo &Loc = S.Current->getSource(OpPC);
     S.FFDiag(Loc, diag::note_constexpr_pointer_comparison_unspecified)
         << LHS.toDiagnosticString(S.getASTContext())
         << RHS.toDiagnosticString(S.getASTContext());
     return false;
-  } else {
-    unsigned VL = LHS.getByteOffset();
-    unsigned VR = RHS.getByteOffset();
-    S.Stk.push<BoolT>(BoolT::from(Fn(Compare(VL, VR))));
-    return true;
   }
+
+  unsigned VL = LHS.getByteOffset();
+  unsigned VR = RHS.getByteOffset();
+  S.Stk.push<BoolT>(BoolT::from(Fn(Compare(VL, VR))));
+  return true;
 }
 
 static inline bool IsOpaqueConstantCall(const CallExpr *E) {
@@ -1069,6 +1064,12 @@ inline bool CmpHelperEQ<Pointer>(InterpState &S, CodePtr OpPC, CompareFn Fn) {
       return false;
   }
 
+  if (LHS.isFunctionPointer() && RHS.isFunctionPointer()) {
+    S.Stk.push<BoolT>(BoolT::from(Fn(Compare(LHS.getIntegerRepresentation(),
+                                             RHS.getIntegerRepresentation()))));
+    return true;
+  }
+
   if (Pointer::hasSameBase(LHS, RHS)) {
     if (LHS.inUnion() && RHS.inUnion()) {
       // If the pointers point into a union, things are a little more
@@ -2787,7 +2788,7 @@ inline bool ArrayDecay(InterpState &S, CodePtr OpPC) {
 
 inline bool GetFnPtr(InterpState &S, CodePtr OpPC, const Function *Func) {
   assert(Func);
-  S.Stk.push<FunctionPointer>(Func);
+  S.Stk.push<Pointer>(Func);
   return true;
 }
 
@@ -2822,7 +2823,7 @@ inline bool GetMemberPtrDecl(InterpState &S, CodePtr OpPC) {
   const auto *FD = cast<FunctionDecl>(MP.getDecl());
   const auto *Func = S.getContext().getOrCreateFunction(FD);
 
-  S.Stk.push<FunctionPointer>(Func);
+  S.Stk.push<Pointer>(Func);
   return true;
 }
 

clang/lib/AST/ByteCode/Pointer.cpp

@@ -152,8 +152,15 @@ APValue Pointer::toAPValue(const ASTContext &ASTCtx) const {
                    CharUnits::fromQuantity(asIntPointer().Value + this->Offset),
                    Path,
                    /*IsOnePastEnd=*/false, /*IsNullPtr=*/false);
-  if (isFunctionPointer())
-    return asFunctionPointer().toAPValue(ASTCtx);
+  if (isFunctionPointer()) {
+    const FunctionPointer &FP = asFunctionPointer();
+    if (const FunctionDecl *FD = FP.getFunction()->getDecl())
+      return APValue(FD, CharUnits::fromQuantity(FP.getOffset() + Offset), {},
+                     /*OnePastTheEnd=*/false, /*IsNull=*/false);
+    return APValue(FP.getFunction()->getExpr(),
+                   CharUnits::fromQuantity(FP.getOffset() + Offset), {},
+                   /*OnePastTheEnd=*/false, /*IsNull=*/false);
+  }
 
   if (isTypeidPointer()) {
     TypeInfoLValue TypeInfo(PointeeStorage.Typeid.TypePtr);
@@ -379,6 +386,9 @@ std::string Pointer::toDiagnosticString(const ASTContext &Ctx) const {
   if (isIntegralPointer())
     return (Twine("&(") + Twine(asIntPointer().Value + Offset) + ")").str();
 
+  if (isFunctionPointer())
+    return asFunctionPointer().toDiagnosticString(Ctx);
+
   return toAPValue(Ctx).getAsString(Ctx, getType());
 }
 

clang/lib/AST/ByteCode/Pointer.h

@@ -525,6 +525,8 @@ class Pointer {
   }
 
   bool isWeak() const {
+    if (isFunctionPointer())
+      return asFunctionPointer().isWeak();
     if (!isBlockPointer())
       return false;
 

clang/test/AST/ByteCode/pointer-to-fnptr.cpp

@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s                                         | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s -fexperimental-new-constant-interpreter | FileCheck %s
+
+template<typename T>
+struct Wrapper {
+  T *Val;
+
+  template<typename _Up>
+  constexpr Wrapper(_Up&& __u) {
+    T& __f = static_cast<_Up&&>(__u);
+    Val = &__f;
+  }
+  constexpr T& get() const { return *Val; }
+};
+
+void f(){}
+int main() {
+  auto W = Wrapper<decltype(f)>(f);
+
+  if (&W.get() != &f)
+    __builtin_abort();
+}
+
+/// We used to convert do the pointer->fnptr conversion
+/// by doing an integer conversion in between, which caused the
+/// %0 line to be:
+/// store ptr inttoptr (i64 138574454870464 to ptr), ptr %__f, align 8
+// CHECK: @_ZN7WrapperIFvvEEC2IRS0_EEOT_
+// CHECK: %0 = load ptr, ptr %__u.addr

clang/unittests/AST/ByteCode/toAPValue.cpp

@@ -135,7 +135,7 @@ TEST(ToAPValue, FunctionPointers) {
 
   {
     const Pointer &GP = getGlobalPtr("func");
-    const FunctionPointer &FP = GP.deref<FunctionPointer>();
+    const Pointer &FP = GP.deref<Pointer>();
     ASSERT_FALSE(FP.isZero());
     APValue A = FP.toAPValue(ASTCtx);
     ASSERT_TRUE(A.hasValue());
@@ -193,7 +193,7 @@ TEST(ToAPValue, FunctionPointersC) {
     const ValueDecl *D = getDecl("func");
     const Pointer &GP = getGlobalPtr("func");
     ASSERT_TRUE(GP.isLive());
-    const FunctionPointer &FP = GP.deref<FunctionPointer>();
+    const Pointer &FP = GP.deref<Pointer>();
     ASSERT_FALSE(FP.isZero());
     APValue A = FP.toAPValue(ASTCtx);
     ASSERT_TRUE(A.hasValue());