Skip to content

[embedded] Mark all functions as 'nounwind' in embedded Swift, add dependency tests #70344

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 12, 2023
Merged

[embedded] Mark all functions as 'nounwind' in embedded Swift, add dependency tests #70344

merged 7 commits into from
Dec 12, 2023

Conversation

kubamracek
Copy link
Contributor

Currently, some (not all) IRGen'd functions end up emiting unwind info and referencing exception personality symbols, both of which is undesirable in embedded Swift. Let's mark all functions an NoUnwind and let's add a test that checks that a simple embedded Swift program in Mach-O and ELF files has exactly the expected dependencies.

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@rauhul
Copy link
Member

rauhul commented Dec 9, 2023

Nice! I can't wait to drop these hacks!

@_cdecl("__aeabi_unwind_cpp_pr0")
func __unused_0() { fatalError() }

@_cdecl("__aeabi_unwind_cpp_pr1")
func __unused_1() { fatalError() }

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@kubamracek kubamracek added the embedded Embedded Swift label Dec 9, 2023
@kubamracek
Copy link
Contributor Author

@swift-ci please test

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@eeckstein
Copy link
Contributor

eeckstein commented Dec 11, 2023 •
edited
Loading

Maybe a stupid question: what about C++ interop? Do we restrict C++ interop to not supporting exceptions with this?
(cc @zoecarver)

@kubamracek
Copy link
Contributor Author

Changed the condition to only apply 'nounwind' if C++ interop is disabled.

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@hyp
Copy link
Contributor

hyp commented Dec 11, 2023

exceptions can be caught when C++ interop is enabled.

@kubamracek kubamracek requested a review from hyp December 11, 2023 17:15
hyp
hyp reviewed Dec 11, 2023
View reviewed changes
lib/IRGen/IRGenModule.cpp

if (Context.LangOpts.hasFeature(Feature::Embedded) &&
!Context.LangOpts.EnableCXXInterop) {
Attrs.addAttribute(llvm::Attribute::NoUnwind);
Copy link
Contributor

@hyp hyp Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is that applied only to Swift functions, and not the imported C++ functions? In this case you can enable nounwind for Swift functions even when C++ interop is enabled, as exceptions are always caught at the boundary between Swift and a call to C++.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right, this only applies to SILFunctions, not imported C++ functions. @eeckstein is that correct?

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@kubamracek
Copy link
Contributor Author

@swift-ci please test Linux platform

1 similar comment
@kubamracek
Copy link
Contributor Author

@swift-ci please test Linux platform

@kubamracek kubamracek enabled auto-merge December 12, 2023 18:29
@kubamracek kubamracek merged commit ec87fbc into swiftlang:main Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyp hyp hyp left review comments

@phausler phausler Awaiting requested review from phausler

@rauhul rauhul Awaiting requested review from rauhul

@zoecarver zoecarver Awaiting requested review from zoecarver

@aschwaighofer aschwaighofer Awaiting requested review from aschwaighofer

@eeckstein eeckstein Awaiting requested review from eeckstein

Assignees
No one assigned
Labels
embedded Embedded Swift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kubamracek @rauhul @eeckstein @hyp