[PATCH] D62975: Require stdcall etc parameters to be complete on ODR use

[Reid Kleckner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL363000: Require stdcall etc parameters to be complete on ODR 
use (authored by rnk, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62975?vs=203423=203922#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62975/new/

https://reviews.llvm.org/D62975

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/Sema/calling-conv-complete-params.c

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2985,6 +2985,9 @@
 def warn_declspec_allocator_nonpointer : Warning<
   "ignoring __declspec(allocator) because the function return type %0 is not "
   "a pointer or reference type">, InGroup;
+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;
Index: cfe/trunk/test/Sema/calling-conv-complete-params.c
===
--- cfe/trunk/test/Sema/calling-conv-complete-params.c
+++ cfe/trunk/test/Sema/calling-conv-complete-params.c
@@ -0,0 +1,53 @@
+// 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) = _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) = _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) = _vector;
+
+#if defined(__cplusplus)
+template  struct TemplateWrapper {
+#ifndef EXPECT_NODIAG
+  // expected-error@+2 {{field has incomplete type 'Foo'}}
+#endif
+  T field;
+};
+
+EXTERN_C void __vectorcall tpl_ok(TemplateWrapper p);
+void(__vectorcall *fp_tpl_ok)(TemplateWrapper) = _ok;
+
+EXTERN_C void __vectorcall tpl_fast(TemplateWrapper p);
+#ifndef EXPECT_NODIAG
+// expected-note@+2 {{requested here}}
+#endif
+void(__vectorcall *fp_tpl_fast)(TemplateWrapper) = _fast;
+#endif
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -14793,6 +14793,81 @@
   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 , 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  = 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()->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 

[PATCH] D62975: Require stdcall etc parameters to be complete on ODR use

[Bob Haarman via Phabricator via cfe-commits]

inglorion accepted this revision.
inglorion added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62975/new/

https://reviews.llvm.org/D62975



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62975: Require stdcall etc parameters to be complete on ODR use

[Reid Kleckner via Phabricator via cfe-commits]

rnk marked 2 inline comments as done.
rnk added a comment.

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) = 
  
  $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`.




Comment at: clang/lib/Sema/SemaExpr.cpp:14801
+  const llvm::Triple  = S.Context.getTargetInfo().getTriple();
+  if (!TT.isOSWindows() || (TT.getArch() != llvm::Triple::x86 &&
+TT.getArch() != llvm::Triple::x86_64))

inglorion wrote:
> Do we need those checks or would it be enough to just check the calling 
> convention?
> 
> Also, I think s/Do nothing/return false/
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.



Comment at: clang/lib/Sema/SemaExpr.cpp:14813
+  default:
+break;
+  }

inglorion wrote:
> You can just return false here.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62975/new/

https://reviews.llvm.org/D62975



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62975: Require stdcall etc parameters to be complete on ODR use

[Bob Haarman via Phabricator via cfe-commits]

inglorion added a comment.

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.




Comment at: clang/lib/Sema/SemaExpr.cpp:14801
+  const llvm::Triple  = S.Context.getTargetInfo().getTriple();
+  if (!TT.isOSWindows() || (TT.getArch() != llvm::Triple::x86 &&
+TT.getArch() != llvm::Triple::x86_64))

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

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



Comment at: clang/lib/Sema/SemaExpr.cpp:14813
+  default:
+break;
+  }

You can just return false here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62975/new/

https://reviews.llvm.org/D62975



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62975: Require stdcall etc parameters to be complete on ODR use

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added reviewers: thakis, rsmith, hans.
Herald added a project: clang.

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) = 

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62975

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/calling-conv-complete-params.cpp

Index: clang/test/SemaCXX/calling-conv-complete-params.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/calling-conv-complete-params.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -fms-extensions -verify -triple i686-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -fms-extensions -verify -triple x86_64-pc-win32 %s
+
+struct Foo; // expected-note 1+ {{forward declaration of 'Foo'}}
+
+void __stdcall fwd_std(struct Foo p);
+#ifdef _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) = _std;
+
+void __fastcall fwd_fast(struct Foo p);
+#ifdef _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) = _fast;
+
+void __vectorcall fwd_vector(struct Foo p);
+// expected-error@+1 {{parameter 'p' must have a complete type to use function 'fwd_vector' with the vectorcall calling convention}}
+void (__vectorcall *fp_fwd_vector)(struct Foo) = _vector;
+
+template  struct TemplateWrapper {
+  T field; // expected-error {{field has incomplete type 'Foo'}}
+};
+
+void __vectorcall tpl_ok(TemplateWrapper p);
+void(__vectorcall *fp_tpl_ok)(TemplateWrapper) = _ok;
+
+void __vectorcall tpl_fast(TemplateWrapper p);
+void(__vectorcall *fp_tpl_fast)(TemplateWrapper) = _fast; // expected-note {{requested here}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14793,6 +14793,74 @@
   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 , FunctionDecl *FD) {
+  // Do nothing on non-Windows, non-x86/x64 platforms.
+  const llvm::Triple  = S.Context.getTargetInfo().getTriple();
+  if (!TT.isOSWindows() || (TT.getArch() != llvm::Triple::x86 &&
+TT.getArch() != llvm::Triple::x86_64))
+return false;
+
+  // Stdcall, fastcall, and vectorcall need this special treatment.
+  CallingConv CC = FD->getType()->castAs()->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 , 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 , SourceLocation Loc, QualType T) override {
+  CallingConv CC = FD->getType()->castAs()->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