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

Investigate Wasmi sign-extension error #968

Closed
jayz22 opened this issue Jul 27, 2023 · 7 comments
Closed

Investigate Wasmi sign-extension error #968

jayz22 opened this issue Jul 27, 2023 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@jayz22
Copy link
Contributor

jayz22 commented Jul 27, 2023

Users are reporting wasmi error about sign extension not being enabled.
https://discord.com/channels/897514728459468821/1133320063995367497/1133320063995367497

@jayz22 jayz22 added the bug Something isn't working label Jul 27, 2023
@jayz22 jayz22 self-assigned this Jul 27, 2023
@ueco-jb
Copy link

ueco-jb commented Jul 28, 2023

I mentioned this on a discord post, but I got this output while trying to optimize contract which fails on allocation (maybe? probably?)

$ soroban contract optimize --wasm pair.wasm
Reading: pair.wasm (55763 bytes)
Writing to: pair.optimized.wasm...
[wasm-validator error in function 87] unexpected false: all used features should be allowed, on
(i32.extend8_s
 (local.get $2)
)

but this tells me nothing, except it may be connected with this error:

error: transaction simulation failed: HostError: Error(WasmVm, InternalError)

Event log (newest first):
   0: [Diagnostic Event] topics:[error, Error(WasmVm, InternalError)], data:"Module(Translation(TranslationError { inner: Validate(BinaryReaderError { inner: BinaryReaderErrorInner { message: "sign extension operations support is not enabled", offset: 28307, needed_hint: None } }) }))"
   1: [Diagnostic Event] topics:[fn_call, Bytes(
...

EDIT: Small update
I got rid of num_bigint and num_traits and they were causing that sign-extension error in my case.

@graydon graydon mentioned this issue Jul 29, 2023
@graydon
Copy link
Contributor

graydon commented Jul 29, 2023

So .. I don't think the allocation failure has anything to do with sign extension. The allocator was broken! I've posted a fix to the SDK in stellar/rs-soroban-sdk#1045 and a test that uses it (along with some budgeting fixes) to the env in #972

But if you're seeing a sign extension error like the one reported from the CLI, I expect this is a CLI issue -- like we're prohibiting sign extension in our use of wasm-validator and we're not supposed to?

@heytdep
Copy link
Contributor

heytdep commented Jul 31, 2023

So .. I don't think the allocation failure has anything to do with sign extension. The allocator was broken! I've posted a fix to the SDK in stellar/rs-soroban-sdk#1045 and a test that uses it (along with some budgeting fixes) to the env in #972

But if you're seeing a sign extension error like the one reported from the CLI, I expect this is a CLI issue -- like we're prohibiting sign extension in our use of wasm-validator and we're not supposed to?

I'm seeing the same error in a cargo test, so I don't think it's a CLI-only issue.

@heytdep
Copy link
Contributor

heytdep commented Jul 31, 2023

It looks like signed extensions are used now also by the previous version WASM binary. I believe it might be something with the rust update that now compiles WASMs that previously weren't using sign extension features to now use it.

In fact my WASM binary now uses i64.extend32_s which is what

i32.wrap/i64
i64.extend_s/i32

did (https://github.com/WebAssembly/spec/blob/main/proposals/sign-extension-ops/Overview.md). I believe that this optimization (one instruction instead of two) was included in the newest rust release, at least for nightly so that's why there are some WASM validation issues now.

If that is the case I believe the feature should be enabled.

@heytdep
Copy link
Contributor

heytdep commented Jul 31, 2023

After taking a look at some old wasm binaries of the same contracts I'm pretty sure that this is the reason some contracts are breaking.

This is how the WAT looked like:

i64.load offset=40
i32.wrap_i64
i64.extend_i32_s

and this is how it looks now using the sign extension feature:

i64.load offset=40
i64.extend32_s

@heytdep
Copy link
Contributor

heytdep commented Jul 31, 2023

As a short term solution, one can compile with

RUSTFLAGS="-C target-cpu=mvp" cargo +nightly build --target wasm32-unknown-unknown --release -Z build-std=std,panic_abort -Z build-std-features=panic_immediate_abort

which seems to compile without sign extension opcodes. But I do not think that it should be a longer term solution. Especially because it seems that there is some sort of LLVM bug which causes RUSTFLAGS="-C target-feature=-sign-ext" to still compile with sign extension opcodes.

@tomerweller
Copy link

Given that rust 1.70 defaults to signed extensions and most WASM runtimes supports these it looks like we should too

graydon added a commit to graydon/rs-soroban-env that referenced this issue Aug 1, 2023
@graydon graydon closed this as completed in c7d0584 Aug 1, 2023
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

No branches or pull requests

5 participants