Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2785723002:
[wasm][arm64] Add an additional stack check for functions with big frames (Closed)
Description[wasm][arm64] Add an additional stack check for functions with big frames
This is the arm64 implementation fo the CL
https://codereview.chromium.org/2763593002
Original message:
[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, rodolph.perfetta@gmail.com, v8-arm-ports@googlegroups.com
Review-Url: https://codereview.chromium.org/2785723002
Cr-Commit-Position: refs/heads/master@{#44792}
Committed: https://chromium.googlesource.com/v8/v8/+/86ba46613354a7cc23a5958340f9982bdb98f352
Patch Set 1 #
Total comments: 2
Patch Set 2 : comments addressed #Messages
Total messages: 24 (13 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.
On 2017/03/29 at 13:58:30, ahaas wrote: > ping.
https://codereview.chromium.org/2785723002/diff/1/src/compiler/arm64/code-gen... File src/compiler/arm64/code-generator-arm64.cc (right): https://codereview.chromium.org/2785723002/diff/1/src/compiler/arm64/code-gen... src/compiler/arm64/code-generator-arm64.cc:1989: __ Mov(jssp, csp); You also need to set jssp as the stack pointer for the assembler: __ SetStackPointer(jssp); then I would add __ AssertStackConsistency(); for good measure. Currently I am wondering how this passes the debug tests: call runtime requires jssp as the assembler stack pointer and asserts for it. So if it passes the test then maybe jssp was already the stack pointer and that doesn't feel right for wasm. Could you add DCHECK(__ StackPointer().Is(csp)); at the beginning of the if block?
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/2785723002/diff/1/src/compiler/arm64/code-gen... File src/compiler/arm64/code-generator-arm64.cc (right): https://codereview.chromium.org/2785723002/diff/1/src/compiler/arm64/code-gen... src/compiler/arm64/code-generator-arm64.cc:1989: __ Mov(jssp, csp); On 2017/04/05 at 15:13:35, Rodolph Perfetta wrote: > You also need to set jssp as the stack pointer for the assembler: > __ SetStackPointer(jssp); > then I would add > __ AssertStackConsistency(); > for good measure. > Currently I am wondering how this passes the debug tests: call runtime requires jssp as the assembler stack pointer and asserts for it. So if it passes the test then maybe jssp was already the stack pointer and that doesn't feel right for wasm. Could you add DCHECK(__ StackPointer().Is(csp)); at the beginning of the if block? Done. I set the stack pointer to the jssp now, and set it back to the csp at the end of this block. I DCHECK in the beginning that the stack pointer is the csp in the beginning. I cannot find where CallRuntime does the "is_jssp" check though.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/10 09:51:22, ahaas wrote: > https://codereview.chromium.org/2785723002/diff/1/src/compiler/arm64/code-gen... > File src/compiler/arm64/code-generator-arm64.cc (right): > > https://codereview.chromium.org/2785723002/diff/1/src/compiler/arm64/code-gen... > src/compiler/arm64/code-generator-arm64.cc:1989: __ Mov(jssp, csp); > On 2017/04/05 at 15:13:35, Rodolph Perfetta wrote: > > You also need to set jssp as the stack pointer for the assembler: > > __ SetStackPointer(jssp); > > then I would add > > __ AssertStackConsistency(); > > for good measure. > > Currently I am wondering how this passes the debug tests: call runtime > requires jssp as the assembler stack pointer and asserts for it. So if it passes > the test then maybe jssp was already the stack pointer and that doesn't feel > right for wasm. Could you add DCHECK(__ StackPointer().Is(csp)); at the > beginning of the if block? > > Done. I set the stack pointer to the jssp now, and set it back to the csp at the > end of this block. I DCHECK in the beginning that the stack pointer is the csp > in the beginning. I cannot find where CallRuntime does the "is_jssp" check > though. You are right. CallRuntime will call CEntryStub and CEntryStub will check the stack is jssp but it is probably already generated and will use a different macro assembler anyway.
lgtm
The CQ bit was checked by ahaas@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
LGTM.
The CQ bit was checked by ahaas@chromium.org
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": 20001, "attempt_start_ts": 1493036561590710,
"parent_rev": "4764cfb017d9ae47526f7453fd88da6ac3e2f421", "commit_rev":
"86ba46613354a7cc23a5958340f9982bdb98f352"}
Message was sent while issue was closed.
Description was changed from ========== [wasm][arm64] Add an additional stack check for functions with big frames This is the arm64 implementation fo the CL https://codereview.chromium.org/2763593002 Original message: [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, rodolph.perfetta@gmail.com, v8-arm-ports@googlegroups.com ========== to ========== [wasm][arm64] Add an additional stack check for functions with big frames This is the arm64 implementation fo the CL https://codereview.chromium.org/2763593002 Original message: [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, rodolph.perfetta@gmail.com, v8-arm-ports@googlegroups.com Review-Url: https://codereview.chromium.org/2785723002 Cr-Commit-Position: refs/heads/master@{#44792} Committed: https://chromium.googlesource.com/v8/v8/+/86ba46613354a7cc23a5958340f9982bdb9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/86ba46613354a7cc23a5958340f9982bdb9... |
