Skip to content

[wasm] Add metadata registration for WebAssembly #66543

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
Jun 20, 2023
Merged

[wasm] Add metadata registration for WebAssembly #66543

merged 1 commit into from
Jun 20, 2023

Conversation

kateinoigakukun
Copy link
Member

This patch adds the metadata registration for the wasm targets, and also adds build support for it.

@kateinoigakukun
Copy link
Member Author

@swift-ci Please smoke test

grynspan
grynspan reviewed Jun 12, 2023
View reviewed changes
@grynspan
Copy link
Contributor

Can anything here be deduped from the ELF implementation? They two implementations are very similar.

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Jun 12, 2023 •
edited
Loading

Good point! I think the list of section names can be shared by X-macro. But I'd like to leave it as is because there is no guarantee that ELF and Wasm use the same section structure and I couldn't figure out good name to express ELF and Wasm things. 😅

@kateinoigakukun kateinoigakukun force-pushed the pr-b1e7535cbac5af8c5e06068a747c1b925d571265 branch from 78958da to 1b75ea8 Compare June 12, 2023 23:13
@kateinoigakukun kateinoigakukun marked this pull request as ready for review June 12, 2023 23:14
@grynspan
Copy link
Contributor

Good point! I think the list of section names can be shared by X-macro. But I'd like to leave it as is because there is no guarantee that ELF and Wasm use the same section structure and I couldn't figure out good name to express ELF and Wasm things. 😅

Maybe just rename the existing ELF file "SwiftRT-ELF-WASM.cpp", and add #if defined(__wasm__) where needed? I think the only spots that would actually need it is where DECLARE_SWIFT_SECTION is declared and where __dso_handle is used. :)

@kateinoigakukun kateinoigakukun force-pushed the pr-b1e7535cbac5af8c5e06068a747c1b925d571265 branch from 1b75ea8 to 7ac6c30 Compare June 19, 2023 06:06
@kateinoigakukun
Copy link
Member Author

@grynspan Thank you for feedback and sorry for my slow response (I was sick last week.. 😵)
I updated my patch to share the most of the content among ELF and Wasm versions :)

@kateinoigakukun
[wasm] Add metadata registration for WebAssembly
8d84a64 
This patch adds the metadata registration for the wasm targets, and also
adds build support for it.
@kateinoigakukun kateinoigakukun force-pushed the pr-b1e7535cbac5af8c5e06068a747c1b925d571265 branch from 7ac6c30 to 8d84a64 Compare June 19, 2023 06:13
@kateinoigakukun
Copy link
Member Author

@swift-ci Please smoke test

@grynspan
Copy link
Contributor

grynspan commented Jun 19, 2023 •
edited
Loading

I'm glad you're feeling better. :)

This looks really good to me! Please be sure to get a review from the core Swift team before merging. :)

al45tair
al45tair approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@al45tair al45tair left a comment

Choose a reason for hiding this comment

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

LGTM.

I wasn't 100% sure about the ELF and WASM versions sharing the .cpp file, but it looks like the implementation is very similar so I can see why you did that.

@kateinoigakukun
Copy link
Member Author

Thank you all for your kind reviews!

@kateinoigakukun kateinoigakukun changed the title [wasm] Add metadata registration SwiftRT-WASM.cpp [wasm] Add metadata registration for WebAssembly Jun 20, 2023
@kateinoigakukun kateinoigakukun merged commit ae0a061 into swiftlang:main Jun 20, 2023
@kateinoigakukun kateinoigakukun deleted the pr-b1e7535cbac5af8c5e06068a747c1b925d571265 branch June 20, 2023 10:52
@grynspan
Copy link
Contributor

grynspan commented Jun 21, 2023 •
edited
Loading

LGTM.

I wasn't 100% sure about the ELF and WASM versions sharing the .cpp file, but it looks like the implementation is very similar so I can see why you did that.

We can actually probably merge the COFF implementation in too, and then this file gets renamed "SwiftRT.cpp" and world peace is achieved.

Edit: Eh, that one's enough different that never mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@al45tair al45tair al45tair approved these changes

@grynspan grynspan grynspan approved these changes

@xedin xedin Awaiting requested review from xedin

@aschwaighofer aschwaighofer Awaiting requested review from aschwaighofer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kateinoigakukun @grynspan @al45tair