Require stdcall etc parameters to be complete on ODR use

[rnk]

Summary

Functions using stdcall, fastcall, or vectorcall with C linkage mangle
in the size of the parameter pack. Calculating the size of the pack
requires the parameter types to complete, which may require template
instantiation.

Previously, we would crash during IRgen when requesting the size of
incomplete or uninstantiated types, as in this reduced example:

struct Foo;
void __fastcall bar(struct Foo o);
void (__fastcall *fp)(struct Foo) = &bar;

Reported in Chromium here: https://crbug.com/971245

[inglorion]

Can you clarify "which will usually result in a linker error"? E.g. an example of link.exe rejecting the object file or the wrong function being called. The reason I ask is that if we can be sure at compile time that the resulting code will not work or do the wrong thing, I think giving an error is appropriate. But if that isn't the case, we would be rejecting code that cl.exe accepts and we might want to make it a Warning instead.

clang/lib/Sema/SemaExpr.cpp

14801 ↗

(On Diff #203423)

Do we need those checks or would it be enough to just check the calling convention?

Also, I think s/Do nothing/return false/

14813 ↗

(On Diff #203423)

You can just return false here.

[rnk]

In D62975#1533019, @inglorion wrote:

Can you clarify "which will usually result in a linker error"? E.g. an example of link.exe rejecting the object file or the wrong function being called.

Their linker gives an error, but then it tries to be helpful:

$ cat b.c
struct Foo {int x; };
int __fastcall bar(struct Foo o) { return o.x + 42; }
extern int (__fastcall *fp)(struct Foo);
int main() {
  struct Foo o = { 13 };
  return (*fp)(o);
}

$ cat t.c
struct Foo;
int __fastcall bar(struct Foo o);
int (__fastcall *fp)(struct Foo) = &bar;

$cl -c t.c b.c && link t.obj b.obj -out:t.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.20.27508.1 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

t.c
b.c
Generating Code...
Microsoft (R) Incremental Linker Version 14.20.27508.1
Copyright (C) Microsoft Corporation.  All rights reserved.

t.obj : error LNK2001: unresolved external symbol @bar@0
  Hint on symbols that are defined and could potentially match:
    @bar@4
t.exe : fatal error LNK1120: 1 unresolved externals

The reason I ask is that if we can be sure at compile time that the resulting code will not work or do the wrong thing, I think giving an error is appropriate. But if that isn't the case, we would be rejecting code that cl.exe accepts and we might want to make it a Warning instead.

I guess the only reason I made it a hard error is that we'd have to teach codegen to tolerate incomplete types if we make it a warning that can be bypassed. But that might be worth doing anyway. We'd just do what they do, mangle to @0.

clang/lib/Sema/SemaExpr.cpp

14801 ↗

(On Diff #203423)

I think you can use the calling conventions on non-Windows, but you don't get the mangling, so I think this should return false. Non-x86 architectures probably ignore the __fastcall qualifier with a warning, so we don't need the arch check.

14813 ↗

(On Diff #203423)

I did it this way in an attempt to avoid worrying about the possibility of compilers that warn about falling off the end of the function. Such compilers used to exist, I don't know if we still support them, but I didn't want to find out, so I arranged it this way.

[inglorion]

Thank you. Given that proceeding with the wrong value will result in either an undefined reference or a reference to what might well be the wrong function at link time, I think making this an error (as you've done) is strictly better.

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

def warn_attribute_not_on_decl : Warning<
   "%0 attribute ignored when parsing type">, InGroup<IgnoredAttributes>;
 def err_base_specifier_attribute : Error<
   "%0 attribute cannot be applied to a base specifier">;
 def err_invalid_attribute_on_virtual_function : Error<
   "%0 attribute cannot be applied to virtual functions">;
 def warn_declspec_allocator_nonpointer : Warning<
   "ignoring __declspec(allocator) because the function return type %0 is not "
   "a pointer or reference type">, InGroup<IgnoredAttributes>;
+def err_cconv_incomplete_param_type : Error<
+  "parameter %0 must have a complete type to use function %1 with the %2 "
+  "calling convention">;
 
 def ext_cannot_use_trivial_abi : ExtWarn<
   "'trivial_abi' cannot be applied to %0">, InGroup<IgnoredAttributes>;
 
 // Availability attribute
 def warn_availability_unknown_platform : Warning<
   "unknown platform %0 in availability macro">, InGroup<Availability>;
 def warn_availability_version_ordering : Warning<

cfe/trunk/lib/Sema/SemaExpr.cpp

switch (SemaRef.ExprEvalContexts.back().Context) {
     case Sema::ExpressionEvaluationContext::Unevaluated:
     case Sema::ExpressionEvaluationContext::UnevaluatedAbstract:
       // Expressions in this context are never evaluated.
       return false;
   }
   llvm_unreachable("Invalid context");
 }
 
+/// Return true if this function has a calling convention that requires mangling
+/// in the size of the parameter pack.
+static bool funcHasParameterSizeMangling(Sema &S, FunctionDecl *FD) {
+  // These manglings don't do anything on non-Windows or non-x86 platforms, so
+  // we don't need parameter type sizes.
+  const llvm::Triple &TT = S.Context.getTargetInfo().getTriple();
+  if (!TT.isOSWindows() || (TT.getArch() != llvm::Triple::x86 &&
+                            TT.getArch() != llvm::Triple::x86_64))
+    return false;
+
+  // If this is C++ and this isn't an extern "C" function, parameters do not
+  // need to be complete. In this case, C++ mangling will apply, which doesn't
+  // use the size of the parameters.
+  if (S.getLangOpts().CPlusPlus && !FD->isExternC())
+    return false;
+
+  // Stdcall, fastcall, and vectorcall need this special treatment.
+  CallingConv CC = FD->getType()->castAs<FunctionType>()->getCallConv();
+  switch (CC) {
+  case CC_X86StdCall:
+  case CC_X86FastCall:
+  case CC_X86VectorCall:
+    return true;
+  default:
+    break;
+  }
+  return false;
+}
+
+/// Require that all of the parameter types of function be complete. Normally,
+/// parameter types are only required to be complete when a function is called
+/// or defined, but to mangle functions with certain calling conventions, the
+/// mangler needs to know the size of the parameter list. In this situation,
+/// MSVC doesn't emit an error or instantiate templates. Instead, MSVC mangles
+/// the function as _foo@0, i.e. zero bytes of parameters, which will usually
+/// result in a linker error. Clang doesn't implement this behavior, and instead
+/// attempts to error at compile time.
+static void CheckCompleteParameterTypesForMangler(Sema &S, FunctionDecl *FD,
+                                                  SourceLocation Loc) {
+  class ParamIncompleteTypeDiagnoser : public Sema::TypeDiagnoser {
+    FunctionDecl *FD;
+    ParmVarDecl *Param;
+
+  public:
+    ParamIncompleteTypeDiagnoser(FunctionDecl *FD, ParmVarDecl *Param)
+        : FD(FD), Param(Param) {}
+
+    void diagnose(Sema &S, SourceLocation Loc, QualType T) override {
+      CallingConv CC = FD->getType()->castAs<FunctionType>()->getCallConv();
+      StringRef CCName;
+      switch (CC) {
+      case CC_X86StdCall:
+        CCName = "stdcall";
+        break;
+      case CC_X86FastCall:
+        CCName = "fastcall";
+        break;
+      case CC_X86VectorCall:
+        CCName = "vectorcall";
+        break;
+      default:
+        llvm_unreachable("CC does not need mangling");
+      }
+
+      S.Diag(Loc, diag::err_cconv_incomplete_param_type)
+          << Param->getDeclName() << FD->getDeclName() << CCName;
+    }
+  };
+
+  for (ParmVarDecl *Param : FD->parameters()) {
+    ParamIncompleteTypeDiagnoser Diagnoser(FD, Param);
+    S.RequireCompleteType(Loc, Param->getType(), Diagnoser);
+  }
+}
+
 namespace {
 enum class OdrUseContext {
   /// Declarations in this context are not odr-used.
   None,
   /// Declarations in this context are formally odr-used, but this is a
   /// dependent context.
   Dependent,
   /// Declarations in this context are odr-used but not actually used (yet).
if (!Func->isDefined()) {
       else if (Func->getMostRecentDecl()->isInlined() &&
                !LangOpts.GNUInline &&
                !Func->getMostRecentDecl()->hasAttr<GNUInlineAttr>())
         UndefinedButUsed.insert(std::make_pair(Func->getCanonicalDecl(), Loc));
       else if (isExternalWithNoLinkageType(Func))
         UndefinedButUsed.insert(std::make_pair(Func->getCanonicalDecl(), Loc));
     }
 
+    // Some x86 Windows calling conventions mangle the size of the parameter
+    // pack into the name. Computing the size of the parameters requires the
+    // parameter types to be complete. Check that now.
+    if (funcHasParameterSizeMangling(*this, Func))
+      CheckCompleteParameterTypesForMangler(*this, Func, Loc);
+
     Func->markUsed(Context);
 
     if (LangOpts.OpenMP && LangOpts.OpenMPIsDevice)
       checkOpenMPDeviceFunction(Loc, Func);
   }
 }
 
 /// Directly mark a variable odr-used. Given a choice, prefer to use

cfe/trunk/test/Sema/calling-conv-complete-params.c

+// RUN: %clang_cc1 -fsyntax-only -fms-extensions -verify -triple i686-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -fms-extensions -verify -triple x86_64-pc-win32 %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -fms-extensions -verify -triple i686-pc-win32 %s
+// RUN: %clang_cc1 -x c++ -DEXTERN_C='extern "C"' -fsyntax-only -fms-extensions -verify -triple i686-pc-win32 %s
+
+#ifndef EXTERN_C
+#define EXTERN_C
+#if defined(__cplusplus)
+#define EXPECT_NODIAG
+// expected-no-diagnostics
+#endif
+#endif
+
+#ifndef EXPECT_NODIAG
+// expected-note-re@+2 1+ {{forward declaration of '{{(struct )?}}Foo'}}
+#endif
+struct Foo;
+
+EXTERN_C void __stdcall fwd_std(struct Foo p);
+#if !defined(EXPECT_NODIAG) && defined(_M_IX86)
+// expected-error@+2 {{parameter 'p' must have a complete type to use function 'fwd_std' with the stdcall calling convention}}
+#endif
+void (__stdcall *fp_fwd_std)(struct Foo) = &fwd_std;
+
+EXTERN_C void __fastcall fwd_fast(struct Foo p);
+#if !defined(EXPECT_NODIAG) && defined(_M_IX86)
+// expected-error@+2 {{parameter 'p' must have a complete type to use function 'fwd_fast' with the fastcall calling convention}}
+#endif
+void (__fastcall *fp_fwd_fast)(struct Foo) = &fwd_fast;
+
+EXTERN_C void __vectorcall fwd_vector(struct Foo p);
+#if !defined(EXPECT_NODIAG)
+// expected-error@+2 {{parameter 'p' must have a complete type to use function 'fwd_vector' with the vectorcall calling convention}}
+#endif
+void (__vectorcall *fp_fwd_vector)(struct Foo) = &fwd_vector;
+
+#if defined(__cplusplus)
+template <typename T> struct TemplateWrapper {
+#ifndef EXPECT_NODIAG
+  // expected-error@+2 {{field has incomplete type 'Foo'}}
+#endif
+  T field;
+};
+
+EXTERN_C void __vectorcall tpl_ok(TemplateWrapper<int> p);
+void(__vectorcall *fp_tpl_ok)(TemplateWrapper<int>) = &tpl_ok;
+
+EXTERN_C void __vectorcall tpl_fast(TemplateWrapper<Foo> p);
+#ifndef EXPECT_NODIAG
+// expected-note@+2 {{requested here}}
+#endif
+void(__vectorcall *fp_tpl_fast)(TemplateWrapper<Foo>) = &tpl_fast;
+#endif