paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.88k stars 688 forks source link

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

Open StackOverflowExcept1on opened 1 month ago

StackOverflowExcept1on commented 1 month ago

Is there an existing issue?

Experiencing problems? Have you tried our Stack Exchange first?

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 https://github.com/paritytech/polkadot-sdk/blob/c8d5e5a383c01ee02c3cc49fbd5e07540b6b79cc/substrate/utils/wasm-builder/src/wasm_project.rs#L836-L842

Solution

No response

Are you willing to help with this request?

Maybe (please elaborate above)

bkchr commented 1 month ago

Do you have a little bit more context?

StackOverflowExcept1on commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

bkchr commented 1 month ago

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: https://github.com/paritytech/substrate/pull/13804#issue-1651514636

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