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

cargo-contract 3.1.0 emits sign_ext ops when using rustc 1.69. #1239

Closed
kvinwang opened this issue Jul 27, 2023 · 11 comments · Fixed by #1280
Closed

cargo-contract 3.1.0 emits sign_ext ops when using rustc 1.69. #1239

kvinwang opened this issue Jul 27, 2023 · 11 comments · Fixed by #1280
Labels
bug Something isn't working

Comments

@kvinwang
Copy link
Contributor

kvinwang commented Jul 27, 2023

Cargo-contract says "Build using a version lower than 1.70.0 to make sure your contract will deploy".

However, we have noticed that sign_ext operations appear in the wasm build with cargo-contract-3.1.0+rustc-1.69 and there are no such problems when building with cargo-contract-3.0.1+rustc-1.69.

This might be due to #1189 enabling SignExt for wasm-opt, leading to the emission of sign_ext instructions even if Rustc didn't originally produce them.

Here is the contract.

@kvinwang kvinwang added the bug Something isn't working label Jul 27, 2023
@ascjones
Copy link
Collaborator

This might be due to #1189 enabling SignExt for wasm-opt, leading to the emission of sign_ext instructions even if Rustc didn't originally produce them.

Interesting. Could you confirm this by cloning cargo-contract, removing the .enable_feature(Feature::SignExt) and then building using cargo run -- contract build --release --manifest-path=/path/to/your/contract

Also maybe @brson can confirm whether this is expected behaviour for wasm-opt.

@kvinwang
Copy link
Contributor Author

Interesting. Could you confirm this by cloning cargo-contract, removing the .enable_feature(Feature::SignExt) and then building using cargo run -- contract build --release --manifest-path=/path/to/your/contract

Tried. There is no sign_ext ops when the .enable_feature(Feature::SignExt) was removed.

@ascjones
Copy link
Collaborator

ascjones commented Jul 27, 2023

Thanks @kvinwang this is a bug we need to address. Because of this and other issues related to the toolchain e.g. #1240, we decided to yank 3.1.0.

This will be replaced shortly with a 4.0.0-alpha release for people wanting to use Rust >= 1.70 immediately #1243.

So currently users who want to use toolchains < 1.70 they will have to use 3.0.1. And for users who want to use >= 1.70 they should use 4.0.0-alpha

We may want to make 4.0.0 compatible with both these versions of the toolchains, but that will require a bit more work, therefore the move back to prerelease alpha.

@SkymanOne
Copy link
Contributor

cargo-contract compatibility with different rust toolchains

Rust <= 1.69 + pallet-contracts < polkadot-1.0.0 = cargo-contract 3.0.1
Rust > = 1.70 + pallet-contracts >= polkadot-1.0.0 = cargo-contract 4.0.0-alpha

@dtr0x
Copy link

dtr0x commented Aug 9, 2023

I followed the instructions and switched to using Rust <= 1.69 + pallet-contracts < polkadot-1.0.0 = cargo-contract 3.0.1. This fixes the issue with the wasm build, but now I am getting the error referenced in #1259:

---- flipper::e2e_tests::it_works stdout ----
thread 'flipper::e2e_tests::it_works' panicked at 'We should find a port before the reader ends', ~/.cargo/registry/src/github.com-1ecc6299db9ec823/ink_e2e-4.2.1/src/node_proc.rs:192:10

Therefore I'm not sure if this is related to the present issue or not, I installed the contracts substrate node at v0.27.0 but the e2e test doesn't seem to be able to spin the node up.

@gcp-development
Copy link

@dtr0x , the e2e test does not work after the 3.1.0 has been yanked. It needs to be updated also.

@brson
Copy link
Contributor

brson commented Aug 10, 2023

This might be due to #1189 enabling SignExt for wasm-opt, leading to the emission of sign_ext instructions even if Rustc didn't originally produce them.

Interesting. Could you confirm this by cloning cargo-contract, removing the .enable_feature(Feature::SignExt) and then building using cargo run -- contract build --release --manifest-path=/path/to/your/contract

Also maybe @brson can confirm whether this is expected behaviour for wasm-opt.

@ascjones I have asked @kripken about this. I am not clear on what it means to enable / disable a feature. It might be the case that enabling a feature allows the optimizers to use that feature, which would result in sign extension ops where they didn't exist before. I do think I recall that binaryen features are not about "gating" or disallowing ops in the input modules, but I'm not sure.

@kripken
Copy link

kripken commented Aug 10, 2023

This is expected behavior for wasm-opt. When a feature is enabled, the optimizer will both accept input modules using that feature and also allow itself to emit new instructions that depend on the feature. (For sign extension, it can do things like turn x << 24 >> 24 - two shifts - into a single sign-extend operation from 8 bits to 32.)

@kripken
Copy link

kripken commented Aug 10, 2023

Note btw that wasm-opt has a pass called --signext-lowering:

   --signext-lowering       lower sign-ext operations to
                            wasm mvp and disable the sign
                            extension feature

I think that might solve your issues here? It would remove any signext stuff that rustc emits that you don't want, and avoid adding any more during optimizations.

To use it, enable the feature (so that a wasm with such instructions can load without failing validation) and then run the pass.

@brson
Copy link
Contributor

brson commented Aug 10, 2023

Turning on signext lowering in the wasm_opt crate would look like:

        OptimizationOptions::from(self.optimization_level)
            .mvp_features_only()
            .add_pass(Pass::SignextLowering)
            .zero_filled_memory(true)
            .debug_info(self.keep_debug_symbols)
            .run(dest_wasm, &dest_optimized)?;

This would seemingly get cargo-contract back to a state where it could disable signext opts, as long as all contracts were sanitized through cargo-contract.

@ascjones
Copy link
Collaborator

Thanks for the suggestion @kripken and @brson. PR here #1280

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants