This is an archive of the discontinued LLVM Phabricator instance.

PR47372: Fix Lambda invoker calling conventions
ClosedPublic

Authored by erichkeane on Oct 16 2020, 9:13 AM.

Details

Summary

As mentioned in the defect, the lambda static invoker does not follow
the calling convention of the lambda itself, which seems wrong. This
patch ensures that the calling convention of operator() is passed onto
the invoker and conversion-operator type.

This is accomplished by extracting the calling-convention determination
code out into a separate function in order to better reflect the 'thiscall'
work, as well as somewhat better support the future implementation of
https://devblogs.microsoft.com/oldnewthing/20150220-00/?p=44623

Diff Detail

Event Timeline

erichkeane requested review of this revision.Oct 16 2020, 9:13 AM
erichkeane created this revision.

I agree that it seems reasonable to preserve an explicit CC from the lambda on the converted function pointer.

clang/lib/Sema/SemaLambda.cpp
1268

I don't think we use calc as a prefix anywhere else in the compiler. Maybe get?

erichkeane marked an inline comment as done.Oct 16 2020, 10:17 AM
erichkeane added inline comments.
clang/lib/Sema/SemaLambda.cpp
1268

WFM!

rjmccall added inline comments.Oct 16 2020, 10:17 AM
clang/lib/Sema/SemaLambda.cpp
1278

...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.

erichkeane added inline comments.Oct 16 2020, 10:34 AM
clang/lib/Sema/SemaLambda.cpp
1278

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...

erichkeane marked an inline comment as done.

Calc->get.

No, if you put a calling convention on a lambda and then convert it to a function pointer, you almost certainly want that CC to be honored.

Is the AttributedType still present in CallOperator->getType()?

No, if you put a calling convention on a lambda and then convert it to a function pointer, you almost certainly want that CC to be honored.

Is the AttributedType still present in CallOperator->getType()?

No, it is not at that point:

(gdb) p CallOperator->getType()->dump()
FunctionProtoType 0x1177a160 'void (void) attribute((thiscall)) const' const thiscall
`-AutoType 0x1177a0d0 'void' sugar

`-BuiltinType 0x11716dc0 'void'

Perhaps a better example:

(gdb) p CallOperator->getType()->dump()
FunctionProtoType 0x1177edf0 'void (void) attribute((vectorcall)) const' const vectorcall
`-AutoType 0x1177a0d0 'void' sugar

`-BuiltinType 0x11716dc0 'void'
rsmith added inline comments.Oct 16 2020, 2:53 PM
clang/lib/Sema/SemaLambda.cpp
1278

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

rsmith added inline comments.Oct 16 2020, 3:10 PM
clang/lib/Sema/SemaLambda.cpp
1278

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.

erichkeane added inline comments.Oct 16 2020, 4:49 PM
clang/lib/Sema/SemaLambda.cpp
1278

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!

We can't have it always match, that would require us to reject the conversion to a bare function-pointer type, which is required by the standard.

I'm not sure if MSVC's multiple conversions hack is conformant or not, but it's at least more conformant than that.

erichkeane added a comment.EditedOct 19 2020, 5:51 AM

We can't have it always match, that would require us to reject the conversion to a bare function-pointer type, which is required by the standard.

I'm not sure if MSVC's multiple conversions hack is conformant or not, but it's at least more conformant than that.

Right, I should have remembered that. Though, I guess that brings up the alternative, which is to simply produce ALL of the conversion/invoke functions like MSVC. I was originally attempting to implement that (under a MSVCCompat flag) and decided fixing this first would be an easier way to go, but I could just continue with that implementation for ALL valid CCs on all platforms.

It is slightly more difficult than just this I believe, as there is some codegen error that I need to fix, but otherwise it should be fine.

EDIT:
Alternatively, I could just change THIS to emit 'both' default versions if the calling convention of () is the 'default member' (and they are different). I'll end up doing all the infrastructure/codegen fixes that I need to do the MSVC version, but it would permit us to have a rule matching the MSVC version in MSVC mode, and be more conservative for the rest.

Thoughts?

Yeah, it might be reasonable to just do that on targets where the defaults are different (just MSVC, I think?) and otherwise preserve the method's calling convention.

Yeah, it might be reasonable to just do that on targets where the defaults are different (just MSVC, I think?) and otherwise preserve the method's calling convention.

I think it is actually just MSVC-32 bit!

Since the work is 99% the same, I think I should start doing that (emitting BOTH in the case where it matches the default member but NOT the default free function CC) in this review, then do a separate commit for the MSVC compat mode where we emit a version for ALL of the calling conventions.

Thanks for the feedback!

Since the work is 99% the same, I think I should start doing that (emitting BOTH in the case where it matches the default member but NOT the default free function CC) in this review, then do a separate commit for the MSVC compat mode where we emit a version for ALL of the calling conventions.

You'll also need an overload resolution tiebreaker to ensure that +[]{} and the like are not ill-formed due to ambiguity.

Since the work is 99% the same, I think I should start doing that (emitting BOTH in the case where it matches the default member but NOT the default free function CC) in this review, then do a separate commit for the MSVC compat mode where we emit a version for ALL of the calling conventions.

You'll also need an overload resolution tiebreaker to ensure that +[]{} and the like are not ill-formed due to ambiguity.

Right, thats a good point. I'll make sure that is one of my tests. Thanks!

Hmm... so I got distracted the last few days with a handful of small SEMA issues that I believe to be solved, so I'm going back to my codegen issues.

It seems that my problem is that we don't actually mangle calling-convention in a pointer types. The result is:
64 bit: https://godbolt.org/z/nhnoG9
32 bit: https://godbolt.org/z/eK6jha

As you can see, the, only 32 bit-windows actually gets that right in that it differentiates between the two calling-conventions on the operator-func-ptr. The result is the other 3 platforms all get the AST right, but don't generate separate implementations for them. MY implementation is still wrong, since something else odd happens with lambdas on 32 bits, but I'd like to solve these problems as well.

I'll work on the Lambda issue to at least get windows 32 bit right (the only place that will have multiple operator-func-ptr in this case), but I'll eventually need to solve the issue with the differing calling conventions for the MSVC compat implementation that is a follow up to this. Any suggestions on where that could fit in?

Mangling more calling conventions when mangling function types in Itanium (except at the top level) is the right thing to do. There's already a place to do so in the mangling. We just haven't done this yet because a lot of those calling convention are supported by other compilers, so we need to coordinate. So you need to check out what other compilers do (GCC, ICC) and imitate them if they're already setting a reasonable default; otherwise, I think there's a standard algorithm for generating these.

Separately, the MSVC mangler should support Clang's CCs if there's a reasonable extensible rule there. I've never been able to figure out the MVSC mangling grammar.

Mangling more calling conventions when mangling function types in Itanium (except at the top level) is the right thing to do. There's already a place to do so in the mangling. We just haven't done this yet because a lot of those calling convention are supported by other compilers, so we need to coordinate. So you need to check out what other compilers do (GCC, ICC) and imitate them if they're already setting a reasonable default; otherwise, I think there's a standard algorithm for generating these.

Separately, the MSVC mangler should support Clang's CCs if there's a reasonable extensible rule there. I've never been able to figure out the MVSC mangling grammar.

Same here :)

I remember there being places to mangle calling convention, I was just surprised they didn't happen in the function type here. GCC doesn't seem to support any of the calling conventions in 64 bit, but I'll see if ICC has selected a mangling scheme.

Turns out this patch: https://github.com/llvm/llvm-project/commit/2e1e0491b7098fcfe01945e8f62cafe1fcb3cf36 is my problem. The issue has to do deducing a 'local' type in the return type. As a result, we choose to mangle any type 'containing' auto as 'auto'.

So ours demangles as: public: __thiscall `void __cdecl usage(void)'::`1'::<lambda_0>::operator <auto>(void)const
MSVC of course demangles as: public: __thiscall <lambda_a0787c3cd0f2458bc332f24e9b753a51>::operator double (__cdecl*)(int,float,double)(void)const

To me, it seems that the ResultType->getContainedAutoType is perhaps in the wrong here (also of interest, the dyn_cast, for a function that already returns AutoType?)? I'm not sure how to workaround this. I can of course create an awkward reproducer of this (https://godbolt.org/z/KWPTTh), but I don't have a good idea on how to better handle this. @majnemer I see you were the initial committer of this, and would like to see if you have any ideas?

Alright, I have the multi-emit dealt with, and the problems that come with that solved. This doesn't do the MSVC-emit-for-all-CCs thing, but I intend to do that in a separate patch with the same infrastructure.

rjmccall added inline comments.Oct 29 2020, 12:16 PM
clang/include/clang/AST/DeclCXX.h
1013

Probably worth clarifying in the comment which invoker is returned by the no-arguments variant.

clang/lib/AST/DeclCXX.cpp
1550

This seems both rather more complicated and rather more expensive than a simple loop which assigns to a local pointer.

1568

Can you clarify how this method is semantically different from:

assert(MD->getParent() == this);
return isLambda() && MD->getName() == getLambdaStaticInvokerName();

other than probably inadvertently working with a method from a completely different class?

clang/lib/AST/MicrosoftMangle.cpp
2268

So, this is changing the mangling of lambda conversion operators. Is this what MSVC does?

clang/lib/Sema/SemaLambda.cpp
1278

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.

1467

Can we make this call a callback or something rather than returning an array that almost always has one element?

erichkeane added inline comments.Oct 29 2020, 1:07 PM
clang/include/clang/AST/DeclCXX.h
1013

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.

clang/lib/AST/DeclCXX.cpp
1550

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.

clang/lib/AST/MicrosoftMangle.cpp
2268

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

clang/lib/Sema/SemaLambda.cpp
1278

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.

1467

Sure! Can do.

erichkeane added inline comments.Oct 29 2020, 1:23 PM
clang/include/clang/AST/DeclCXX.h
1013

On second thought... I might leave this alone. Otherwise the ItaniumMangler basically needs to reproduce the functionality. I think the improved comment is an appropriate change here.

erichkeane marked an inline comment as done.

@rjmccall : Think I got everything, is this what you mean?

rjmccall accepted this revision.Oct 29 2020, 2:23 PM

Alright, LGTM.

clang/include/clang/AST/DeclCXX.h
1013

Fine by me.

clang/lib/AST/DeclCXX.cpp
1550

I mean that if you really want to assert that, you could do so by checking whether the existing function is the same as what you've already found; you don't need a vector. But just not checking that seems fine to me.

This revision is now accepted and ready to land.Oct 29 2020, 2:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2020, 6:40 AM