Skip to content

IRGen: fix swifterror attribute mismatch for WebAssembly #39359

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 1 commit into from
May 28, 2022
Merged

IRGen: fix swifterror attribute mismatch for WebAssembly #39359

merged 1 commit into from
May 28, 2022

Conversation

@MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Sep 18, 2021

On WebAssembly, tail optional arguments are not allowed because Wasm requires callee and caller signature to be the same. So LLVM adds dummy arguments for swiftself and swifterror. If there is swiftself but is no swifterror in a swiftcc function or invocation, then LLVM adds dummy swifterror parameter or argument. To count up how many dummy arguments should be added, we need to mark it as swifterror even though it's not in register.

Related to SR-9307.

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Sep 18, 2021

@kateinoigakukun I'm following your previous changes and comments here. I hope the description makes sense. Do you think there's a way to add any tests for this, or will tests require the driver change to be merged first to enable testing the WASI/Wasm triple?

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Sep 18, 2021

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov marked this pull request as ready for review September 18, 2021 20:32
@MaxDesiatov MaxDesiatov mentioned this pull request Sep 18, 2021
Closed
78 tasks
@kateinoigakukun
Copy link
Member

kateinoigakukun commented Sep 18, 2021 •
edited
Loading

@MaxDesiatov IRGen things can be tested without the driver change. (But we need to fix the current CI failure at first. Now I'm working on it)

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Sep 27, 2021

@kateinoigakukun now that community CI failure is fixed, how do we set IGM.ShouldUseSwiftError to different values for testing? Or having another look at the diff, maybe we should modify isSwiftErrorLoweredInRegister function instead here?

@DougGregor
Copy link
Member

DougGregor commented Dec 10, 2021

@swift-ci please smoke test

@DougGregor
Copy link
Member

DougGregor commented Dec 10, 2021 •
edited
Loading

(EDIT: Re-running Windows tests due to an unrelated error)

DougGregor
DougGregor requested changes Dec 10, 2021
View reviewed changes
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

Can you add an IRGen test to verify that the extra parameter is there for wasm?

lib/IRGen/IRGenModule.cpp
// This mismatch of attributes would be issue when lowering to WebAssembly.
// While lowering, LLVM counts how many dummy params are necessary to match
// callee and caller signature. So we need to add them correctly.
if (functionName == "swift_willThrow") {
Copy link
Member

@DougGregor DougGregor Dec 10, 2021

Choose a reason for hiding this comment

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

Could we model this with a flag in the RuntimeFunctions.def DSL? The specific function name check feels brittle.

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Dec 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This would require some refinement of such DSL, right? Currently it doesn't allow setting attributes on parameters. What would be the best way to specify these attributes in RuntimeFunctions.def then?

Copy link
Member

@DougGregor DougGregor May 24, 2022

Choose a reason for hiding this comment

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

Sorry, I missed your reply in my flood of GitHub notifications. I think we can do this refactoring, but it shouldn't hold up this PR. Re-running testing and switching to "accept", sorry.

@DougGregor
Copy link
Member

DougGregor commented Dec 10, 2021

@swift-ci please test Windows

@DougGregor
Copy link
Member

DougGregor commented May 24, 2022

@swift-ci please test

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented May 24, 2022 •
edited
Loading 

swifterror argument for call has mismatched parameter
%swift.error** %6
  call swiftcc void @swift_willThrow(i8* swiftself undef, %swift.error** noalias nocapture readonly dereferenceable(4) %6) #5
in function $sSYsSeRzSb8RawValueSYRtzrlE4fromxs7Decoder_p_tKcfC

this macOS failure is related I guess?

@kateinoigakukun have you seen this error before?

@kateinoigakukun
Copy link
Member

kateinoigakukun commented May 25, 2022

Hmm weird. I perhaps missed somewhere that calls the function. Let me investigate

@kateinoigakukun
Copy link
Member

kateinoigakukun commented May 28, 2022

@swift-ci please test

@kateinoigakukun
Copy link
Member

kateinoigakukun commented May 28, 2022

@swift-ci please test

@kateinoigakukun
Copy link
Member

kateinoigakukun commented May 28, 2022 •
edited
Loading

OK, I forgot that swifterror is not supported on x86. I've fixed it and CI is now green ✅

@kateinoigakukun kateinoigakukun merged commit e9fb909 into main May 28, 2022
@kateinoigakukun kateinoigakukun deleted the maxd/wasm-swifterror branch May 28, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DougGregor DougGregor DougGregor approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MaxDesiatov @kateinoigakukun @DougGregor