[wasm][jspi][win] Update TEB on stack switch [v8/v8 : main]
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
PTAL. I haven't been able to confirm that this was the root cause of the linked issue, but I suspect that this could prevent us from handling crashes gracefully on Windows. Generally speaking, I think it's a good idea to keep these limits synced with the actual stack state since we don't know exactly how Windows might use them. This would normally be done by the system's threads API or by the Fibers API, but since we are switching more "manually" we have to update these limits ourselves.
One example I found of how Windows uses these limits is to perform stack checks on large frames. For native code we usually switch the central stack so this shouldn't be an issue, but for fast C calls we stay on the secondary stacks, so that could be one case where a mismatching stack limit would cause a crash.
Related details
- Jakob Kummerow
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
PTAL. I haven't been able to confirm that this was the root cause of the linked issue, but I suspect that this could prevent us from handling crashes gracefully on Windows. Generally speaking, I think it's a good idea to keep these limits synced with the actual stack state since we don't know exactly how Windows might use them. This would normally be done by the system's threads API or by the Fibers API, but since we are switching more "manually" we have to update these limits ourselves.
One example I found of how Windows uses these limits is to perform stack checks on large frames. For native code we usually switch the central stack so this shouldn't be an issue, but for fast C calls we stay on the secondary stacks, so that could be one case where a mismatching stack limit would cause a crash.
PS: Michael for src/base/platform, Jakob for the rest. Thanks!
Related details
- Jakob Kummerow
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
2 comments
DCHECK(thread_stack_limit == nullptr || new_limit <= thread_stack_limit);Pointer comparisons are UB unless both pointers point into the same array. Please cast to `Address` before comparing numerical pointer values.
Related details
- Michael Lippautz
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
1 comment
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thibaud Michaud
| Commit-Queue | +2 |
1 comment
DCHECK(thread_stack_limit == nullptr || new_limit <= thread_stack_limit);Pointer comparisons are UB unless both pointers point into the same array. Please cast to `Address` before comparing numerical pointer values.
Done (casting to uintptr_t, Address is not declared in this scope. And casting the rhs only, the lhs has an implicit conversion operator to uintptr_t).
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/base/platform/platform-win32.cc
Insertions: 2, Deletions: 1.
@@ -1907,7 +1907,8 @@
void Stack::SaveStackLimit() {
Stack::StackSlot new_limit = ObtainCurrentThreadStackLimit();
// The stack limit can only move down.
- DCHECK(thread_stack_limit == nullptr || new_limit <= thread_stack_limit);
+ DCHECK(thread_stack_limit == nullptr ||
+ new_limit <= reinterpret_cast<uintptr_t>(thread_stack_limit));
thread_stack_limit = new_limit;
}
```
Change information
[wasm][jspi][win] Update TEB on stack switch
This is a tentative fix for issue 494341343. On Windows, the TEB
structure describes the state of the current thread, and in particular
it contains the bounds of the stack: StackBase and StackLimit. We don't
currently update these values on stack switch, and running with a stack
pointer outside of the range could lead to unexpected crashes.
Unlike StackBase, StackLimit changes during execution: it is the limit
of the *committed* stack space and moves down as the native stack grows.
Its value is saved and restored in thread local field as we switch to
and from the native stack.
- M src/base/platform/platform-posix.cc
- M src/base/platform/platform-win32.cc
- M src/base/platform/platform.h
- M src/execution/isolate.cc
- M src/wasm/stacks.cc
- M src/wasm/stacks.h
- M src/wasm/turboshaft-graph-interface-inl.h
- M src/wasm/wasm-external-refs.cc
Code-Review: +1 by Jakob Kummerow, +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Niklas Wenzel (Gerrit)
Niklas Wenzel added 1 comment![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |