Stream: git-wasmtime

Topic: wasmtime / issue #6026 x64: Take SIGFPE signals for divid...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 18:44):

github-actions[bot] commented on issue #6026:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 22:18):

alexcrichton commented on issue #6026:

if you want to branch-fold away the avoid_div_traps=true case as part of this PR

Nah I think it makes sense to go ahead and do that here, and I feel it pretty nicely simplified the rules!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 22:19):

alexcrichton commented on issue #6026:

One minor non-optimal bit about this PR is that currently I've represented the trap as TrapCode inside of the div instructions, but it arguably should be Option<TrapCode> and None for cases such as divide-by-constant-that-isn't-zero. Dealing with Option is a bit of a hassle in ISLE right now though and the only "cost" of this is a slightly larger trap metadata section which doesn't seem like it'll break the bank or cause any major issues in the meantime.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 19:53):

thibaultcha commented on issue #6026:

After upgrading to Wasmtime 8.0.0, how can we revert back to the previous behavior? This patch causes Valgrind to complain about SIGFPE when before 8.0.0 everything was just fine.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 19:57):

thibaultcha edited a comment on issue #6026:

After upgrading to Wasmtime 8.0.0, how can we revert back to the previous behavior? This patch causes Valgrind to complain about SIGFPE when before 8.0.0 everything was just fine. Just to be clear we don't want to raise SIGFPE when Wasmtime encounters a div by 0, we want it to throw a trap as usual and nothing more.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:26):

cfallin commented on issue #6026:

@thibaultcha there isn't really a good way to do that; this PR removed the code-paths that support inline checks rather than relying on signals, because it vastly simplifies things and we judged that it would be fine to use SIGFPE handling instead for the embedding scenarios we support.

It's possible we could revert this whole PR, but that does have a maintenance cost, so it would be useful to know more about the requirements of your case. Is the issue mainly Valgrind warnings (and if so, is there a way to define a suppression for them)? Or is there something more fundamental about your embedding environment where signals are not acceptable?

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 22:18):

alexcrichton commented on issue #6026:

Out of curiosity, if SIGFPE is causing problems how does valgrind handle wasm performing out-of-bounds memory loads? Those are done with guard pages and signal handling right now, so if those are working then I'd expecte SIGFPE to work as well. Or are you perhaps configuring wasm to have no signals by using dynamic memory with explicit bounds checks?

(I'm also curious about what Chris mentioned in terms of suppressions and info about signals, too)

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 22:54):

thibaultcha commented on issue #6026:

It seems we have sorted it out! It wasn't my intention to ask for a revert this patch; it was rather that since I read:

Wasmtime would configure avoid_div_traps=true

I was initially wondering if we as users could revert this setting to get back to the previous behavior and fix our issue.

We found that a Valgrind flag does the trick for this test and allows us to correctly handle SIGFPE with the new cranelift behavior: --vex-iropt-register-updates=allregs-at-each-insn.

Thank you both for the replies!

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 23:19):

cfallin commented on issue #6026:

Great! I'm glad you've got a solution and let us know if you have any other issues.

We're always happy to take PRs to docs if you feel like there's a place where a Valgrind-related tip might have helped you (but no pressure at all, of course); otherwise at least these comments should be searchable if others hit the same thing in the future.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 23:33):

thibaultcha edited a comment on issue #6026:

It seems we have sorted it out! It wasn't my intention to ask for a revert this patch; it was rather that since I read:

Wasmtime would configure avoid_div_traps=true

I was initially wondering if we as users could revert this setting to get back to the previous behavior and fix our issue.

We found that a Valgrind flag does the trick for this test and allows us to correctly handle SIGFPE with the new cranelift behavior: --vex-iropt-register-updates=allregs-at-each-insn.

Thank you both for the replies!

Last updated: Oct 23 2024 at 20:03 UTC