[Clang][Sema] Implement GCC -Wcast-function-type

[ychen]

Summary

Warn when a function pointer is cast to an incompatible function
pointer. In a cast involving function types with a variable argument
list only the types of initial arguments that are provided are
considered. Any parameter of pointer-type matches any other
pointer-type. Any benign differences in integral types are ignored, like
int vs. long on ILP32 targets. Likewise type qualifiers are ignored. The
function type void (*) (void) is special and matches everything, which
can be used to suppress this warning. In a cast involving pointer to
member types this warning warns whenever the type cast is changing the
pointer to member type. This warning is enabled by -Wextra.

[ychen]

ping?

[ychen]

ping.

[ychen]

• update

[ychen]

ping

[rsmith]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8390

I'd find something like "cast from function type %0 to incompatible function type %1" to be easier to read. But I suppose %0 and %1 are the pointer types, not the function types, so perhaps "cast from %0 to %1 converts to incompatible function type" or something like that?

clang/lib/Sema/SemaCast.cpp

1044–1048

In addition to allowing the cases where the sizes are the same width, should we also allow cases where the promoted types are the same width (eg, int versus short)? What does GCC do?

1050–1055

I think a "same type" check (Context.hasSameUnqualifiedType(SrcType, DestType)) would be more appropriate here than a structural equivalence check.

1067–1081

This can be simplified a bit.

1082–1083

We should also handle the case where the source is of function type and the destination is a reference-to-function type.

Maybe also block pointer types?

1090

Please use auto * here (or const auto * as the linter suggests).

[ychen]

• Add code/tests for function -> function pointer and block pointer -> function pointer.
• Use Context.hasSameUnqualifiedType
• Style

clang/lib/Sema/SemaCast.cpp

1044–1048

GCC does exactly that. I didn't find a way to do that by looking at TargetInfo. Maybe there is a way for clang?

1050–1055

Agreed.

1082–1083

Yep. Code/tests added for function -> function pointer and block pointer -> function pointer.

[rsmith]

Looks good. Some possible improvements to make the diagnostic a bit more precise, but I'd be happy if you want to address some of those as follow-ups after this change.

clang/lib/Sema/SemaCast.cpp

1041

I think we should also treat references like pointers here, and similarly for block pointers and ObjC pointers. Perhaps using hasPointerRepresentation() would make sense?

1044–1048

Hm. The right thing to do would be to call ABIInfo::isPromotableIntegerTypeForABI, but that's defined in CodeGen so we can't call it from here without violating layering. I suppose we could pull it up into the target info in Basic, if you're prepared to do that kind of refactoring, but maybe for now we can skip this check and emit a few warnings that GCC doesn't.

1050–1055

Taking this a bit further: we could use hasSimilarType to allow changes in nested cv-qualifiers. But given that we've already allowed arbitrary differences in pointee types, I suppose this would only affect pointers-to-members, and it might be preferable to allow arbitrary changes between pointer-to-member types here (except in the MS ABI case where there are meaningful differences in pointer-to-member representations between classes).

What does GCC do with differences in pointer-to-member types in function parameter types?

[ychen]

Thanks for the insightful suggestions. I'll definitely send a follow-up patch for these (pointerLike arguments, TargetInfo refactoring, promotable integral types).

[ychen]

• update doc to match diagnostics

clang/docs/DiagnosticsReference.rst

 -Wbad-function-cast
 -------------------
 **Diagnostic text:**
 
 +--------------------------------------------------------------------------------------------------------------------------------------------------------------+
 |:warning:`warning:` |nbsp| :diagtext:`cast from function call of type` |nbsp| :placeholder:`A` |nbsp| :diagtext:`to non-matching type` |nbsp| :placeholder:`B`|
 +--------------------------------------------------------------------------------------------------------------------------------------------------------------+
 
+-Wcast-function-type
+-------------------
+**Diagnostic text:**
+
++--------------------------------------------------------------------------------------------------------------------------------------------------------------+
+|:warning:`warning:` |nbsp| :diagtext:`cast from` |nbsp| :placeholder:`A` |nbsp| :diagtext:`to` |nbsp| :placeholder:`B` |nbsp| :diagtext:`converts to incompatible function types`|
++--------------------------------------------------------------------------------------------------------------------------------------------------------------+
 
 -Wbinary-literal
 ----------------
 Controls `-Wc++14-binary-literal`_, `-Wc++98-c++11-compat-binary-literal`_, `-Wgnu-binary-literal`_.
 
 
 -Wbind-to-temporary-copy
 ------------------------

clang/include/clang/Basic/DiagnosticGroups.td

 def ObjCReceiver : DiagGroup<"receiver-expr">;
 def OperatorNewReturnsNull : DiagGroup<"new-returns-null">;
 def OverlengthStrings : DiagGroup<"overlength-strings">;
 def OverloadedVirtual : DiagGroup<"overloaded-virtual">;
 def PrivateExtern : DiagGroup<"private-extern">;
 def SelTypeCast : DiagGroup<"cast-of-sel-type">;
 def FunctionDefInObjCContainer : DiagGroup<"function-def-in-objc-container">;
 def BadFunctionCast : DiagGroup<"bad-function-cast">;
+def CastFunctionType : DiagGroup<"cast-function-type">;
 def ObjCPropertyImpl : DiagGroup<"objc-property-implementation">;
 def ObjCPropertyNoAttribute : DiagGroup<"objc-property-no-attribute">;
 def ObjCPropertyAssignOnObjectType : DiagGroup<"objc-property-assign-on-object-type">;
 def ObjCProtocolQualifiers : DiagGroup<"objc-protocol-qualifiers">;
 def ObjCMissingSuperCalls : DiagGroup<"objc-missing-super-calls">;
 def ObjCDesignatedInit : DiagGroup<"objc-designated-initializers">;
 def ObjCRetainBlockProperty : DiagGroup<"objc-noncopy-retain-block-property">;
 def ObjCReadonlyPropertyHasSetter : DiagGroup<"objc-readonly-with-setter-property">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_cast_calling_conv : Warning<
   "cast between incompatible calling conventions '%0' and '%1'; "
   "calls through this pointer may abort at runtime">,
   InGroup<DiagGroup<"cast-calling-convention">>;
 def note_change_calling_conv_fixit : Note<
   "consider defining %0 with the '%1' calling convention">;
 def warn_bad_function_cast : Warning<
   "cast from function call of type %0 to non-matching type %1">,
   InGroup<BadFunctionCast>, DefaultIgnore;
+def warn_cast_function_type : Warning<
+  "cast from %0 to %1 converts to incompatible function types">,
+  InGroup<CastFunctionType>, DefaultIgnore;
 def err_cast_pointer_to_non_pointer_int : Error<
   "pointer cannot be cast to type %0">;
 def err_cast_to_bfloat16 : Error<"cannot type-cast to __bf16">;
 def err_cast_from_bfloat16 : Error<"cannot type-cast from __bf16">;
 def err_typecheck_expect_scalar_operand : Error<
   "operand of type %0 where arithmetic or pointer type is required">;
 def err_typecheck_cond_incompatible_operands : Error<
   "incompatible operand types%diff{ ($ and $)|}0,1">;

clang/lib/Sema/SemaCast.cpp

 //===--- SemaCast.cpp - Semantic Analysis for Casts -----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements semantic analysis for cast expressions, including
 //  1) C-style casts like '(int) x'
 //  2) C++ functional casts like 'int(x)'
 //  3) C++ named casts like 'static_cast<int>(x)'
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/Initialization.h"
+#include "clang/Sema/SemaInternal.h"
 #include "llvm/ADT/SmallVector.h"
 #include <set>
 using namespace clang;
 
 
 
 enum TryCastResult {
   TC_NotApplicable, ///< The cast method is not applicable.
static void DiagnoseReinterpretUpDownCast(Sema &Self, const Expr *SrcExpr,
   Self.Diag(BeginLoc, diag::warn_reinterpret_different_from_static)
     << DerivedType << BaseType << !VirtualBase << int(ReinterpretKind)
     << OpRange;
   Self.Diag(BeginLoc, diag::note_reinterpret_updowncast_use_static)
     << int(ReinterpretKind)
     << FixItHint::CreateReplacement(BeginLoc, "static_cast");
 }
 
+static bool argTypeIsABIEquivalent(QualType SrcType, QualType DestType,
+                                   ASTContext &Context) {
+  if (SrcType->isPointerType() && DestType->isPointerType())
+    return true;
+
+  // Allow integral type mismatch if their size are equal.
+  if (SrcType->isIntegralType(Context) && DestType->isIntegralType(Context))
+    if (Context.getTypeInfoInChars(SrcType).Width ==
+        Context.getTypeInfoInChars(DestType).Width)
+      return true;
+
+  return Context.hasSameUnqualifiedType(SrcType, DestType);
+}
+
+static bool checkCastFunctionType(Sema &Self, const ExprResult &SrcExpr,
+                                  QualType DestType) {
+  if (Self.Diags.isIgnored(diag::warn_cast_function_type,
+                           SrcExpr.get()->getExprLoc()))
+    return true;
+
+  QualType SrcType = SrcExpr.get()->getType();
+  const FunctionType *SrcFTy = nullptr;
+  const FunctionType *DstFTy = nullptr;
+  if (((SrcType->isBlockPointerType() || SrcType->isFunctionPointerType()) &&
+       DestType->isFunctionPointerType()) ||
+      (SrcType->isMemberFunctionPointerType() &&
+       DestType->isMemberFunctionPointerType())) {
+    SrcFTy = SrcType->getPointeeType()->castAs<FunctionType>();
+    DstFTy = DestType->getPointeeType()->castAs<FunctionType>();
+  } else if (SrcType->isFunctionType() && DestType->isFunctionReferenceType()) {
+    SrcFTy = SrcType->castAs<FunctionType>();
+    DstFTy = DestType.getNonReferenceType()->castAs<FunctionType>();
+  } else {
+    return true;
+  }
+  assert(SrcFTy && DstFTy);
+
+  auto IsVoidVoid = [](const FunctionType *T) {
+    if (!T->getReturnType()->isVoidType())
+      return false;
+    if (const auto *PT = T->getAs<FunctionProtoType>())
+      return !PT->isVariadic() && PT->getNumParams() == 0;
+    return false;
+  };
+
+  // Skip if either function type is void(*)(void)
+  if (IsVoidVoid(SrcFTy) || IsVoidVoid(DstFTy))
+    return true;
+
+  // Check return type.
+  if (!argTypeIsABIEquivalent(SrcFTy->getReturnType(), DstFTy->getReturnType(),
+                              Self.Context))
+    return false;
+
+  // Check if either has unspecified number of parameters
+  if (SrcFTy->isFunctionNoProtoType() || DstFTy->isFunctionNoProtoType())
+    return true;
+
+  // Check parameter types.
+
+  const auto *SrcFPTy = cast<FunctionProtoType>(SrcFTy);
+  const auto *DstFPTy = cast<FunctionProtoType>(DstFTy);
+
+  // In a cast involving function types with a variable argument list only the
+  // types of initial arguments that are provided are considered.
+  unsigned NumParams = SrcFPTy->getNumParams();
+  unsigned DstNumParams = DstFPTy->getNumParams();
+  if (NumParams > DstNumParams) {
+    if (!DstFPTy->isVariadic())
+      return false;
+    NumParams = DstNumParams;
+  } else if (NumParams < DstNumParams) {
+    if (!SrcFPTy->isVariadic())
+      return false;
+  }
+
+  for (unsigned i = 0; i < NumParams; ++i)
+    if (!argTypeIsABIEquivalent(SrcFPTy->getParamType(i),
+                                DstFPTy->getParamType(i), Self.Context))
+      return false;
+
+  return true;
+}
+
 /// CheckReinterpretCast - Check that a reinterpret_cast\<DestType\>(SrcExpr) is
 /// valid.
 /// Refer to C++ 5.2.10 for details. reinterpret_cast is typically used in code
 /// like this:
 /// char *bytes = reinterpret_cast\<char*\>(int_ptr);
 void CastOperation::CheckReinterpretCast() {
   if (ValueKind == VK_RValue && !isPlaceholder(BuiltinType::Overload))
     SrcExpr = Self.DefaultFunctionArrayLvalueConversion(SrcExpr.get());
if (SrcExpr.get()->getType() == Self.Context.OverloadTy) {
                       DestType, /*listInitialization=*/false);
     }
   }
 
   if (isValidCast(tcr)) {
     if (Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers())
       checkObjCConversion(Sema::CCK_OtherCast);
     DiagnoseReinterpretUpDownCast(Self, SrcExpr.get(), DestType, OpRange);
+
+    if (!checkCastFunctionType(Self, SrcExpr, DestType))
+      Self.Diag(OpRange.getBegin(), diag::warn_cast_function_type)
+          << SrcExpr.get()->getType() << DestType << OpRange;
   } else {
     SrcExpr = ExprError();
   }
 }
 
 
 /// CheckStaticCast - Check that a static_cast\<DestType\>(SrcExpr) is valid.
 /// Refer to C++ 5.2.9 for details. Static casts are mostly used for making
if (SrcExpr.get()->getType() == Self.Context.OverloadTy) {
       diagnoseBadCast(Self, msg, (FunctionalStyle ? CT_Functional : CT_CStyle),
                       OpRange, SrcExpr.get(), DestType, ListInitialization);
     }
   }
 
   if (isValidCast(tcr)) {
     if (Kind == CK_BitCast)
       checkCastAlign();
+
+    if (!checkCastFunctionType(Self, SrcExpr, DestType))
+      Self.Diag(OpRange.getBegin(), diag::warn_cast_function_type)
+          << SrcExpr.get()->getType() << DestType << OpRange;
+
   } else {
     SrcExpr = ExprError();
   }
 }
 
 /// DiagnoseBadFunctionCast - Warn whenever a function call is cast to a
 ///  non-matching type. Such as enum function call to int, int call to
 /// pointer; etc. Cast to 'void' is an exception.
else if (!Self.CheckObjCARCUnavailableWeakConversion(DestType, SrcType)) {
       Self.Diag(SrcExpr.get()->getBeginLoc(),
                 diag::err_arc_convesion_of_weak_unavailable)
           << 1 << SrcType << DestType << SrcExpr.get()->getSourceRange();
       SrcExpr = ExprError();
       return;
     }
   }
 
+  if (!checkCastFunctionType(Self, SrcExpr, DestType))
+    Self.Diag(OpRange.getBegin(), diag::warn_cast_function_type)
+        << SrcType << DestType << OpRange;
+
   DiagnoseCastOfObjCSEL(Self, SrcExpr, DestType);
   DiagnoseCallingConvCast(Self, SrcExpr, DestType, OpRange);
   DiagnoseBadFunctionCast(Self, SrcExpr, DestType);
   Kind = Self.PrepareScalarCast(SrcExpr, DestType);
   if (SrcExpr.isInvalid())
     return;
 
   if (Kind == CK_BitCast)

clang/test/Sema/warn-cast-function-type.c

+// RUN: %clang_cc1 -x c %s -fsyntax-only -Wcast-function-type -triple x86_64-- -verify
+
+int x(long);
+
+typedef int (f1)(long);
+typedef int (f2)(void*);
+typedef int (f3)();
+typedef void (f4)();
+typedef void (f5)(void);
+typedef int (f6)(long, int);
+typedef int (f7)(long,...);
+
+f1 *a;
+f2 *b;
+f3 *c;
+f4 *d;
+f5 *e;
+f6 *f;
+f7 *g;
+
+void foo(void) {
+  a = (f1 *)x;
+  b = (f2 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function types}} */
+  c = (f3 *)x;
+  d = (f4 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f4 *' (aka 'void (*)()') converts to incompatible function types}} */
+  e = (f5 *)x;
+  f = (f6 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f6 *' (aka 'int (*)(long, int)') converts to incompatible function types}} */
+  g = (f7 *)x;
+}

clang/test/Sema/warn-cast-function-type.cpp

+// RUN: %clang_cc1 -x c++ %s -fblocks -fsyntax-only -Wcast-function-type -triple x86_64-- -verify
+
+int x(long);
+
+typedef int (f1)(long);
+typedef int (f2)(void*);
+typedef int (f3)(...);
+typedef void (f4)(...);
+typedef void (f5)(void);
+typedef int (f6)(long, int);
+typedef int (f7)(long,...);
+typedef int (&f8)(long, int);
+
+f1 *a;
+f2 *b;
+f3 *c;
+f4 *d;
+f5 *e;
+f6 *f;
+f7 *g;
+
+struct S
+{
+  void foo (int*);
+  void bar (int);
+};
+
+typedef void (S::*mf)(int);
+
+void foo() {
+  a = (f1 *)x;
+  b = (f2 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function types}} */
+  b = reinterpret_cast<f2 *>(x); /* expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function types}} */
+  c = (f3 *)x;
+  d = (f4 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f4 *' (aka 'void (*)(...)') converts to incompatible function types}} */
+  e = (f5 *)x;
+  f = (f6 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f6 *' (aka 'int (*)(long, int)') converts to incompatible function types}} */
+  g = (f7 *)x;
+
+  mf p1 = (mf)&S::foo; /* expected-warning {{cast from 'void (S::*)(int *)' to 'mf' (aka 'void (S::*)(int)') converts to incompatible function types}} */
+
+  f8 f2 = (f8)x; /* expected-warning {{cast from 'int (long)' to 'f8' (aka 'int (&)(long, int)') converts to incompatible function types}} */
+  (void)f2;
+
+  int (^y)(long);
+  f = (f6 *)y; /* expected-warning {{cast from 'int (^)(long)' to 'f6 *' (aka 'int (*)(long, int)') converts to incompatible function types}} */
+}