Wasmtime GitHub notifications bot (Apr 17 2024 at 22:11): Wasmtime GitHub notifications bot (Apr 17 2024 at 22:11):jameysharp opened PR #8397 from jameysharp:minimal-probestack to bytecodealliance:main:
When we have
enable_probestackturned on and setprobestack_strategyto "inline", we have to compute how many pages of the stack we'll probe.The current implementation rounds our stack frame size up to the nearest multiple of the page size, then probes each page once.
However, if our stack frame is not a multiple of the page size, that means there's a partial page at the end. It's not necessary to probe that partial page, just like it's unnecessary to probe at all if the frame is smaller than one page. Either way, any signal handler needs to be prepared for stack accesses on that last page to fault at any time during the function's execution.
Wasmtime GitHub notifications bot (Apr 17 2024 at 22:11):jameysharp requested fitzgen for a review on PR #8397.
Wasmtime GitHub notifications bot (Apr 17 2024 at 22:11):jameysharp requested wasmtime-compiler-reviewers for a review on PR #8397.
Wasmtime GitHub notifications bot (Apr 18 2024 at 17:51):fitzgen submitted PR review:
Good find!
Wasmtime GitHub notifications bot (Apr 18 2024 at 17:51):fitzgen submitted PR review:
Good find!
Wasmtime GitHub notifications bot (Apr 18 2024 at 17:51):fitzgen created PR review comment:
Mind leaving a comment about why this doesn't need aligning up here? Kind of unfortunate it would need to be copied to all the other inline probestack code paths. Maybe that is a sign that we should factor this little bit of logic out to a generic helper?
Wasmtime GitHub notifications bot (Dec 12 2024 at 00:32):sunfishcode updated PR #8397.
Wasmtime GitHub notifications bot (Dec 12 2024 at 00:35):sunfishcode submitted PR review.
Wasmtime GitHub notifications bot (Dec 12 2024 at 00:35):sunfishcode created PR review comment:
I've now added comments explaining why we round down. I looked at factoring this code out into a helper, but it felt like it made the code a little less readable, so I didn't add it to the PR, however I'm open to suggestions.
Wasmtime GitHub notifications bot (Dec 12 2024 at 00:37):sunfishcode commented on PR #8397:
@fitzgen I've now rebased this on main, added comments addressing your review feedback above, and ported this patch to the s390x backend. Would you mind taking another look?
Wasmtime GitHub notifications bot (Dec 12 2024 at 00:37):sunfishcode requested fitzgen for a review on PR #8397.
Wasmtime GitHub notifications bot (Dec 12 2024 at 14:13):fitzgen submitted PR review:
Thanks for getting this over the line!
Wasmtime GitHub notifications bot (Dec 12 2024 at 15:20):sunfishcode updated PR #8397.
Wasmtime GitHub notifications bot (Dec 12 2024 at 16:14):sunfishcode merged PR #8397.
Last updated: Dec 23 2024 at 13:07 UTC