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

Consider removing useless -sign-ext flag in substrate-wasm-builder #5777

Open
2 tasks done
StackOverflowExcept1on opened this issue Sep 19, 2024 · 5 comments
Open
2 tasks done
Labels
I5-enhancement An additional feature request. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@StackOverflowExcept1on
Copy link

StackOverflowExcept1on commented Sep 19, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Motivation

https://github.com/rust-lang/blog.rust-lang.org/blob/4a72d620768932213baf39b03c6f1e61c00cd038/posts/2024-08-26-webassembly-targets-change-in-default-target-features.md

Request

Should be removed here

let mut rustflags = String::new();
match target {
RuntimeTarget::Wasm => {
rustflags.push_str(
"-C target-cpu=mvp -C target-feature=-sign-ext -C link-arg=--export-table ",
);
},

Solution

No response

Are you willing to help with this request?

Maybe (please elaborate above)

@StackOverflowExcept1on StackOverflowExcept1on added the I5-enhancement An additional feature request. label Sep 19, 2024
@github-actions github-actions bot added the I10-unconfirmed Issue might be valid, but it's not yet known. label Sep 19, 2024
@StackOverflowExcept1on StackOverflowExcept1on changed the title Consider to disable multi-value and reference-types features Consider disabling multi-value and reference-types features Sep 19, 2024
@bkchr
Copy link
Member

bkchr commented Sep 19, 2024

Do you have a little bit more context?

@StackOverflowExcept1on
Copy link
Author

@bkchr Rust 1.82 (stable Oct 17) enables multi-value and reference-types by default. I think it makes sense to additionally disable these features.

@nazar-pc
Copy link
Contributor

According to link you have provided -C target-cpu=mvp is already sufficient to disable all of these features, not sure why Substrate also has -C target-feature=-sign-ext given that same document explicitly mentions that it'll be disabled for mvp CPU.

@StackOverflowExcept1on
Copy link
Author

StackOverflowExcept1on commented Sep 20, 2024

Yep, maybe it make sense just leave -C target-cpu=mvp since Rust added more documentation about this stuff

@StackOverflowExcept1on StackOverflowExcept1on changed the title Consider disabling multi-value and reference-types features Consider removing useless -sign-ext flag in substrate-wasm-builder Sep 20, 2024
@bkchr
Copy link
Member

bkchr commented Sep 22, 2024

not sure why Substrate also has -C target-feature=-sign-ext given that same document explicitly mentions that it'll be disabled for mvp CPU.

Apparently it did not worked before: paritytech/substrate#13804 (comment)

AFAIR we also just disabled this feature because there were issues with our wasm parser and not because we didn't wanted this feature.

CC @koute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

No branches or pull requests

3 participants