Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Add __has_extension(swiftcc) support #85347

Merged

Conversation

kateinoigakukun
Copy link
Member

This patch adds swiftcc feature to check if the target supports Swift calling convention as well as we do for swiftasynccc.

This support is necessary to fix apple/swift#72343

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 15, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 15, 2024

@llvm/pr-subscribers-clang

Author: Yuta Saito (kateinoigakukun)

Changes

This patch adds swiftcc feature to check if the target supports Swift calling convention as well as we do for swiftasynccc.

This support is necessary to fix apple/swift#72343


Full diff: https://github.com/llvm/llvm-project/pull/85347.diff

2 Files Affected:

  • (modified) clang/include/clang/Basic/Features.def (+3)
  • (modified) clang/test/Sema/swift-call-conv.c (+6-1)
diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index 7c0755b7318306..81fd14aea9a417 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -102,6 +102,9 @@ FEATURE(memory_sanitizer,
 FEATURE(thread_sanitizer, LangOpts.Sanitize.has(SanitizerKind::Thread))
 FEATURE(dataflow_sanitizer, LangOpts.Sanitize.has(SanitizerKind::DataFlow))
 FEATURE(scudo, LangOpts.Sanitize.hasOneOf(SanitizerKind::Scudo))
+FEATURE(swiftcc,
+  PP.getTargetInfo().checkCallingConvention(CC_Swift) ==
+  clang::TargetInfo::CCCR_OK)
 FEATURE(swiftasynccc,
   PP.getTargetInfo().checkCallingConvention(CC_SwiftAsync) ==
   clang::TargetInfo::CCCR_OK)
diff --git a/clang/test/Sema/swift-call-conv.c b/clang/test/Sema/swift-call-conv.c
index 755c18f5183f85..c40e433ce06b81 100644
--- a/clang/test/Sema/swift-call-conv.c
+++ b/clang/test/Sema/swift-call-conv.c
@@ -1,7 +1,12 @@
 // RUN: %clang_cc1 -triple aarch64-unknown-windows-msvc -fsyntax-only %s -verify
 // RUN: %clang_cc1 -triple thumbv7-unknown-windows-msvc -fsyntax-only %s -verify
 // RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only %s -verify
+// RISC-V does not support swiftcall
+// RUN: %clang_cc1 -triple riscv32-unknown-elf -fsyntax-only %s -verify
 
+#if __has_feature(swiftcc)
 // expected-no-diagnostics
-
+#else
+// expected-warning@+2 {{'__swiftcall__' calling convention is not supported for this target}}
+#endif
 void __attribute__((__swiftcall__)) f(void) {}

Copy link
Collaborator

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a past review (#72159), @AaronBallman noted that the existing use of FEATURE (as in FEATURE(swiftasynccc,...) was not the right qualifier to use. And it should have been an EXTENSION instead. We should use EXTENSION for the swiftcc check.

Quoting from the top of the file:

// FEATURE(...) should be used to advertise support for standard language       
// features, whereas EXTENSION(...) should be used for clang extensions. Note   
// that many of the identifiers in this file don't follow this rule for backward
// compatibility reasons.

@@ -102,6 +102,9 @@ FEATURE(memory_sanitizer,
FEATURE(thread_sanitizer, LangOpts.Sanitize.has(SanitizerKind::Thread))
FEATURE(dataflow_sanitizer, LangOpts.Sanitize.has(SanitizerKind::DataFlow))
FEATURE(scudo, LangOpts.Sanitize.hasOneOf(SanitizerKind::Scudo))
FEATURE(swiftcc,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use EXTENSION instead of FEATURE.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kateinoigakukun kateinoigakukun force-pushed the katei/add-swiftcc-feature-upstream branch from 65c4b28 to 50da3b8 Compare March 15, 2024 16:27
@kateinoigakukun kateinoigakukun changed the title [clang] Add __has_feature(swiftcc) support [clang] Add __has_extension(swiftcc) support Mar 15, 2024
@kateinoigakukun
Copy link
Member Author

Oh, I didn't see the past discussion. Thanks!

@kateinoigakukun kateinoigakukun force-pushed the katei/add-swiftcc-feature-upstream branch from 50da3b8 to f9b6687 Compare March 17, 2024 10:31
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy enough with the changes so long as Swift folks are okay with the change in behavior for existing code. CC @compnerd @rjmccall for opinions (I'll leave the approval to one of them).

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's fine with me.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to have a test for swiftasynccc as well.

This patch adds `swiftcc` extension to check if the target supports Swift
calling convention as well as we do for `swiftasynccc`.
Also `swiftasynccc` is now considered to be a Clang extension rather than a
language standard feature to reflect the nature of the attribute.
@AaronBallman
Copy link
Collaborator

Do you need someone to commit this on your behalf?

@kateinoigakukun kateinoigakukun merged commit 679e594 into llvm:main Mar 20, 2024
5 checks passed
@kateinoigakukun kateinoigakukun deleted the katei/add-swiftcc-feature-upstream branch March 20, 2024 23:04
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embedded wasm32-unknown-none-wasm incorrectly calls class destructor with C_CC
6 participants