Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335513004:
[Interpreter] Adds stackcheck in InterpreterPushArgsAndCall/Construct builtins. (Closed)
Description[Interpreter] Adds stackcheck in InterpreterPushArgsAndCall/Construct builtins.
In ignition, arguments to function calls and function constructors are
pushed onto the stack before calling the function. It is required to check
that stack does not overflow when pushing the arguments.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/7f3d15aad423aabf2f9116a929c8fd750615610a
Cr-Commit-Position: refs/heads/master@{#39470}
Patch Set 1 #Patch Set 2 : fix for ia32 #
Total comments: 9
Patch Set 3 : Reworked ia32 PushArgs builtin and addressed comments from Ross. #
Total comments: 4
Patch Set 4 : Reverted refactoring in ia32. #
Total comments: 6
Patch Set 5 : Addressed comments from Ross. #
Messages
Total messages: 39 (27 generated)
The CQ bit was checked by mythria@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: Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/14091)
The CQ bit was checked by mythria@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.
mythria@chromium.org changed reviewers: + mstarzinger@chromium.org, rmcilroy@chromium.org
I added a stack check for InterpreterPushArgs builtins. I had to fix a mjsunit test. PTAL. Thanks, Mythri https://codereview.chromium.org/2335513004/diff/20001/test/mjsunit/stack-trac... File test/mjsunit/stack-traces-overflow.js (right): https://codereview.chromium.org/2335513004/diff/20001/test/mjsunit/stack-trac... test/mjsunit/stack-traces-overflow.js:48: (first_frame.indexOf("stack-traces-overflow.js:32:3") > 0) ); Michi, Does full-codegen only check for a stack overflow at the start of function? I tried looking at full-codegen, if I understand correctly it only checks if the number of locals make the stack overflow. Doesn't it also check that temporary values it pushes while evaluating the function does not overflow the stack? I just wanted to be sure, this check is correct.
+ balazs.kilvady@
https://codereview.chromium.org/2335513004/diff/20001/src/builtins/arm/builti... File src/builtins/arm/builtins-arm.cc (right): https://codereview.chromium.org/2335513004/diff/20001/src/builtins/arm/builti... src/builtins/arm/builtins-arm.cc:1235: __ bkpt(0); Comment that this should be unreachable (all arches) https://codereview.chromium.org/2335513004/diff/20001/src/builtins/ia32/built... File src/builtins/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/2335513004/diff/20001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:795: __ Pop(edi); Not really keen on this pop. Could you not just use esi instead of edi above to avoid the extra push? https://codereview.chromium.org/2335513004/diff/20001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:966: __ Pop(edi); Really don't like this - it's impossible to see that these have been pushed since they are pushed as scratch2 / scratch 1. Can you rework to avoid using extra registers, or at least move the pushes to the same function as they are poped https://codereview.chromium.org/2335513004/diff/20001/src/builtins/x64/builti... File src/builtins/x64/builtins-x64.cc (right): https://codereview.chromium.org/2335513004/diff/20001/src/builtins/x64/builti... src/builtins/x64/builtins-x64.cc:808: Register scratch, Register scratch1) { You don't use scratch1 here. (if you do need it, scratch->scratch1 and scratch1->scratch2)
I reworked the ia32 PushArgs builtin and addressed other comments. PTAL. Thanks, Mythri https://codereview.chromium.org/2335513004/diff/20001/src/builtins/arm/builti... File src/builtins/arm/builtins-arm.cc (right): https://codereview.chromium.org/2335513004/diff/20001/src/builtins/arm/builti... src/builtins/arm/builtins-arm.cc:1235: __ bkpt(0); On 2016/09/12 14:43:17, rmcilroy wrote: > Comment that this should be unreachable (all arches) Done. https://codereview.chromium.org/2335513004/diff/20001/src/builtins/ia32/built... File src/builtins/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/2335513004/diff/20001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:795: __ Pop(edi); On 2016/09/12 14:43:17, rmcilroy wrote: > Not really keen on this pop. Could you not just use esi instead of edi above to > avoid the extra push? As discussed offline, we can't use esi, because it holds context. So we can't avoid this pop. https://codereview.chromium.org/2335513004/diff/20001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:966: __ Pop(edi); On 2016/09/12 14:43:17, rmcilroy wrote: > Really don't like this - it's impossible to see that these have been pushed > since they are pushed as scratch2 / scratch 1. Can you rework to avoid using > extra registers, or at least move the pushes to the same function as they are > poped I reworked the code. I don't think we can avoid using registers, but I reworked it so that Pushes and Pops happen in this function. We do not have any free registers. Since we have to pop the return address before we push anything onto the stack, we need some free registers. Two is the minimum we need. In the new code, I use three to make the InterpreterPushArgsCopyArguments loop simpler. I can revert back to the older version of using only two temporary registers. https://codereview.chromium.org/2335513004/diff/20001/src/builtins/x64/builti... File src/builtins/x64/builtins-x64.cc (right): https://codereview.chromium.org/2335513004/diff/20001/src/builtins/x64/builti... src/builtins/x64/builtins-x64.cc:808: Register scratch, Register scratch1) { On 2016/09/12 14:43:17, rmcilroy wrote: > You don't use scratch1 here. (if you do need it, scratch->scratch1 and > scratch1->scratch2) Thanks, I forgot to remove scratch1. Done.
The CQ bit was checked by mythria@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/2335513004/diff/40001/src/builtins/ia32/built... File src/builtins/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/2335513004/diff/40001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:942: (num_slots_above_ret_address)*kPointerSize)); space between binary ops https://codereview.chromium.org/2335513004/diff/40001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:1026: Generate_InterpreterPushArgsCopyArguments(masm, ecx, edx, edi, ebx); As discussed, could we merge these back into the helper again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I merged the helpers back into one helper. PTAL. Thanks, Mythri https://codereview.chromium.org/2335513004/diff/40001/src/builtins/ia32/built... File src/builtins/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/2335513004/diff/40001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:942: (num_slots_above_ret_address)*kPointerSize)); On 2016/09/13 10:13:56, rmcilroy wrote: > space between binary ops Done. https://codereview.chromium.org/2335513004/diff/40001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:1026: Generate_InterpreterPushArgsCopyArguments(masm, ecx, edx, edi, ebx); On 2016/09/13 10:13:56, rmcilroy wrote: > As discussed, could we merge these back into the helper again. Done.
The CQ bit was checked by mythria@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/2335513004/diff/60001/src/builtins/ia32/built... File src/builtins/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/2335513004/diff/60001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:813: // We have to pop return address and the two temporary registers before we please update this comment. https://codereview.chromium.org/2335513004/diff/60001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:839: __ add(scratch1, Immediate(1)); // Add one for receiver. Should this be done unconditionally, or only if receiver_in_args? https://codereview.chromium.org/2335513004/diff/60001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:843: __ j(less_equal, stack_overflow); // Signed comparison. Please just call Generate_StackOverflowCheck (with a parameter for whether the reciever needs to be added).
Thanks Ross, I addressed your comments. PTAL. Thanks, Mythri https://codereview.chromium.org/2335513004/diff/60001/src/builtins/ia32/built... File src/builtins/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/2335513004/diff/60001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:813: // We have to pop return address and the two temporary registers before we On 2016/09/13 17:12:12, rmcilroy wrote: > please update this comment. Done. https://codereview.chromium.org/2335513004/diff/60001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:839: __ add(scratch1, Immediate(1)); // Add one for receiver. On 2016/09/13 17:12:12, rmcilroy wrote: > Should this be done unconditionally, or only if receiver_in_args? This should be done unconditionally. We have reserve a slot for receiver whether or not it is available in the arguments. https://codereview.chromium.org/2335513004/diff/60001/src/builtins/ia32/built... src/builtins/ia32/builtins-ia32.cc:843: __ j(less_equal, stack_overflow); // Signed comparison. On 2016/09/13 17:12:12, rmcilroy wrote: > Please just call Generate_StackOverflowCheck (with a parameter for whether the > reciever needs to be added). Done. Scratch1 already contains the required offset to increment SP. So I did not compute it again here. If it is not clean, we can recompute it here. It costs us 3 extra instructions. (mov scratch1, eax; add scratch1, 1; shl scratch1, kPointerSizeLog2;)
The CQ bit was checked by mythria@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, thanks.
The CQ bit was checked by mythria@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.
The CQ bit was checked by mythria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Adds stackcheck in InterpreterPushArgsAndCall/Construct builtins. In ignition, arguments to function calls and function constructors are pushed onto the stack before calling the function. It is required to check that stack does not overflow when pushing the arguments. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Adds stackcheck in InterpreterPushArgsAndCall/Construct builtins. In ignition, arguments to function calls and function constructors are pushed onto the stack before calling the function. It is required to check that stack does not overflow when pushing the arguments. BUG=v8:4280 LOG=N Committed: https://crrev.com/7f3d15aad423aabf2f9116a929c8fd750615610a Cr-Commit-Position: refs/heads/master@{#39470} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7f3d15aad423aabf2f9116a929c8fd750615610a Cr-Commit-Position: refs/heads/master@{#39470} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
