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

Add reference type support by default for darwin to support WASI-SDK-25 #3978

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

woodsmc
Copy link
Contributor

@woodsmc woodsmc commented Dec 19, 2024

This resolved the issue WASI-SDK 25 call_indrect doesn't work on Darwin, by turning on reference types by default for iwasm on Darwin.

The error message is triggered by the way call indirect is rendered by WASI-SDK25, which has changed from WASI-SDK24. Turning on the reference checks allows iwasm on Darwin use the WASI-SDK generated code directly.

Updated with additional information

Copy link
Collaborator

@loganek loganek left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@yamt yamt left a comment

Choose a reason for hiding this comment

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

do you know any reasons this should be per-platform?
otherwise i guess it's better to put this in config_common.cmake
as we do for WAMR_BUILD_BULK_MEMORY.

@lum1n0us
Copy link
Collaborator

do you know any reasons this should be per-platform? otherwise i guess it's better to put this in config_common.cmake as we do for WAMR_BUILD_BULK_MEMORY.

completely agree with that. @wenyongh @loganek @tianlong @no1wudi what's your opinion?

Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh
Copy link
Contributor

do you know any reasons this should be per-platform? otherwise i guess it's better to put this in config_common.cmake as we do for WAMR_BUILD_BULK_MEMORY.

completely agree with that. @wenyongh @loganek @tianlong @no1wudi what's your opinion?

Ref-types feature is a relatively complex/big feature and will increase lots of footprint if it is enabled, and it seems that it is seldom used by some platforms, I guess it is better to let the platform's CMakeListx.txt decide how to set it. There were similar changes in PR #3894. If we decide to enable it by default in config_common.cmake, had better revert #3894 also.

@woodsmc
Copy link
Contributor Author

woodsmc commented Dec 21, 2024

I think it would make sense to enable for the desktop development platforms. For new users coming to WAMR seeing the output of RUST or the WASI-SDK run is important for evaluating the platform. For experienced users optimizing for size is key. For the RTOS based platforms I'd guess that these should be built without Ref-types.

Mentioning the options selected during the build process - and the options not selected would be really great improvement.

Additionally we should (I haven't yet) make sure that we can turn off ref-types in the WASI-SDK to support those platforms that don't want this feature.

@loganek
Copy link
Collaborator

loganek commented Dec 21, 2024

Mentioning the options selected during the build process - and the options not selected would be really great improvement.

AFAIK some of the options are printed when generating CMake file, but some of them are not. Also, it'd be good to print the enabled features when user calls iwasm --help or iwasm -v. Would you mind open an issue so we can track the improvement?

@lum1n0us
Copy link
Collaborator

lum1n0us commented Dec 22, 2024

Mentioning the options selected during the build process - and the options not selected would be really great improvement.

AFAIK some of the options are printed when generating CMake file, but some of them are not. Also, it'd be good to print the enabled features when user calls iwasm --help or iwasm -v. Would you mind open an issue so we can track the improvement?

I am creating a new document about which wasm proposals are on-by-default and which are off-by-default. Are you suggesting we should output something like that during cmake configuration and execution by iwasm ?

Might need to add the library case

@lum1n0us
Copy link
Collaborator

I think it would make sense to enable for the desktop development platforms. For new users coming to WAMR seeing the output of RUST or the WASI-SDK run is important for evaluating the platform. For experienced users optimizing for size is key. For the RTOS based platforms I'd guess that these should be built without Ref-types.

In that case, it should include linux, macos and windows, right?

@lum1n0us
Copy link
Collaborator

lum1n0us commented Dec 22, 2024

I am going to merge this PR first and please continue our discussion here.

@lum1n0us lum1n0us merged commit 4cda74a into bytecodealliance:main Dec 22, 2024
383 checks passed
@loganek
Copy link
Collaborator

loganek commented Dec 22, 2024

Are you suggesting we should output something like that during cmake configuration and execution by iwasm ?

Yes, something along those lines.

Additionally we should (I haven't yet) make sure that we can turn off ref-types in the WASI-SDK to support those platforms that don't want this feature.

@woodsmc At least in Rust it's not possible when using "stable" compiler (and I guess it might not be possible for WASI-SDK either, but I'm not 100% sure). Our current approach is to use wasm-opt and it's --disable-reference-types flag, but we're debating in a team about any potential alternatives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants