[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 <https://reviews.llvm.org/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 at 0
    Hint on symbols that are defined and could potentially match:
      @bar at 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 &TT = 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