[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+    return DefaultFree;
+  return CallOpCC;
----------------
rsmith wrote:
> erichkeane wrote:
> > rjmccall wrote:
> > > ...I made this comment in my first review, but Phabricator threw it away.
> > > 
> > > The attributes let you explicitly request the default method CC, right?  I think you need to check for an explicit attribute rather than just checking CC identity.  There should be an AttributedType in the sugar.
> > They do, but I can't seem to find a way to find them.  The calling convention is already merged into the functiontype by the time we get here, the AttributedType isn't accessible.
> > 
> > So it seems we don't distinguish between "modified by attribute", "modified-default by command line", and "modified-default by TargetInfo."
> > 
> > That said, I somewhat think this is the right thing to do anyway.  If you're on a platform where the default call convention is different between a free-function and member-function, I'd think that this is what you MEAN...
> The `AttributedType` should be present in the type on the `TypeSourceInfo` for the call operator. It might not be present on the type retrieved by `getType()`, though.
> 
> Concretely, what targets have different calling conventions for member versus non-member functions, and what do those calling conventions do differently? (For example, if the default member calling convention treats `this` differently, it doesn't seem reasonable to apply that to the static invoker...)
Answering my own question: the only case where the default member calling convention is different from the default non-member calling convention is for MinGW on 32-bit x86, where members use `thiscall` by default (which passes the first parameter in `%ecx`).

Is it reasonable for `[] [[clang::thiscall]] {}` to result in a static invoker using the `thiscall` calling convention? Intuitively I'd say no, but there's not really anything specific to member functions in `thiscall` -- it just means that we pass the first argument in a register. (However, the first argument of the lambda and the first argument of its static invoker are different, so it's still somewhat unclear whether inheriting `thiscall` is appropriate. But usually for any given lambda only one of the two is actually used.)

But there's another quirk here: the default non-member calling convention can be set on the command line with `-fdefault-calling-conv=` (and a few other flags such as `-mrtd`). This doesn't affect member functions by default. So what should happen if this is compiled with `-fdefault-calling-conv=stdcall` (assuming the default calling convention is otherwise `cdecl`):

```
auto *p0 = [] [[clang::stdcal]] {};
auto *p1 = [] {};
auto *p2 = [] [[clang::cdecl]] {};
```

`p0` seems easy: the default non-member calling convention and the explicit calling convention are the same. The invoker should be `stdcall`.

For `p1`, the default member calling convention is `cdecl` but the default non-member calling convention is `stdcall`. In this case, conformance requires us to use `stdcall` for the pointer type, because `p1` is required to have type `void (*)()`, which is a `stdcall` function pointer in this configuration.

For `p2`, the call operator's calling convention has been explicitly set to the default member calling convention. I think in this case I'd expect a `cdecl` static invoker.

So I think I'm inclined to say we should always apply an explicit calling convention to both the call operator and the static invoker -- that seems like the simplest and easiest to explain rule, and probably matches user expectations most of the time, especially given the observation that you're usually writing a lambda only for one or the other of the call operator and the static invoker, so if you explicitly write a calling convention attribute, you presumably want it to apply to the part of the lambda's interface that you're using.


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

https://reviews.llvm.org/D89559