Stream: git-wasmtime

Topic: wasmtime / PR #7470 cranelift: Make inline stackprobe unr...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 12:59):

afonso360 opened PR #7470 from afonso360:unroll-valgrind to bytecodealliance:main:

:wave: Hey,

This PR alters our stackprobe unroll sequences to move the stackpointer before writing to the stack. I don't think there was anything wrong with what we were doing before but it makes valgrind really unhappy. (See #7454)

I've tested this with cg_clif on x86, and it now cleanly passes valgrind for the original reported testcase.

Fixes: #7454
prtest:full

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 12:59):

afonso360 requested fitzgen for a review on PR #7470.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 12:59):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #7470.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 16:29):

fitzgen submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 16:29):

fitzgen submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 16:29):

fitzgen created PR review comment:

Out of curiosity / unrelated to this PR: do you know why it is necessary to probe via storing to the stack rather than loading from the stack? It seems like either would accomplish the task AFAICT, and loads should be cheaper than stores.

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

cfallin submitted PR review.

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

cfallin created PR review comment:

From the point of view of global cost, stores might actually be better, I suspect: if it faults in a new page, a load would cause a read-only mapping to the global zero page to be created, and the first write would then cause another page fault (at least if the stack is an ordinary MAP_ANON region). Or at least, we've seen elsewhere that it's better for first touch to be write when TLB contention is high...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 17:13):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 17:13):

fitzgen created PR review comment:

Ah okay that makes sense. Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 20:59):

afonso360 merged PR #7470.

Last updated: Dec 23 2024 at 13:07 UTC