Wrong comparison instruction used for Generate_PushBoundArguments
Reported by
manjian2...@gmail.com,
Aug 21
|
||||
Issue description
Steps to reproduce the problem:
I can't reproduce this problem, it needs a specify condition.
What is the expected behavior?
Don't throw stack overflow error.
What went wrong?
when a v8 thread is created with the following condition:
stack_bottom: 0x800a3930
real_climit: 0x7ffcb28c
jslimit: 0x7ffcb28c
climit: 0x7ffcb28c
__ sub(sp, sp, Operand(r4, LSL, kPointerSizeLog2));
// Check the stack for overflow. We are not trying to catch interruptions
// (i.e. debug break and preemption) here, so check the "real stack
// limit".
__ CompareRoot(sp, Heap::kRealStackLimitRootIndex);
__ b(gt, &done); // Signed comparison.
uses signed compare, for sp is a negative number, and stack limit is a positive number, the signed comparison will get sp < real_stack_limit, and the exception will be thrown.
Did this work before? N/A
Chrome version: 68.0.3440.106 Channel: stable
OS Version: 6.0
Flash Version:
,
Sep 4
,
Sep 5
This indeed seems to be a valid issue. We perform the comparison with Heap::kRealStackLimitRootIndex elsewhere too. There, we either subtract it from SP and use signed comparison, or compare to SP using unsigned comparison.
,
Sep 6
patch d8.cc with the following patch, then run teststackoverflow.js. You can witness
teststackoverflow.js:4: RangeError: Maximum call stack size exceeded
b();
^
RangeError: Maximum call stack size exceeded
at teststackoverflow.js:4:3
at teststackoverflow.js:5:3
,
Sep 6
build d8 with ./tools/dev/v8gen.py android.arm.debug ninja -C out.gn/android.arm.debug/ d8
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/c79fa4187066b5305c43092b22da358daf3e6a6e commit c79fa4187066b5305c43092b22da358daf3e6a6e Author: Yang Guo <yangguo@chromium.org> Date: Thu Sep 06 07:40:23 2018 Use unsigned comparison for stack checks We use signed comparison when we compare the difference between SP and stack limit to the size we are going to push, but need to use unsigned comparison when we compare SP and stack limit directly. R=mvstanton@chromium.org Bug: chromium:876210 Change-Id: I3ca5233677c42aebadb78920592a7c6d8e33a825 Reviewed-on: https://chromium-review.googlesource.com/1206870 Reviewed-by: Michael Stanton <mvstanton@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#55675} [modify] https://crrev.com/c79fa4187066b5305c43092b22da358daf3e6a6e/src/builtins/arm/builtins-arm.cc [modify] https://crrev.com/c79fa4187066b5305c43092b22da358daf3e6a6e/src/builtins/arm64/builtins-arm64.cc [modify] https://crrev.com/c79fa4187066b5305c43092b22da358daf3e6a6e/src/builtins/ia32/builtins-ia32.cc [modify] https://crrev.com/c79fa4187066b5305c43092b22da358daf3e6a6e/src/builtins/mips/builtins-mips.cc [modify] https://crrev.com/c79fa4187066b5305c43092b22da358daf3e6a6e/src/builtins/mips64/builtins-mips64.cc [modify] https://crrev.com/c79fa4187066b5305c43092b22da358daf3e6a6e/src/builtins/x64/builtins-x64.cc
,
Sep 6
Fixed. Thanks for the bug report! |
||||
►
Sign in to add a comment |
||||
Comment 1 by schenney@chromium.org
, Aug 21