-
Notifications
You must be signed in to change notification settings - Fork 315
Generate atomic instructions defined in the threads proposal #2386
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
base: main
Are you sure you want to change the base?
Generate atomic instructions defined in the threads proposal #2386
Conversation
|
@abrown Would you have a few minutes to look at this? I'm not up-to-speed yet on shared-everything threads. |
|
FYI, Andrew is on leave right now, so it might be a bit until he'll be able to look at this. (Not providing any timelines here because once he is back, he might have a lot to work through before getting to this for all I know.) |
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Could you also run the validate-valid-module fuzzer locally for a bit to double-check these are correct? That can be done with:
$ FUZZER=validate_valid_module cargo +nightly fuzz run -s none run
If nothing shows up for 10-15m I'd say it's good to go.
| #[inline] | ||
| fn atomic_instruction_valid(module: &Module, _builder: &mut CodeBuilder) -> bool { | ||
| module.config.threads_enabled && module.config.shared_everything_threads_enabled | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this it's ok to drop shared_everything_threads_enabled since these instructions only require the wasm threads proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh ok I see, will do, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alexcrichton , I have run this for 15 minutes and I did not see any issues apart from some warnings at the start which appear on the main branch also:
WARNING: Failed to find function "__sanitizer_acquire_crash_state". Reason dlsym(RTLD_DEFAULT, __sanitizer_acquire_crash_state): symbol not found.
WARNING: Failed to find function "__sanitizer_print_stack_trace". Reason dlsym(RTLD_DEFAULT, __sanitizer_print_stack_trace): symbol not found.
WARNING: Failed to find function "__sanitizer_set_death_callback". Reason dlsym(RTLD_DEFAULT, __sanitizer_set_death_callback): symbol not found.
Just to sanity check that I was running the tool correctly and that I could recognise an error, I also deliberately added a mistake to it and it fell over after a couple of seconds with a very clear and obvious error.
Would you like me to attach the output of the successful run?
This commit adds suppport for emitting atomic instructions when wasm_smith::Config::threads_enabled and wasm_smith::Config::shared_everything_threads_enabled are true. A link to the atomic instructions in the threads proposal: https://webassembly.github.io/threads/core/syntax/instructions.html#syntax-instr-atomic-memory
ae2f630 to
40bf650
Compare
This commit adds suppport for emitting atomic instructions when wasm_smith::Config::threads_enabled and
wasm_smith::Config::shared_everything_threads_enabled are true.
A link to the atomic instructions in the threads proposal: https://webassembly.github.io/threads/core/syntax/instructions.html#syntax-instr-atomic-memory
Addresses #2384