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

Show weirdness of Windows on CI #9688

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexcrichton
Copy link
Member

Work-in-progress while I get to this to the point where I can share with folks. Not intended for merge

@alexcrichton
Copy link
Member Author

@jsturtevant if you don't mind and if you're willing we could use some more Windows-specific advice/help on this. For background on this I was working on #9675 recently and was having a tough time landing it as it was consistently failing on CI. The failure was only on MinGW and not on other Windows targets (e.g. MSVC) or other platforms (e.g. macOS or Linux). The actual PR itself turned out to be largely unrelated to the crashes on MinGW since the reproduction was quite simple.

I've setup the PR here to showcase what's going on. Specifically we have a bit of C code in a helpers.c file around calling setjmp. We can't call setjmp from Rust as the Rust compiler has no concept of a function that returns twice (e.g. setjmp), so we have to rely on C for this. The current version I linked here is the one with the workaround I landed in #9675. Basically we want to write a function that looks like:

  platform_jmp_buf buf;
  if (platform_setjmp(buf) != 0) {
    return false;
  }
  *buf_storage = &buf;
  return body(payload, callee);

but this crashes on MinGW.

I've arranged a number of CI jobs here to showcase the various results of what's happening. Each job is run in release/debug mode to show the effect of a "fix" with/without optimizations. Each CI job is running cargo run test.wat which is executing the test.wat file in this PR in the CLI, basically just instantiating it. The test.wat file looks like:

(module (func unreachable) (start 0))

so a small wasm module that traps on instantiation. The longjmp in our trap handler (as unreachable compiles to ud2 so we execute a vectored exception handling on Windows) is what's crashing.

The results of CI look like:

build mode -DGOOD -DBAD -DBAD_V1 -DBAD -DBAD_V2
debug
release

For a bit easier to digest this is what we have for diffs:

-DBAD_V1

diff --git a/crates/wasmtime/src/runtime/vm/helpers.c b/crates/wasmtime/src/runtime/vm/helpers.c
index 84711450a..58ffea805 100644
--- a/crates/wasmtime/src/runtime/vm/helpers.c
+++ b/crates/wasmtime/src/runtime/vm/helpers.c
@@ -110,16 +110,16 @@ static bool wasmtime_setjmp_inverted(void **buf_storage,
                                      void *payload, void *callee) {
   platform_jmp_buf buf;
   if (platform_setjmp(buf) != 0) {
-    return true;
+    return false;
   }
   *buf_storage = &buf;
-  return !body(payload, callee);
+  return body(payload, callee);
 }

 bool VERSIONED_SYMBOL(wasmtime_setjmp)(void **buf_storage,
                                        bool (*body)(void *, void *),
                                        void *payload, void *callee) {
-  return !wasmtime_setjmp_inverted(buf_storage, body, payload, callee);
+  return wasmtime_setjmp_inverted(buf_storage, body, payload, callee);
 }

 void VERSIONED_SYMBOL(wasmtime_longjmp)(void *JmpBuf) {

-DBAD_V2

diff --git a/crates/wasmtime/src/runtime/vm/helpers.c b/crates/wasmtime/src/runtime/vm/helpers.c
index 84711450a..f40745099 100644
--- a/crates/wasmtime/src/runtime/vm/helpers.c
+++ b/crates/wasmtime/src/runtime/vm/helpers.c
@@ -110,16 +110,17 @@ static bool wasmtime_setjmp_inverted(void **buf_storage,
                                      void *payload, void *callee) {
   platform_jmp_buf buf;
   if (platform_setjmp(buf) != 0) {
-    return true;
+    return false;
   }
   *buf_storage = &buf;
-  return !body(payload, callee);
+  bool ret = body(payload, callee);
+  return ret;
 }

 bool VERSIONED_SYMBOL(wasmtime_setjmp)(void **buf_storage,
                                        bool (*body)(void *, void *),
                                        void *payload, void *callee) {
-  return !wasmtime_setjmp_inverted(buf_storage, body, payload, callee);
+  return wasmtime_setjmp_inverted(buf_storage, body, payload, callee);
 }

 void VERSIONED_SYMBOL(wasmtime_longjmp)(void *JmpBuf) {

the really weird thing is that -DBAD_V2, the only real change is "put the return value in a local variable", causes the test to be "fixed". In release mode this breaks down though.

So overall, is this something you'd be able to help us out debugging? We have a workaround for now so it's not too urgent, but the workaround is pretty wonky and feels quite brittle, so it seems like it'd be best to get a better understanding of it.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant