Page MenuHomePhabricator

Disallow most calling convention attributes on PS4.
ClosedPublic

Authored by Sunil_Srivastava on Jul 15 2019, 4:57 PM.

Details

Summary

We wish to disable most calling convention attributes for PS4, allowing just cdecl (and the equivalent sysv_abi on PS4), which are default.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 4:58 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This has my blessing as PS4 code owner, but I'd like other eyes on it with respect to how we've gone about it.
+ rnk, rjmccall as the most likely suspects.

I'm fine with the idea of targets making unsupported CCs hard errors.

test/Sema/no_callconv.cpp
44 ↗(On Diff #209995)

Please include a newline at the end of this file.

rnk added a comment.Jul 16 2019, 1:24 PM

Some suggestions, you don't have to take all of them.

include/clang/Basic/DiagnosticSemaKinds.td
2961 ↗(On Diff #209995)

This duplication merely to say "not supported" instead of "ignored" seems unfortunate. We could reasonable change "ignored" to "unsupported" and then you could use the warn_cconv_ignored.Text accessor to share the text.

include/clang/Basic/TargetInfo.h
1271 ↗(On Diff #209995)

I feel like perhaps a cleaner way of implementing this would be to have the driver pass down -Werror=unknown-calling-conv (sp) on PS4. That would also allow users to tell clang to ignore their CC annotations by disabling the warning, which could be useful when porting a codebase widely annotated with __stdcall, for example.

lib/Basic/Targets/X86.h
646–647 ↗(On Diff #209995)

For better separation of concerns, I would suggest putting this in PS4OSTargetInfo::checkCallingConvention, so that the main x64 code doesn't have to consider that OS-specific concern.

Sunil_Srivastava marked an inline comment as done.
Sunil_Srivastava added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
2961 ↗(On Diff #209995)

Good point. I will make a pre-commit of just changing the wording (and renaming) of warn_cconv_ignored. Then the warn_conv_unsupported.Text will be used in the actual change.

wristow added a subscriber: wristow.
wristow added inline comments.
include/clang/Basic/TargetInfo.h
1271 ↗(On Diff #209995)

I can see the value of allowing users to ignore the CC annotations to ease a porting task, but we on PS4 prefer not to allow that. We're probably more cautious than most about ABI incompatibilities, so if others decide to restrict use of unsupported CCs, they may very well want to go this route of the driver disabling the CCs. But we'll still make them hard errors on PS4.

aaron.ballman added inline comments.
test/Sema/no_callconv.cpp
1 ↗(On Diff #209995)

Does this really need an svn:executable property on the file?

Sunil_Srivastava marked an inline comment as done.Jul 17 2019, 11:08 AM
Sunil_Srivastava added inline comments.
test/Sema/no_callconv.cpp
1 ↗(On Diff #209995)

I am not sure where that comes from. I did an 'svn add' and 'svn diff'.
Regardless, it will be a new test file with normal lines.

Pre-commit to change the wording and ID of warn_cconv_ignore has gone in in r366368.

I will be shortly updating this review to account for that, and for other points raised here.

  • Adapted suggestion from Ried.
  • Cleaned up line ending and properties of the test case.
Sunil_Srivastava marked an inline comment as done.Jul 18 2019, 2:21 PM
Sunil_Srivastava marked an inline comment as done.
Sunil_Srivastava marked an inline comment as done.Jul 18 2019, 2:25 PM
Sunil_Srivastava marked an inline comment as done.
Sunil_Srivastava added a reviewer: wristow.
aaron.ballman accepted this revision.Jul 19 2019, 8:02 AM

LGTM aside from a nit. You should give other reviewers a chance to sign off in case they have additional comments.

lib/Basic/Targets/OSTargets.h
564 ↗(On Diff #210674)

Line length seems a bit long for coding style requirements.

This revision is now accepted and ready to land.Jul 19 2019, 8:02 AM
Sunil_Srivastava marked an inline comment as done.Jul 19 2019, 9:46 AM
Sunil_Srivastava added inline comments.
lib/Basic/Targets/OSTargets.h
564 ↗(On Diff #210674)

Ooops. You are right. Will split according to clang-format. Would you want me to update the review?

wristow accepted this revision.Jul 19 2019, 11:13 AM

LGTM aside from a nit. You should give other reviewers a chance to sign off in case they have additional comments.

LGTM too, once the line-length is fixed (and IMO, no need to update the review for just that line-length change).

rnk accepted this revision.Jul 19 2019, 11:14 AM

lgtm

Closed by commit rL366617: Disallow most calling convention attributes on PS4 (authored by ssrivastava, committed by ). · Explain WhyJul 19 2019, 2:38 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 2:39 PM