126487 2014-01-04 00:17:47 -0800 CStack: The fixed up jsStackLimit in doCallToJavaScript should not exceed the native stack limit. 2014-01-06 12:21:13 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 126320 1 msaboff msaboff mark.lam oldest_to_newest 964382 0 msaboff 2014-01-04 00:17:47 -0800 When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range. Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check(). Therefore the ASSERT ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack))); is not valid. 964383 1 220369 msaboff 2014-01-04 00:22:42 -0800 Created attachment 220369 Patch 964400 2 mark.lam 2014-01-04 07:19:00 -0800 (In reply to comment #0) > When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range. Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check(). Therefore the ASSERT > ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack))); > is not valid. I disagree. In llint_stack_check, we do: exec = exec->callerFrame(); vm.topCallFrame = exec; before we instantiate "Interpreter::ErrorHandlingMode mode(exec);” which checks the assertion. Hence, we’re asserting that the caller frame should always be within the bounds of topOfStack as computed by vm.topCallFrame->topOfFrame(). There’s no reason why the caller frame should be on the stack already and exceeding the topOfStack. There is one possible scenario that I can see violating this assertion, and that is if the caller frame is a VMEntrySentinelFrame. If so, then I think we’ll have other problems from using it as the ExecState for the subsequent throw operations to follow. Can you please share the test case that led to this assertion failing? I would like to investigate this further. I don’t think removing the assertion is the right thing to do here. 964406 3 msaboff 2014-01-04 09:09:45 -0800 (In reply to comment #2) > (In reply to comment #0) > > When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range. Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check(). Therefore the ASSERT > > ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack))); > > is not valid. > > I disagree. In llint_stack_check, we do: > > exec = exec->callerFrame(); > vm.topCallFrame = exec; > > before we instantiate "Interpreter::ErrorHandlingMode mode(exec);” which checks the assertion. Hence, we’re asserting that the caller frame should always be within the bounds of topOfStack as computed by vm.topCallFrame->topOfFrame(). There’s no reason why the caller frame should be on the stack already and exceeding the topOfStack. > > There is one possible scenario that I can see violating this assertion, and that is if the caller frame is a VMEntrySentinelFrame. If so, then I think we’ll have other problems from using it as the ExecState for the subsequent throw operations to follow. > > Can you please share the test case that led to this assertion failing? I would like to investigate this further. I don’t think removing the assertion is the right thing to do here. As I said in the description, we exceed the bounds of the stack because the CodeBlock has been set for the callFrame. In llint_stack_check, we check if the frame pointer, which is in m_vm.topCallFrame, is within the stack bounds. The ASSERT in question though checks that m_vm.topCallFrame->topOfFrame(), which includes any locals for m_vm.topCallFrame is in bounds. The failure I was seeing is that the frame pointer is exactly at the edge of the stack bounds, but with the locals it is over the bounds. This was one of the recursive tests in testapi. 964456 4 mark.lam 2014-01-04 22:48:05 -0800 (In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #0) > > > When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range. Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check(). Therefore the ASSERT > > > ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack))); > > > is not valid. > > > > I disagree. In llint_stack_check, we do: > > > > exec = exec->callerFrame(); > > vm.topCallFrame = exec; > > > > before we instantiate "Interpreter::ErrorHandlingMode mode(exec);” which checks the assertion. Hence, we’re asserting that the caller frame should always be within the bounds of topOfStack as computed by vm.topCallFrame->topOfFrame(). There’s no reason why the caller frame should be on the stack already and exceeding the topOfStack. > > As I said in the description, we exceed the bounds of the stack because the CodeBlock has been set for the callFrame. In llint_stack_check, we check if the frame pointer, which is in m_vm.topCallFrame, is within the stack bounds. The ASSERT in question though checks that m_vm.topCallFrame->topOfFrame(), which includes any locals for m_vm.topCallFrame is in bounds. Indeed. But as I pointed out, we’re checking the assertion against the caller frame, not the new frame that we’re running functionInitialization() on. The caller frame should have been stack checked previously and passed the stack check. Hence, the assertion should not be failing now. The assertion failure implies that something has gone awry, and that we should investigate it. > The failure I was seeing is that the frame pointer is exactly at the edge of the stack bounds, but with the locals it is over the bounds. This was one of the recursive tests in testapi. I don’t see this assertion failure when I run testapi with a x86_64 debug build on Mavericks. Are you seeing this as part of the work on the ARM64 port? 964462 5 msaboff 2014-01-04 23:23:23 -0800 (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (In reply to comment #0) > > > > When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range. Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check(). Therefore the ASSERT > > > > ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack))); > > > > is not valid. > > > > > > I disagree. In llint_stack_check, we do: > > > > > > exec = exec->callerFrame(); > > > vm.topCallFrame = exec; > > > > > > before we instantiate "Interpreter::ErrorHandlingMode mode(exec);” which checks the assertion. Hence, we’re asserting that the caller frame should always be within the bounds of topOfStack as computed by vm.topCallFrame->topOfFrame(). There’s no reason why the caller frame should be on the stack already and exceeding the topOfStack. > > > > As I said in the description, we exceed the bounds of the stack because the CodeBlock has been set for the callFrame. In llint_stack_check, we check if the frame pointer, which is in m_vm.topCallFrame, is within the stack bounds. The ASSERT in question though checks that m_vm.topCallFrame->topOfFrame(), which includes any locals for m_vm.topCallFrame is in bounds. > > Indeed. But as I pointed out, we’re checking the assertion against the caller frame, not the new frame that we’re running functionInitialization() on. The caller frame should have been stack checked previously and passed the stack check. Hence, the assertion should not be failing now. The assertion failure implies that something has gone awry, and that we should investigate it. > > > The failure I was seeing is that the frame pointer is exactly at the edge of the stack bounds, but with the locals it is over the bounds. This was one of the recursive tests in testapi. > > I don’t see this assertion failure when I run testapi with a x86_64 debug build on Mavericks. Are you seeing this as part of the work on the ARM64 port? It fails on ARM64. Try running just testapi LLInt only. Using the command line JSC_useJIT=0 DYLD_FRAMEWORK_PATH=. lldb ./testapi -- testapi.js I get: (lldb) r ... PASS: derivedOnlyDescriptor.value should be 2 and is. PASS: derivedOnlyDescriptor.configurable should be true and is. PASS: derivedOnlyDescriptor.enumerable should be false and is. PASS: undefined instanceof MyObject should be false and is. ASSERTION FAILED: m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack)) /Volumes/Data/src/webkit.cstack/Source/JavaScriptCore/runtime/VMEntryScope.cpp(136) : void JSC::VMEntryScope::updateStackLimits() Process 19432 stopped * thread #1: tid = 0x3ded04, 0x00000001006b1638 JavaScriptCore`WTFCrash + 68 at Assertions.cpp:341, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef) frame #0: 0x00000001006b1638 JavaScriptCore`WTFCrash + 68 at Assertions.cpp:341 (lldb) bt 15 * thread #1: tid = 0x3ded04, 0x00000001006b1638 JavaScriptCore`WTFCrash + 68 at Assertions.cpp:341, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef) * frame #0: 0x00000001006b1638 JavaScriptCore`WTFCrash + 68 at Assertions.cpp:341 frame #1: 0x000000010048d31c JavaScriptCore`JSC::VMEntryScope::updateStackLimits(this=0x000000016fdc7910) + 616 at VMEntryScope.cpp:136 frame #2: 0x0000000100472c7c JavaScriptCore`JSC::JSStack::updateStackLimit(this=0x000000015ee078a8) + 80 at JSStack.cpp:208 frame #3: 0x00000001003924c8 JavaScriptCore`JSC::Interpreter::ErrorHandlingMode::~ErrorHandlingMode(this=0x000000016fcef838) + 144 at Interpreter.cpp:106 frame #4: 0x000000010039242c JavaScriptCore`JSC::Interpreter::ErrorHandlingMode::~ErrorHandlingMode(this=0x000000016fcef838) + 24 at Interpreter.cpp:102 frame #5: 0x00000001004cbfac JavaScriptCore`llint_stack_check(exec=0x000000016fcef900, pc=0x000000015ed08a20) + 240 at LLIntSlowPaths.cpp:472 frame #6: 0x00000001004d6314 JavaScriptCore`llint_function_for_call_prologue + 240 frame #7: 0x00000001004db244 JavaScriptCore`llint_op_call + 184 frame #8: 0x00000001004db244 JavaScriptCore`llint_op_call + 184 frame #9: 0x00000001004db244 JavaScriptCore`llint_op_call + 184 frame #10: 0x00000001004db244 JavaScriptCore`llint_op_call + 184 frame #11: 0x00000001004db244 JavaScriptCore`llint_op_call + 184 frame #12: 0x00000001004db244 JavaScriptCore`llint_op_call + 184 frame #13: 0x00000001004db244 JavaScriptCore`llint_op_call + 184 frame #14: 0x00000001004db244 JavaScriptCore`llint_op_call + 184 964688 6 220369 mark.lam 2014-01-06 11:24:51 -0800 Comment on attachment 220369 Patch The issue is due to a bug in the jsStackLimit fixup code in doCallToJavaScript. Patch coming soon. 964690 7 220443 mark.lam 2014-01-06 11:25:45 -0800 Created attachment 220443 The jsStackLimit fixup in doCallToJavaScript should not exceed the native stack limit. 964691 8 mark.lam 2014-01-06 11:30:06 -0800 The assertion was failing because the jsStackLimit in doCallToJavaScript was erroneously setting the jsStackLimit (lower i.e. allowing more stack space) then the native stack limit. As a result, the caller frame was allowed to be pushed on to the stack. The only reason this did not result in a fault yet was because the push frame was taking up space in the host zone of the stack. Once I fixed the jsStackLimit fix up in doCallToJavaScript to cap it at VM::m_stackLimit, the assertion ceases to fail. The assertion is valid. 964709 9 220443 msaboff 2014-01-06 12:13:37 -0800 Comment on attachment 220443 The jsStackLimit fixup in doCallToJavaScript should not exceed the native stack limit. View in context: https://bugs.webkit.org/attachment.cgi?id=220443&action=review r=me with comment fix. > Source/JavaScriptCore/ChangeLog:11 > + was missing, and resulted in an assertion failure when running testapi Duplicate "was" 964713 10 mark.lam 2014-01-06 12:20:04 -0800 Thanks for the review. Landed in r161361 on the jsCStack branch: <http://trac.webkit.org/r161361>. 220369 2014-01-04 00:22:42 -0800 2014-01-06 11:25:45 -0800 Patch 126487.patch text/plain 1519 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTYxMzA4KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBA CisyMDE0LTAxLTAzICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIENTdGFjayBCcmFuY2g6IEFTU0VSVGluZyB0aGF0IHRoZSBjdXJyZW50IHRvcCBvZiBmcmFt ZSBpcyB3aXRoaW4gdGhlIHN0YWNrIHJhbmdlIGlzIG5vdCB2YWxpZCBpbiBWTUVudHJ5U2NvcGU6 OnVwZGF0ZVN0YWNrTGltaXRzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMjY0ODcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBSZW1vdmVkIHRoZSBpbXByb3BlciBBU1NFUlQuCisKKyAgICAgICAgKiBydW50 aW1lL1ZNRW50cnlTY29wZS5jcHA6CisgICAgICAgIChKU0M6OlZNRW50cnlTY29wZTo6dXBkYXRl U3RhY2tMaW1pdHMpOgorCiAyMDE0LTAxLTAzICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBw bGUuY29tPgogCiAgICAgICAgIGNzdGFjayBCcmFuY2g6IEFSTTY0IExMSW50IGZhaWxzIHRvIGNv bXBpbGUgZHVlIHRvIGluc3RydWN0aW9ucyBsaW1pdGF0aW9ucyB3aXRoIHNwCkluZGV4OiBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9WTUVudHJ5U2NvcGUuY3BwCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1ZNRW50cnlTY29wZS5jcHAJKHJldmlzaW9u IDE2MTI5OSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1ZNRW50cnlTY29wZS5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTEzMyw3ICsxMzMsNiBAQCB2b2lkIFZNRW50cnlTY29wZTo6 dXBkYXRlU3RhY2tMaW1pdHMoKQogCiAgICAgLy8gU29tZSBzYW5pdHkgY2hlY2tzIGZvciBvdXIg cG9pbnRlcnMgaW50byB0aGUgc3RhY2s6CiAgICAgQVNTRVJUKG1fdm0uaW50ZXJwcmV0ZXItPnN0 YWNrKCkuY29udGFpbnNBZGRyZXNzKHJlaW50ZXJwcmV0X2Nhc3Q8UmVnaXN0ZXIqPihtX3ZtLnN0 YWNrUG9pbnRlckF0Vk1FbnRyeSkpKTsKLSAgICBBU1NFUlQobV92bS5pbnRlcnByZXRlci0+c3Rh Y2soKS5jb250YWluc0FkZHJlc3MocmVpbnRlcnByZXRfY2FzdDxSZWdpc3Rlcio+KHRvcE9mSlNT dGFjaykpKTsKICAgICBBU1NFUlQoY3VycmVudFN0YWNrUG9pbnRlcigpIDw9IHRvcE9mSlNTdGFj ayk7CiAgICAgQVNTRVJUKHRvcE9mSlNTdGFjayA8PSBtX3ZtLnN0YWNrUG9pbnRlckF0Vk1FbnRy eSk7CiAjZW5kaWYgLy8gIUVOQUJMRShMTElOVF9DX0xPT1ApCg== 220443 2014-01-06 11:25:45 -0800 2014-01-06 12:13:36 -0800 The jsStackLimit fixup in doCallToJavaScript should not exceed the native stack limit. bug-126487.patch text/plain 1764 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTYxMzU1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDE0LTAxLTA2ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBD U3RhY2s6IGpzU3RhY2tMaW1pdCBmaXh1cCBzaG91bGQgbm90IGV4Y2VlZCBuYXRpdmUgc3RhY2sg bGltaXQuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MjY0ODcuCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg SW4gZG9DYWxsVG9KYXZhU2NyaXB0LCB3ZSBmaXh1cCBWTTo6bV9qc1N0YWNrTGltaXQgb25jZSB3 ZSBrbm93IHdoYXQgdGhlCisgICAgICAgIHN0YWNrUG9pbnRlckF0Vk1FbnRyeSBpcy4gSG93ZXZl ciwgd2UgYWxzbyBuZWVkIHRvIGVuc3VyZSB0aGF0IHRoZSBmaXhlZAorICAgICAgICB1cCBqc1N0 YWNrTGltaXQgZG9lcyBub3QgZXhjZWVkIHRoZSBuYXRpdmUgc3RhY2sgbGltaXQuIFRoaXMgY2hl Y2sgd2FzCisgICAgICAgIHdhcyBtaXNzaW5nLCBhbmQgcmVzdWx0ZWQgaW4gYW4gYXNzZXJ0aW9u IGZhaWx1cmUgd2hlbiBydW5uaW5nIHRlc3RhcGkKKyAgICAgICAgb24gYXJjaGl0ZWN0dXJlcyB0 aGF0IGhhdmUgc21hbGxlciBzdGFja3MuIFRoaXMgaXMgbm93IGZpeGVkLgorCisgICAgICAgICog bGxpbnQvTG93TGV2ZWxJbnRlcnByZXRlcjY0LmFzbToKKwogMjAxNC0wMS0wNSAgTWljaGFlbCBT YWJvZmYgIDxtc2Fib2ZmQGFwcGxlLmNvbT4KIAogICAgICAgICBDU3RhY2sgQnJhbmNoOiBBUk02 NCBGaXggb3RoZXIgcHJvbG9ndWUgYW5kIGVwaWxvZ3VlIG1hY3JvcyBpbiBMTEludApJbmRleDog U291cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0ZXI2NC5hc20KPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2xsaW50L0xvd0xldmVsSW50ZXJwcmV0 ZXI2NC5hc20JKHJldmlzaW9uIDE2MTM1NCkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9sbGlu dC9Mb3dMZXZlbEludGVycHJldGVyNjQuYXNtCSh3b3JraW5nIGNvcHkpCkBAIC0xMjgsNiArMTI4 LDEyIEBAIG1hY3JvIGRvQ2FsbFRvSmF2YVNjcmlwdChtYWtlQ2FsbCkKICAgICBtb3ZlIHNwLCB0 ZW1wMgogICAgIHN1YnAgVk06OnN0YWNrUG9pbnRlckF0Vk1FbnRyeVt2bV0sIHRlbXAyLCB0ZW1w MgogICAgIHN1YnAgVk06Om1fanNTdGFja0xpbWl0W3ZtXSwgdGVtcDIsIHRlbXAyCitpZiBDX0xP T1AKK2Vsc2UKKyAgICBicGFlcSB0ZW1wMiwgVk06Om1fc3RhY2tMaW1pdFt2bV0sIC5ub05lZWRU b0NhcEpTU3RhY2tUb05hdGl2ZVN0YWNrTGltaXQKKyAgICBsb2FkcCBWTTo6bV9zdGFja0xpbWl0 W3ZtXSwgdGVtcDIKKy5ub05lZWRUb0NhcEpTU3RhY2tUb05hdGl2ZVN0YWNrTGltaXQ6CitlbmQK ICAgICBzdG9yZXAgdGVtcDIsIFZNOjptX2pzU3RhY2tMaW1pdFt2bV0KICAgICBtb3ZlIHNwLCB0 ZW1wMgogICAgIHN0b3JlcCB0ZW1wMiwgVk06OnN0YWNrUG9pbnRlckF0Vk1FbnRyeVt2bV0K