This is an archive of the discontinued LLVM Phabricator instance.

Support for MSVS default calling convention options (/Gd, /Gz, /Gv, /Gr)
ClosedPublic

Authored by a.makarov on May 11 2016, 8:40 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

a.makarov retitled this revision from to Support for MSVS default calling convention options (/Gd, /Gz, /Gv, /Gr).
a.makarov updated this object.
a.makarov added a reviewer: rnk.
a.makarov added a subscriber: cfe-commits.
rnk added inline comments.May 11 2016, 8:48 AM
include/clang/Basic/LangOptions.def
220 ↗(On Diff #56918)

Let's get rid of this and have it use a single DefaultCallingConv LangOpt.

include/clang/Driver/CC1Options.td
613 ↗(On Diff #56918)

Let's unify the -cc1 layer with -mrtd, and just have a single -fdefault-calling-convention=stdcall/etc flag.

lib/AST/ASTContext.cpp
8610 ↗(On Diff #56918)

I don't think we need to guard this on MSVCCompat, it'll be unset usually. It will also help us avoid a special path for MRTD.

8616–8619 ↗(On Diff #56918)

Neither fastcall nor stdcall can be applied to variadic functions.

majnemer added inline comments.
lib/AST/ASTContext.cpp
8604–8606 ↗(On Diff #56918)

Do these flags not have an effect on methods?

rnk added inline comments.May 11 2016, 8:56 AM
lib/AST/ASTContext.cpp
8604–8606 ↗(On Diff #56918)

Nope:
https://msdn.microsoft.com/en-us/library/46t77ak2.aspx
"for all functions except C++ member functions..."

majnemer added inline comments.May 11 2016, 8:58 AM
lib/AST/ASTContext.cpp
8604–8606 ↗(On Diff #56918)

Yeah, that's what the documentation says. But do they really have no effect? :)

a.makarov added inline comments.May 12 2016, 5:53 AM
lib/AST/ASTContext.cpp
8604–8606 ↗(On Diff #56918)

Yes, I've checked. Member functions are not affected =)

a.makarov added inline comments.May 12 2016, 7:42 AM
lib/AST/ASTContext.cpp
8616–8619 ↗(On Diff #56918)

As I've got from MSDN, vectorcall and stdcall cannot be applied to vararg functions. Are you sure about fastcall?)

Thanks for the review! I've updated the patch, please take a look.
Modifications:

  • the dependency from MS compatibility mode is removed;
  • the option is renamed into '-fdefault-calling-conv' (since it's not in MS compatibility mode);
  • the '-mrtd' option is now an alias for '-fdefault-calling-conv=stdcall';
  • 'stdcall' is now ignored for variadic functions;
  • since 'vectorcall' requires SSE2 or higher (https://msdn.microsoft.com/en-us/library/dn375768.aspx) the necessary check is added
majnemer added inline comments.May 12 2016, 10:20 AM
include/clang/Basic/LangOptions.def
175 ↗(On Diff #57062)

I'd remove the "MS" from this langopt now that it helps implement -mrtd.

Renamed option 'DefaultMSCallingConv' into 'DefaultCallingConv' and enum 'DefaultMSCallingConvention' into 'DefaultCallingConvention'.

rnk added inline comments.May 13 2016, 5:17 PM
include/clang/Driver/CC1Options.td
615 ↗(On Diff #57071)

Doesn't this have to live in Options.td if you want it to be available in the driver interface? Otherwise it's a -cc1-only flag.

lib/Sema/SemaDeclAttr.cpp
3898 ↗(On Diff #57071)

Use leading upper case variables to achieve consistency with our insanity, please. :)

I've updated the patch. Please take a look.

rnk accepted this revision.May 17 2016, 11:16 AM
rnk edited edge metadata.

Thanks, lgtm!

This revision is now accepted and ready to land.May 17 2016, 11:16 AM
This revision was automatically updated to reflect the committed changes.