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

Switch to maintained WASM instrumentation library #829

Closed
Stebalien opened this issue Sep 1, 2022 · 8 comments
Closed

Switch to maintained WASM instrumentation library #829

Stebalien opened this issue Sep 1, 2022 · 8 comments
Assignees
Milestone

Comments

@Stebalien
Copy link
Member

Switch from parity-wasm (now unmaintained paritytech/parity-wasm#334) to wasmparser (https://crates.io/crates/wasmparser) and wasm-encoder (https://crates.io/crates/wasm-encoder).

This means re-implementing our instrumentation logic, unfortunately. But I'd rather not maintain parity-wasm.

See #668, #602.

@Stebalien Stebalien added this to the M2.2 milestone Sep 1, 2022
@Stebalien
Copy link
Member Author

Given that this "works" right now, we can probably target M2.2 here.

@hunjixin
Copy link
Contributor

hunjixin commented Oct 1, 2022

you can assigned this issue to me, i would try to resolve this during isolation

@Stebalien
Copy link
Member Author

Stay sane! And good luck!

@hunjixin
Copy link
Contributor

hunjixin commented Oct 3, 2022

@Stebalien confirm that this stack overflow check work can across multiple contracts?

	instrument_functions(&mut ctx, &mut module_info)?;       //inject calls in  function body but calldirect not check
	thunk::generate_thunks(&mut ctx, &mut module_info)?;   //inject the export function that be called by other actor 

right?

@Stebalien
Copy link
Member Author

We don't track stack height across calls.

The code is written this way because it instruments the code before and after every function call (instrument_functions). However, it can't put any code before or after a call into an exported function because that call comes from outside of the module. So it instead replaces exported functions with "thunks" (small wrapper functions) that run a bit of stack accounting code before calling the actual function.

There is an inter-contract stack limit, but that's a fixed limit of 1025 calls.

@hunjixin
Copy link
Contributor

hunjixin commented Oct 5, 2022

@Stebalien finished a draft version, all units and bench tests have passed. but the performance will definitely be lower than the old version. because wasmpaser/encoder can't directly modify the original file.

many code basically follow, wasmparser parses the [u8] and got a section reader, make a section builder variable, read a elementsfrom the section reader, edits the element, and finally adds the elements to the builder.

@Stebalien
Copy link
Member Author

because wasmpaser/encoder can't directly modify the original file.

parity-wasm doesn't do this either, right? I mean, in both cases, we:

  • Take the original file, parse it into some structured representation.
  • Apply a set of transforms.
  • Write it back.

@Stebalien
Copy link
Member Author

Implemented.

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 a pull request may close this issue.

2 participants