Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763593002:
[wasm][arm] Add an additional stack check for functions with big frames. (Closed)
Description[wasm][arm] Add an additional stack check for functions with big frames.
Stack overflow checks are typically implemented as part of the TurboFan
graph of a function. This means that the stack check code is executed
after frame construction. When a frame is too big, though, there may not
be enough space on the stack anymore to throw the stack overflow
exception after frame construction. With this CL we do an additional
stack check before frame construction for functions with big frames.
As discussed offline with mstarzinger, I do this change currently only
for WebAssembly.
This CL contains only the changes for arm. I will do the other platforms
in separate CLs.
R=mstarzinger@chromium.org, v8-arm-ports@googlegroups.com
Review-Url: https://codereview.chromium.org/2763593002
Cr-Commit-Position: refs/heads/master@{#44065}
Committed: https://chromium.googlesource.com/v8/v8/+/9f01d5c1e0c634a0d6e40a514ac7e49382b5abe3
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments addressed #
Total comments: 3
Patch Set 3 : Load the actual stack limit. #
Total comments: 3
Messages
Total messages: 30 (17 generated)
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. https://codereview.chromium.org/2763593002/diff/1/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2763593002/diff/1/src/runtime/runtime-wasm.cc... src/runtime/runtime-wasm.cc:143: SealHandleScope shs(isolate); nit: The {Isolate::StackOverflow} method creates its own handle scope, not much sense in sealing the current one I think. Can we just drop this? https://codereview.chromium.org/2763593002/diff/1/src/runtime/runtime-wasm.cc... src/runtime/runtime-wasm.cc:144: DCHECK_LE(0, args.length()); nit: DCHECK_EQ? Also, please move the check up to right after the HandleScope, to keep it in sync with other runtime functions.
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2763593002/diff/1/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2763593002/diff/1/src/runtime/runtime-wasm.cc... src/runtime/runtime-wasm.cc:143: SealHandleScope shs(isolate); On 2017/03/20 at 14:54:51, Michael Starzinger wrote: > nit: The {Isolate::StackOverflow} method creates its own handle scope, not much sense in sealing the current one I think. Can we just drop this? I removed the HandleScope and moved the SealHandleScope to line 1. https://codereview.chromium.org/2763593002/diff/1/src/runtime/runtime-wasm.cc... src/runtime/runtime-wasm.cc:144: DCHECK_LE(0, args.length()); On 2017/03/20 at 14:54:51, Michael Starzinger wrote: > nit: DCHECK_EQ? Also, please move the check up to right after the HandleScope, to keep it in sync with other runtime functions. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rodolph.perfetta@gmail.com changed reviewers: + rodolph.perfetta@gmail.com
DBC https://codereview.chromium.org/2763593002/diff/20001/src/compiler/arm/code-g... File src/compiler/arm/code-generator-arm.cc (right): https://codereview.chromium.org/2763593002/diff/20001/src/compiler/arm/code-g... src/compiler/arm/code-generator-arm.cc:2417: __ add(kScratchReg, kScratchReg, kScratchReg holds the address of the stack limit not the stack limit (on ARM the mov doesn't de-reference)
https://codereview.chromium.org/2763593002/diff/20001/src/compiler/arm/code-g... File src/compiler/arm/code-generator-arm.cc (right): https://codereview.chromium.org/2763593002/diff/20001/src/compiler/arm/code-g... src/compiler/arm/code-generator-arm.cc:2417: __ add(kScratchReg, kScratchReg, On 2017/03/20 22:07:17, Rodolph Perfetta wrote: > kScratchReg holds the address of the stack limit not the stack limit (on ARM the > mov doesn't de-reference) Nice catch!
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2763593002/diff/20001/src/compiler/arm/code-g... File src/compiler/arm/code-generator-arm.cc (right): https://codereview.chromium.org/2763593002/diff/20001/src/compiler/arm/code-g... src/compiler/arm/code-generator-arm.cc:2417: __ add(kScratchReg, kScratchReg, On 2017/03/21 at 09:13:50, Michael Starzinger wrote: > On 2017/03/20 22:07:17, Rodolph Perfetta wrote: > > kScratchReg holds the address of the stack limit not the stack limit (on ARM the > > mov doesn't de-reference) > > Nice catch! Fixed. Thanks for catching this one.
On 2017/03/21 at 10:44:40, ahaas wrote: > https://codereview.chromium.org/2763593002/diff/20001/src/compiler/arm/code-g... > File src/compiler/arm/code-generator-arm.cc (right): > > https://codereview.chromium.org/2763593002/diff/20001/src/compiler/arm/code-g... > src/compiler/arm/code-generator-arm.cc:2417: __ add(kScratchReg, kScratchReg, > On 2017/03/21 at 09:13:50, Michael Starzinger wrote: > > On 2017/03/20 22:07:17, Rodolph Perfetta wrote: > > > kScratchReg holds the address of the stack limit not the stack limit (on ARM the > > > mov doesn't de-reference) > > > > Nice catch! > > Fixed. Thanks for catching this one. Rodolph, can you take another look please?
https://codereview.chromium.org/2763593002/diff/40001/src/compiler/arm/code-g... File src/compiler/arm/code-generator-arm.cc (right): https://codereview.chromium.org/2763593002/diff/40001/src/compiler/arm/code-g... src/compiler/arm/code-generator-arm.cc:2446: if (saves_fp != 0) { this code will decrement the stack further, so shouldn't it be taken into account by the code you added above? https://codereview.chromium.org/2763593002/diff/40001/src/compiler/arm/code-g... src/compiler/arm/code-generator-arm.cc:2458: if (saves != 0) { ditto.
https://codereview.chromium.org/2763593002/diff/40001/src/compiler/arm/code-g... File src/compiler/arm/code-generator-arm.cc (right): https://codereview.chromium.org/2763593002/diff/40001/src/compiler/arm/code-g... src/compiler/arm/code-generator-arm.cc:2446: if (saves_fp != 0) { On 2017/03/23 at 13:02:20, Rodolph Perfetta wrote: > this code will decrement the stack further, so shouldn't it be taken into account by the code you added above? Good observation, but I think it does not matter. There is a second stack check happening later at the beginning of the function code. The main reason why I added this stack check is that for some WebAssembly functions pushing the shrink slots got us so far below the stack limit that the other stack check could not call the runtime anymore to throw a StackOverflow exception. If the stack check above succeeds, then pushing some registers here should not push us beyond the buffer that we have below the stack limit.
lgtm
On 2017/03/23 at 14:20:54, rodolph.perfetta wrote: > lgtm Thanks
The CQ bit was checked by ahaas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2763593002/#ps40001 (title: "Load the actual stack limit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490279116987180,
"parent_rev": "03179ab375ff3e1a9869d252a1a86f19563830ec", "commit_rev":
"9f01d5c1e0c634a0d6e40a514ac7e49382b5abe3"}
Message was sent while issue was closed.
Description was changed from ========== [wasm][arm] Add an additional stack check for functions with big frames. Stack overflow checks are typically implemented as part of the TurboFan graph of a function. This means that the stack check code is executed after frame construction. When a frame is too big, though, there may not be enough space on the stack anymore to throw the stack overflow exception after frame construction. With this CL we do an additional stack check before frame construction for functions with big frames. As discussed offline with mstarzinger, I do this change currently only for WebAssembly. This CL contains only the changes for arm. I will do the other platforms in separate CLs. R=mstarzinger@chromium.org, v8-arm-ports@googlegroups.com ========== to ========== [wasm][arm] Add an additional stack check for functions with big frames. Stack overflow checks are typically implemented as part of the TurboFan graph of a function. This means that the stack check code is executed after frame construction. When a frame is too big, though, there may not be enough space on the stack anymore to throw the stack overflow exception after frame construction. With this CL we do an additional stack check before frame construction for functions with big frames. As discussed offline with mstarzinger, I do this change currently only for WebAssembly. This CL contains only the changes for arm. I will do the other platforms in separate CLs. R=mstarzinger@chromium.org, v8-arm-ports@googlegroups.com Review-Url: https://codereview.chromium.org/2763593002 Cr-Commit-Position: refs/heads/master@{#44065} Committed: https://chromium.googlesource.com/v8/v8/+/9f01d5c1e0c634a0d6e40a514ac7e49382b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/9f01d5c1e0c634a0d6e40a514ac7e49382b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
