Add -Wcast-calling-convention to warn when casting away calling conventions

[rnk]

Summary

This only warns on casts of the address of a function defined in the
current TU. In this case, the fix is likely to be local and the warning
useful.

Here are some things we could experiment with in the future:

• Fire on declarations as well as definitions
• Limit the warning to non-void function prototypes
• Limit the warning to mismatches of caller and callee cleanup CCs

This warning is currently off by default while we study its usefulness.

[thakis]

Nice!

include/clang/Basic/DiagnosticGroups.td

294 ↗

(On Diff #48234)

nit: since this isn't referenced in this file, consider using an unnamed inline InGroup<DiagGroup<"cast-calling-convention"> in the other file instead

include/clang/Basic/DiagnosticSemaKinds.td

6515 ↗

(On Diff #48234)

Maybe add ", likely to abort at runtime" to communicate the gravity of the situation better.

6516 ↗

(On Diff #48234)

I'd build a large codebase of your choice with this enabled and see how it does. If it does well I'd land it default-on. (I spent some time enabling DefaultIgnore warnings that nobody turned on after they landed recently, and I'd prefer to not having to do this for this warning too :-) )

lib/Sema/SemaCast.cpp

1762 ↗

(On Diff #48234)

If it's an inline function in a header, would we want to warn? Should this check if the body's SourceLocation is from the main file?

1775 ↗

(On Diff #48234)

Consider doing something like r164858 did to see if there's a define for the calling convention (WINAPI or what have you) and suggest that instead

[rtrieu]

lib/Sema/SemaCast.cpp

1729 ↗

(On Diff #48234)

Capitalize Diagnose like the other Diagnose functions in the file.

1752–1753 ↗

(On Diff #48234)

Which UnaryOperator are you attempting to strip off here? This one will strip off every UnaryOperator.

1769–1773 ↗

(On Diff #48234)

Usually in Sema, we uses a SmallVector backed ostream for making custom strings.

SmallString<64> CCAttrText;
llvm::raw_svector_ostream OS(CCAttrText);
OS << "some string";

[rnk]

• address comments

[rnk]

include/clang/Basic/DiagnosticGroups.td

294 ↗

(On Diff #48234)

Sure. I dono, I never really liked that style since it makes it harder to add it to a group later.

include/clang/Basic/DiagnosticSemaKinds.td

6516 ↗

(On Diff #48234)

My attempt to bait you into doing this work has failed. :) I'd still like to land it as ignored-by-default so I can run this warning across Chromium with a try job instead of doing it locally on my workstation. Sound good?

lib/Sema/SemaCast.cpp

1752–1753 ↗

(On Diff #48234)

Oops, added a test for that.

1762 ↗

(On Diff #48234)

Yeah, we do want to warn even if it's from a header. If it were defined elsewhere with a different convention, that'd be an ODR violation.

1775 ↗

(On Diff #48234)

Done, that was fun. :)

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def err_cast_pointer_from_non_pointer_int : Error<
   "operand of type %0 cannot be cast to a pointer type">;
 def warn_cast_pointer_from_sel : Warning<
   "cast of type %0 to %1 is deprecated; use sel_getName instead">,
   InGroup<SelTypeCast>;
 def warn_function_def_in_objc_container : Warning<
   "function definition inside an Objective-C container is deprecated">,
   InGroup<FunctionDefInObjCContainer>;
-  
+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 err_cast_pointer_to_non_pointer_int : Error<
   "pointer cannot be cast to type %0">;
 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<

cfe/trunk/lib/Sema/SemaCast.cpp

 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.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 "llvm/ADT/SmallVector.h"
 #include <set>
 using namespace clang;
 
 
 
 enum TryCastResult {
if (SrcPtrTy->isObjCSelType()) {
         DT = DestType->getPointeeType();
       if (!DT.getUnqualifiedType()->isVoidType())
         Self.Diag(SrcExpr.get()->getExprLoc(),
                   diag::warn_cast_pointer_from_sel)
         << SrcType << DestType << SrcExpr.get()->getSourceRange();
     }
 }
 
+/// Diagnose casts that change the calling convention of a pointer to a function
+/// defined in the current TU.
+static void DiagnoseCallingConvCast(Sema &Self, const ExprResult &SrcExpr,
+                                    QualType DstType, SourceRange OpRange) {
+  // Check if this cast would change the calling convention of a function
+  // pointer type.
+  QualType SrcType = SrcExpr.get()->getType();
+  if (Self.Context.hasSameType(SrcType, DstType) ||
+      !SrcType->isFunctionPointerType() || !DstType->isFunctionPointerType())
+    return;
+  const auto *SrcFTy =
+      SrcType->castAs<PointerType>()->getPointeeType()->castAs<FunctionType>();
+  const auto *DstFTy =
+      DstType->castAs<PointerType>()->getPointeeType()->castAs<FunctionType>();
+  CallingConv SrcCC = SrcFTy->getCallConv();
+  CallingConv DstCC = DstFTy->getCallConv();
+  if (SrcCC == DstCC)
+    return;
+
+  // We have a calling convention cast. Check if the source is a pointer to a
+  // known, specific function that has already been defined.
+  Expr *Src = SrcExpr.get()->IgnoreParenImpCasts();
+  if (auto *UO = dyn_cast<UnaryOperator>(Src))
+    if (UO->getOpcode() == UO_AddrOf)
+      Src = UO->getSubExpr()->IgnoreParenImpCasts();
+  auto *DRE = dyn_cast<DeclRefExpr>(Src);
+  if (!DRE)
+    return;
+  auto *FD = dyn_cast<FunctionDecl>(DRE->getDecl());
+  const FunctionDecl *Definition;
+  if (!FD || !FD->hasBody(Definition))
+    return;
+
+  // The source expression is a pointer to a known function defined in this TU.
+  // Diagnose this cast, as it is probably bad.
+  StringRef SrcCCName = FunctionType::getNameForCallConv(SrcCC);
+  StringRef DstCCName = FunctionType::getNameForCallConv(DstCC);
+  Self.Diag(OpRange.getBegin(), diag::warn_cast_calling_conv)
+      << SrcCCName << DstCCName << OpRange;
+
+  // The checks above are cheaper than checking if the diagnostic is enabled.
+  // However, it's worth checking if the warning is enabled before we construct
+  // a fixit.
+  if (Self.Diags.isIgnored(diag::warn_cast_calling_conv, OpRange.getBegin()))
+    return;
+
+  // Try to suggest a fixit to change the calling convention of the function
+  // whose address was taken. Try to use the latest macro for the convention.
+  // For example, users probably want to write "WINAPI" instead of "__stdcall"
+  // to match the Windows header declarations.
+  SourceLocation NameLoc = Definition->getNameInfo().getLoc();
+  Preprocessor &PP = Self.getPreprocessor();
+  SmallVector<TokenValue, 6> AttrTokens;
+  SmallString<64> CCAttrText;
+  llvm::raw_svector_ostream OS(CCAttrText);
+  if (Self.getLangOpts().MicrosoftExt) {
+    // __stdcall or __vectorcall
+    OS << "__" << DstCCName;
+    IdentifierInfo *II = PP.getIdentifierInfo(OS.str());
+    AttrTokens.push_back(II->isKeyword(Self.getLangOpts())
+                             ? TokenValue(II->getTokenID())
+                             : TokenValue(II));
+  } else {
+    // __attribute__((stdcall)) or __attribute__((vectorcall))
+    OS << "__attribute__((" << DstCCName << "))";
+    AttrTokens.push_back(tok::kw___attribute);
+    AttrTokens.push_back(tok::l_paren);
+    AttrTokens.push_back(tok::l_paren);
+    IdentifierInfo *II = PP.getIdentifierInfo(DstCCName);
+    AttrTokens.push_back(II->isKeyword(Self.getLangOpts())
+                             ? TokenValue(II->getTokenID())
+                             : TokenValue(II));
+    AttrTokens.push_back(tok::r_paren);
+    AttrTokens.push_back(tok::r_paren);
+  }
+  StringRef AttrSpelling = PP.getLastMacroWithSpelling(NameLoc, AttrTokens);
+  if (!AttrSpelling.empty())
+    CCAttrText = AttrSpelling;
+  OS << ' ';
+  Self.Diag(NameLoc, diag::note_change_calling_conv_fixit)
+      << FD << DstCCName << FixItHint::CreateInsertion(NameLoc, CCAttrText);
+}
+
 static void checkIntToPointerCast(bool CStyle, SourceLocation Loc,
                                   const Expr *SrcExpr, QualType DestType,
                                   Sema &Self) {
   QualType SrcType = SrcExpr->getType();
 
   // Not warning on reinterpret_cast, boolean, constant expressions, etc
   // are not explicit design choices, but consistent with GCC's behavior.
   // Feel free to modify them if you've reason/evidence for an alternative.
static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr,
 
   // Any pointer can be cast to an Objective-C pointer type with a C-style
   // cast.
   if (CStyle && DestType->isObjCObjectPointerType()) {
     return TC_Success;
   }
   if (CStyle)
     DiagnoseCastOfObjCSEL(Self, SrcExpr, DestType);
-  
+  DiagnoseCallingConvCast(Self, SrcExpr, DestType, OpRange);
+
   // Not casting away constness, so the only remaining check is for compatible
   // pointer categories.
 
   if (SrcType->isFunctionPointerType()) {
     if (DestType->isFunctionPointerType()) {
       // C++ 5.2.10p6: A pointer to a function can be explicitly converted to
       // a pointer to a function of a different type.
       return TC_Success;
else if (!Self.CheckObjCARCUnavailableWeakConversion(DestType, SrcType)) {
                 diag::err_arc_convesion_of_weak_unavailable)
         << 1 << SrcType << DestType << SrcExpr.get()->getSourceRange();
       SrcExpr = ExprError();
       return;
     }
   }
   
   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)
     checkCastAlign();
 

cfe/trunk/test/Sema/callingconv-cast.c

+// RUN: %clang_cc1 -fms-extensions -triple i686-pc-windows-msvc -Wcast-calling-convention -DMSVC -Wno-pointer-bool-conversion -verify -x c %s
+// RUN: %clang_cc1 -fms-extensions -triple i686-pc-windows-msvc -Wcast-calling-convention -DMSVC -Wno-pointer-bool-conversion -verify -x c++ %s
+// RUN: %clang_cc1 -fms-extensions -triple i686-pc-windows-msvc -Wcast-calling-convention -DMSVC -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s --check-prefix=MSFIXIT
+// RUN: %clang_cc1 -triple i686-pc-windows-gnu -Wcast-calling-convention -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s --check-prefix=GNUFIXIT
+
+// expected-note@+1 {{consider defining 'mismatched_before_winapi' with the 'stdcall' calling convention}}
+void mismatched_before_winapi(int x) {}
+
+#ifdef MSVC
+#define WINAPI __stdcall
+#else
+#define WINAPI __attribute__((stdcall))
+#endif
+
+// expected-note@+1 3 {{consider defining 'mismatched' with the 'stdcall' calling convention}}
+void mismatched(int x) {}
+
+typedef void (WINAPI *callback_t)(int);
+void take_callback(callback_t callback);
+
+int main() {
+  // expected-warning@+1 {{cast between incompatible calling conventions 'cdecl' and 'stdcall'}}
+  take_callback((callback_t)mismatched);
+
+  // expected-warning@+1 {{cast between incompatible calling conventions 'cdecl' and 'stdcall'}}
+  callback_t callback = (callback_t)mismatched; // warns
+  (void)callback;
+
+  // expected-warning@+1 {{cast between incompatible calling conventions 'cdecl' and 'stdcall'}}
+  callback = (callback_t)&mismatched; // warns
+
+  // No warning, just to show we don't drill through other kinds of unary operators.
+  callback = (callback_t)!mismatched;
+
+  // expected-warning@+1 {{cast between incompatible calling conventions 'cdecl' and 'stdcall'}}
+  callback = (callback_t)&mismatched_before_winapi; // warns
+
+  // Probably a bug, but we don't warn.
+  void (*callback2)(int) = mismatched;
+  take_callback((callback_t)callback2);
+
+  // Another way to suppress the warning.
+  take_callback((callback_t)(void*)mismatched);
+}
+
+// MSFIXIT: fix-it:"D:\\src\\llvm\\tools\\clang\\test\\Sema\\callingconv-cast.c":{16:6-16:6}:"WINAPI "
+// MSFIXIT: fix-it:"D:\\src\\llvm\\tools\\clang\\test\\Sema\\callingconv-cast.c":{16:6-16:6}:"WINAPI "
+// MSFIXIT: fix-it:"D:\\src\\llvm\\tools\\clang\\test\\Sema\\callingconv-cast.c":{16:6-16:6}:"WINAPI "
+// MSFIXIT: fix-it:"D:\\src\\llvm\\tools\\clang\\test\\Sema\\callingconv-cast.c":{7:6-7:6}:"__stdcall "
+
+// GNUFIXIT: fix-it:"D:\\src\\llvm\\tools\\clang\\test\\Sema\\callingconv-cast.c":{16:6-16:6}:"WINAPI "
+// GNUFIXIT: fix-it:"D:\\src\\llvm\\tools\\clang\\test\\Sema\\callingconv-cast.c":{16:6-16:6}:"WINAPI "
+// GNUFIXIT: fix-it:"D:\\src\\llvm\\tools\\clang\\test\\Sema\\callingconv-cast.c":{16:6-16:6}:"WINAPI "
+// GNUFIXIT: fix-it:"D:\\src\\llvm\\tools\\clang\\test\\Sema\\callingconv-cast.c":{7:6-7:6}:"__attribute__((stdcall)) "