Handling ignored calling conventions

I'm trying to figure out a rational design to solve a pretty big
problem in clang. Right now, clang happily accepts calling
conventions even for architectures that don't support them. For
instance, stdcall on x64. Generally speaking, they get lowered from
clang into LLVM IR during codegen and LLVM simply ignores the problem.
However, this still causes problems such as with name mangling. The
real kicker is: the Win32 APIs have this mismatch *everywhere* and
MSVC will silently ignore nonsense calling conventions in the most
literal sense of the term. Eg)

int __stdcall foo();
int __fastcall foo { return 0; }

On x86, this will err because of the mismatch. On x64, IPF and ARM,
this compiles fine. This is documented behavior on MSDN, FWIW. In
any case, it's causing us fits on Win64 because we can't link against
any of the Win32 APIs thanks to mangling. We mangle as though it's
really stdcall since that's what the attribute says but the OS
libraries mangle as though it were cdecl for x64.

There's been a fair amount of discussion on IRC about the topic, and a
whirlwind of patches from me trying to solve the problem. What I
believe the consensus has been is:

0) We will ignore calling conventions that do not make sense for the
target architecture
1) We want to warn when calling conventions are ignored because of
target differences
  a) But only if the calling convention wouldn't otherwise cause an
error (eg, __stdcall int foo = 12 should still err regardless of
architecture and not warn).
2) When we ignore a calling convention, we modify it to be CC_Default
at the sema level
a) This does cause problems with rewriters since it now has the wrong
spelling, but this is acceptable because we have the same problem with
other attributes

This gives us the desired behavior both in terms of semantics as well
as name mangling, and we generate more sensible code for LLVM.

I am looking for a bit of design help as to how to model this. In my
current incarnation, I am handling all of this in sema by looking at
the target triple and comparing it to the calling convention. But
this feels messy to me. I am thinking that pushing this information
into TargetInfo and subclasses is more sensible, but am looking for
confirmation. Something along the lines of:

virtual bool TargetInfo::AcceptsCallingConvention( CallingConv CC ) const;

So that Sema can just do:

if (!Context.getTargetInfo().AcceptsCallingConvention(CC))
  Ignore = true;

Do you agree with what I think the consensus is? Does this seem like
a reasonable solution to you?

Thanks!

~Aaron

Does that even solve your problem? MIPS does not accept __stdcall.
x64 "accepts" __stdcall but ignores it. How are you differentiating
the cases?

John.

If we want to turn illegal calling conventions into
error/warning/acceptable, then I could use an enum for the return
value of this call in TargetInfo instead of a boolean. This would
provide more flexibility for the various targets to decide what to do
for a given calling convention.

~Aaron

I'm trying to figure out a rational design to solve a pretty big
problem in clang. Right now, clang happily accepts calling
conventions even for architectures that don't support them. For
instance, stdcall on x64. Generally speaking, they get lowered from
clang into LLVM IR during codegen and LLVM simply ignores the problem.
However, this still causes problems such as with name mangling. The
real kicker is: the Win32 APIs have this mismatch *everywhere* and
MSVC will silently ignore nonsense calling conventions in the most
literal sense of the term. Eg)

int __stdcall foo();
int __fastcall foo { return 0; }

On x86, this will err because of the mismatch. On x64, IPF and ARM,
this compiles fine. This is documented behavior on MSDN, FWIW. In
any case, it's causing us fits on Win64 because we can't link against
any of the Win32 APIs thanks to mangling. We mangle as though it's
really stdcall since that's what the attribute says but the OS
libraries mangle as though it were cdecl for x64.

There's been a fair amount of discussion on IRC about the topic, and a
whirlwind of patches from me trying to solve the problem. What I
believe the consensus has been is:

0) We will ignore calling conventions that do not make sense for the
target architecture
1) We want to warn when calling conventions are ignored because of
target differences
a) But only if the calling convention wouldn't otherwise cause an
error (eg, __stdcall int foo = 12 should still err regardless of
architecture and not warn).
2) When we ignore a calling convention, we modify it to be CC_Default
at the sema level
a) This does cause problems with rewriters since it now has the wrong
spelling, but this is acceptable because we have the same problem with
other attributes

This gives us the desired behavior both in terms of semantics as well
as name mangling, and we generate more sensible code for LLVM.

I am looking for a bit of design help as to how to model this. In my
current incarnation, I am handling all of this in sema by looking at
the target triple and comparing it to the calling convention. But
this feels messy to me. I am thinking that pushing this information
into TargetInfo and subclasses is more sensible, but am looking for
confirmation. Something along the lines of:

virtual bool TargetInfo::AcceptsCallingConvention( CallingConv CC ) const;

So that Sema can just do:

if (!Context.getTargetInfo().AcceptsCallingConvention(CC))
Ignore = true;

Pushing this information into TargetInfo makes sense, but I don't think this solves the whole problem. If __stdcall and __fastcall are the same on MSVC's x64, the AcceptsCallingConversion function doesn't give us enough information to tell that. I think what you need is for the target to provide:

  - A mapping from a given calling convention to its canonical calling convention, which establishes identity in the type system
  - The default calling conversion if none is supplied
  - Whether a particular calling conversion is supported or not

Do you agree with what I think the consensus is? Does this seem like
a reasonable solution to you?

Seems like a good direction.

  - Doug

Aaron,

If we want to turn illegal calling conventions into
error/warning/acceptable, then I could use an enum for the return
value of this call in TargetInfo instead of a boolean. This would
provide more flexibility for the various targets to decide what to do
for a given calling convention.

Right now Sema has all the abilities to process attributes on per-target basis.
This this how dllimport / dllexport attributes are handled, for example.

I feel this is the proper place to handle target specific stuff like
calling convention.

Here's the latest iteration of the patch; this time it pushes decision
making down into the TargetInfo subclasses to decide whether a given
calling convention is acceptable, and what the default calling
convention should be.

This patch also updates a lot of the test cases and clarifies the
target architecture for some of them (to ensure we're getting correct
test coverage).

~Aaron

callconv.patch (12.9 KB)

I like the approach on this iteration.

Looks good to me.

Sorry for the delay. A couple points.

First, making Basic depend on an AST header isn't okay; you need
to pull the enum into Basic, probably into Specifiers.h.

Second, the caps convention for method names isLikeThis. It's not
universally followed in clang because we don't like massive restyling
commits, but please don't introduce more counter-convention names.

Third, the boolean value for AcceptsCallConv is backwards.
That's okay given that you want to extend AcceptsCallConv later,
but it suggests that you shouldn't name your method like a
predicate, because I would totally be tempted to do something like:
  if (Target.acceptsCallingConv(...)) {

Fourth, the default for a target should not be to accept all CCs.
It's okay to expect targets to implement this function if they want
to support variant CCs.

Finally, I claim that CC_C should be accepted by all targets.

John.

Here's the latest iteration of the patch; this time it pushes decision
making down into the TargetInfo subclasses to decide whether a given
calling convention is acceptable, and what the default calling
convention should be.

This patch also updates a lot of the test cases and clarifies the
target architecture for some of them (to ensure we're getting correct
test coverage).

Sorry for the delay. A couple points.

Thanks for the review!

First, making Basic depend on an AST header isn't okay; you need
to pull the enum into Basic, probably into Specifiers.h.

This was my biggest concern as well, but I wasn't certain where they
should live. Specifiers.h makes perfect sense.

Second, the caps convention for method names isLikeThis. It's not
universally followed in clang because we don't like massive restyling
commits, but please don't introduce more counter-convention names.

Simple slip of the fingers; good catch.

Third, the boolean value for AcceptsCallConv is backwards.
That's okay given that you want to extend AcceptsCallConv later,
but it suggests that you shouldn't name your method like a
predicate, because I would totally be tempted to do something like:
  if (Target.acceptsCallingConv(...)) {

That's a good point. I've renamed the method to testCallingConvention
which should be a bit more clear. I've kept AcceptsCallConv as the
enumeration name.

Fourth, the default for a target should not be to accept all CCs.
It's okay to expect targets to implement this function if they want
to support variant CCs.

Finally, I claim that CC_C should be accepted by all targets.

Done and done.

I've attached the fixed patch; hopefully it addresses everything! Thanks!

~Aaron

callconv.patch (15.1 KB)

Ping?

Sorry for the continued nit-picking, but checkCallingConvention would be
more standard, and the enum really should get renamed. I would go with
CallingConvCheckResult.

With that, this is approved.

John.

Committed in r165015, thank you for the reviews!

~Aaron