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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:1013
   CXXMethodDecl *getLambdaStaticInvoker() const;
+  CXXMethodDecl *getLambdaStaticInvoker(CallingConv CC) const;
+
----------------
rjmccall wrote:
> Probably worth clarifying in the comment which invoker is returned by the no-arguments variant.
I just looked and the first is actually only used 1x!  I think I can just replace that usage and remove the former overload if that is acceptable to you.


================
Comment at: clang/lib/AST/DeclCXX.cpp:1550
+                 return FTy->getCallConv() == CC;
+               });
+
----------------
rjmccall wrote:
> This seems both rather more complicated and rather more expensive than a simple loop which assigns to a local pointer.
Do you mean the filtering copy-if?  The idea was to make it so that the assert below still applies.  If that isn't necessary, this DOES simplify to a std::find and a return.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:2268
+    } else if (const auto *AT = dyn_cast_or_null<AutoType>(
+                   ResultType->getContainedAutoType())) {
       Out << '?';
----------------
rjmccall wrote:
> So, this is changing the mangling of lambda conversion operators.  Is this what MSVC does?
Yes, this mangles the 'return type' of the conversion operator, which MSVC does as well.  We have an incompatible mangling (already) that skipped this because the type has some sort of deduction in it (to protect against a type that is defined in terms of the same type).




================
Comment at: clang/lib/Sema/SemaLambda.cpp:1467
+  llvm::SmallVector<CallingConv, 2> Conventions =
+      getLambdaConversionFunctionCallConv(S, CallOpProto);
+
----------------
rjmccall wrote:
> Can we make this call a callback or something rather than returning an array that almost always has one element?
Sure!  Can do.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+    return DefaultFree;
+  return CallOpCC;
----------------
rjmccall wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > 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.
> > > 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.
> > 
> > Do we have a good way to determining which of the 3 mechanisms the user used to set the calling convention?  (Attribute vs -fdefault-calling-conv= vs just using the defualt?)
> > 
> > It would be easy enough to ALWAYS have the static-invoker match the calling-convention of the operator(), as long as having it as a 'this' call isn't a problem, which given what you said above, I don't think thats a problem.  The implementation is trivial, since it is just removing the condition above.
> > 
> > I guess I'm just asking which you'd like, and if it is the "only have invoker match if user requested explicitly", if you had any hints on how to figure that out.
> > 
> > Thanks!
> I would guess that we probably have an AttributedType if and only if the user wrote an attribute explicitly, but I don't know for sure if we make one in the case where it's the only signature information in the lambda.  I also don't know if it survives the type-rewrite that happens after return-type inference.
I've not been able to get it to produce an AttributedType here at all.  The calling-convention is merged into the function-type directly and the attributed-type is canonicalized away in every combination that i could provide.


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

https://reviews.llvm.org/D89559