190656 2018-10-16 18:38:09 -0700 useProbeOSRExit causes failures for Win64 DFG JIT 2018-11-01 10:42:52 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ross.kirsling ross.kirsling commit-queue ews-watchlist keith_miller mark.lam msaboff pvollan saam stephan.szabo webkit-bug-importer ysuzuki oldest_to_newest 1469992 0 ross.kirsling 2018-10-16 18:38:09 -0700 About three dozen JSC tests fail for WinCairo in dfg-eager mode only, but all of these pass if useProbeOSRExit is switched off. Two-thirds appear to be failing the `!Heap::heap(value) || Heap::heap(value) == Heap::heap(this)` assert in JSObject::putDirectInternal: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/JSObjectInlines.h#L270 The rest are failing isObject() assertions. --- Here is a simple test case that reproduces the issue (distilled from stress/sparse-map-non-skip.js): test.js ``` function checkGetter(object) { if (object.foo !== 0) throw new Error(`bad value for object.foo! expected 0, found ${object.foo}`); } noInline(checkGetter); for (var i = 0; i < 2305; ++i) checkGetter({ get foo() { return 0; } }); checkGetter({ get foo() { return 0; } }); ``` dfg-whitelist.txt ``` <global>#Cuu2O0 ``` With this as input, `jsc --useConcurrentJIT=false --useProbeOSRExit=true --dfgWhitelist=dfg-whitelist.txt test.js` suffices to repro. Debug: > ASSERTION FAILED: getter.isObject() || setter.isObject() > ...\jit\JITOperations.cpp(1748) : JSC::operationPutGetterSetter > 1 00007FFB3E4321EA > 2 00007FFB2FC42F11 > 3 00007FFB30910027 > 4 000002B0398E2AE0 Release: > Exception: Error: bad value for object.foo! expected 0, found undefined > checkGetter@test.js:3:20 > global code@test.js:10:12 1471014 1 ross.kirsling 2018-10-19 16:02:49 -0700 I'll try to list up what I know so far: - FWIW, testmasm has no failures on WinCairo. - If I dataLog on save and restore in DFGOSREntry.cpp, AssemblyHelpers.h, and DFGOSRExit.cpp (and use printEachOSRExit), it definitely looks like callee-save registers are saved and restored consistently whether or not the probe is used. - If I use dumpDFGDisassembly and blot out all raw hex values (and normalize Structure identifiers), then the output is consistent with or without the probe. There is a final large codeblock which is only spit out in the non-probe case, but that's also true on Mac, which isn't broken. - On the last run of operationPutGetterSetter (in JITOperations.cpp), the encodedGetterValue has an entirely different prefix (0x00007FFB... instead of 0x000001...) from the first 2305 times. If we look at the JSCell for the getter, there is one point of potential note... All cases without probe, first 2305 cases with probe: m_structureID 0x00000104 unsigned int m_indexingTypeAndMisc 0x00 '\0' unsigned char m_type JSFunctionType (0x18 '\x18') JSC::JSType m_flags 0x0e '\xe' unsigned char m_cellState DefinitelyWhite (0x01 '\x1') JSC::CellState Final case, after DFG OSR exit, with probe: m_structureID 0x0000000d unsigned int m_indexingTypeAndMisc 0x18 '\x18' unsigned char m_type CellType (0x00 '\0') JSC::JSType m_flags 0x00 '\0' unsigned char m_cellState PossiblyBlack (0x00 '\0') JSC::CellState ...namely, that 0x18 seems like it's jumped up a slot. But then, everything after it is zeroes, so this may be a red herring...? - One way or another, it's hard to imagine what other than Win64 calling convention could be at play here. Now, although we're saving and restoring RBX, RSI, RDI, R12, R13, R14, and R15, we're intentionally skipping over RBP and RSP -- RBP appears to be callee-save for x86_64 in general, so surely that one's a non-issue(?), but RSP is callee-save for Windows (according to MSDN, although it's not indicated as such in RegisterSet or GPRInfo) so perhaps the probe mucks with the stack pointer in a way that Win64 doesn't like? 1471683 2 stephan.szabo 2018-10-23 15:24:43 -0700 Adding a little bit of information from what was seen today. The value that appears to come through for operationPutGetterSetter's encodedGetterValue seems to be the same value that was used in the prior call to operationPutByIdStrictOptimize's uid (which is also the fifth argument to that call). The timeline with probe on seems to be: operationNewFunction is called and seems to return in rax the value that we would want to use later as encodedGetterValue ctiMasmProbeTrampoline is started - goes into executeProbe and then executeOSRExit and some more before leaving operationPutByIdNonStrictOptimize is called and a value is placed into memory for the uid value. operationPutGetterSetter is called and the value on the stack for the fifth argument has the same value as what was passed to the previous call as uid. 1471701 3 stephan.szabo 2018-10-23 16:55:23 -0700 In the case with probe on, when it goes to set up the arguments for operationPutByIdNonStrictOptimize I seem to see that rsp+20h and rbp-60h [in my test the last 4 digits of rsp were D220 and rbp was D300] are the same location, so setting the argument seems to overwrite the location that the getter was stored in. Later, when the call to operationPutGetterSetter goes to get the value, it reads the overwritten value and then puts that on the stack as the argument value and things fail. With the probe off, at the point the argument is set up, I seem to have values like rsp ending with D2E0 and rbp ending with D3A0, so the two values are in different places. When the call to operationPutGetterSetter goes to get the value it gets the value from operationNewFunction and puts that on the stack as the argument and things work. 1471703 4 stephan.szabo 2018-10-23 16:59:06 -0700 (In reply to Stephan Szabo from comment #3) > In the case with probe on, when it goes to set up the arguments for > operationPutByIdNonStrictOptimize I seem to see that rsp+20h and rbp-60h [in > my test the last 4 digits of rsp were D220 and rbp was D300] are the same Correction D280 and D300 1472514 5 ross.kirsling 2018-10-26 11:33:44 -0700 Update: This definitely is a stack pointer management problem. When the probe is off, RSP always has the same value at the first line of operationPutGetterSetter in JITOperations, whether before DFG entry or after DFG exit: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/JITOperations.cpp#L1742 (* Technically, with or without the probe, RSP is raised by 0x10 just on the single run after calling prepareOSREntry but before actually switching out of baseline JIT. Apparently this isn't problematic though.) When the probe is on, RSP ends up raised by 0x40 after DFG exit. As such, if we hack in a `exitState.stackPointerOffset -= 0x40;` line right before setting RSP in executeOSRExit, the test case succeeds: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/dfg/DFGOSRExit.cpp#L437 1472583 6 ross.kirsling 2018-10-26 14:22:30 -0700 (In reply to Ross Kirsling from comment #5) > As such, if we hack in a `exitState.stackPointerOffset -= 0x40;` line right > before setting RSP in executeOSRExit, the test case succeeds: > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/dfg/ > DFGOSRExit.cpp#L437 To put this last part a bit more clearly, if we add 8 to the variable count returned from the DFG::VariableEventStream::reconstruct call here: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/dfg/DFGOSRExit.cpp#L401 And comment out a sanity-check assert for Probe::Stack::lowWatermark here: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/assembler/ProbeStack.h#L156 Then we finally have a state in which all the failing dfg-eager tests pass. 1472605 7 saam 2018-10-26 15:32:25 -0700 Seems like the probe itself is probably making mistakes on windows ABI 1473114 8 ross.kirsling 2018-10-29 16:40:35 -0700 A bit of progress: The issue doesn't seem to be with the return value of DFG::VariableEventStream::reconstruct, as this is the same regardless of probe and regardless of platform. the issue seems to be that this return value isn't appropriate as a stack pointer offset -- after all, it is simply thrown away in the non-probe path. It appears that if we mimic this line from the non-probe path: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/dfg/DFGOSRExit.cpp#L1379 That is, if we replace numVariables with `codeBlock->jitCode()->dfgCommon()->requiredRegisterCountForExit` on this line: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/dfg/DFGOSRExit.cpp#L402 Then all of the relevant test cases pass (even on Mac!). The catch is that we still need to remove the lowWatermark assert mentioned in my last comment: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/assembler/ProbeStack.h#L156 Not sure at the moment what correction needs to occur here. 1473141 9 353338 ross.kirsling 2018-10-29 17:44:57 -0700 Created attachment 353338 Patch 1473318 10 353338 keith_miller 2018-10-30 11:03:58 -0700 Comment on attachment 353338 Patch r=me. 1473335 11 353338 commit-queue 2018-10-30 11:32:29 -0700 Comment on attachment 353338 Patch Clearing flags on attachment: 353338 Committed r237595: <https://trac.webkit.org/changeset/237595> 1473337 12 commit-queue 2018-10-30 11:32:31 -0700 All reviewed patches have been landed. Closing bug. 1473341 13 webkit-bug-importer 2018-10-30 11:34:02 -0700 <rdar://problem/45675298> 1474161 14 ross.kirsling 2018-11-01 10:42:52 -0700 Ugh, we might not be totally out of the woods yet... While this patch fixed all dfg-eager failures in Debug, apparently four failures remain in Release only: stress/destructuring-assignment-accepts-iterables.js.dfg-eager stress/map-constructor.js.dfg-eager stress/set-constructor.js.dfg-eager stress/weak-map-constructor.js.dfg-eager And sure enough, disabling the probe makes them pass. 353338 2018-10-29 17:44:57 -0700 2018-10-30 11:32:29 -0700 Patch bug-190656-20181029174457.patch text/plain 3883 ross.kirsling U3VidmVyc2lvbiBSZXZpc2lvbjogMjM3NTc1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA2 NTNlNWNkYWE1YTZkOGI2OTNlODA1OTk0NWFlZDMyNTdkYTUyMGQ4Li4yNzc5ZmI4YzU3ZjBkNTU2 OTMxNDFiNmRjYmRkZDI0NmE1NmQ1YTY3IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMSBAQAorMjAxOC0xMC0yOSAgUm9zcyBLaXJzbGluZyAgPHJvc3Mua2lyc2xpbmdAc29u eS5jb20+CisKKyAgICAgICAgdXNlUHJvYmVPU1JFeGl0IGNhdXNlcyBmYWlsdXJlcyBmb3IgV2lu NjQgREZHIEpJVAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTkwNjU2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgKiBhc3NlbWJsZXIvUHJvYmVDb250ZXh0LmNwcDoKKyAgICAgICAgKEpTQzo6UHJvYmU6OmV4 ZWN1dGVQcm9iZSk6CisgICAgICAgIElmIGxvd1dhdGVybWFyayBpcyBleHBlY3RlZCB0byBlcXVh bCBsb3dXYXRlcm1hcmtGcm9tVmlzaXRpbmdEaXJ0eVBhZ2VzICpyZWdhcmRsZXNzKiBvZiB0aGUg aW5wdXQgcGFyYW0sCisgICAgICAgIHRoZW4gbGV0J3MganVzdCBjYWxsIGxvd1dhdGVybWFya0Zy b21WaXNpdGluZ0RpcnR5UGFnZXMgaW5zdGVhZC4KKworICAgICAgICAqIGRmZy9ERkdPU1JFeGl0 LmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpPU1JFeGl0OjpleGVjdXRlT1NSRXhpdCk6CisgICAg ICAgIFRoZSByZXN1bHQgb2YgVmFyaWFibGVFdmVudFN0cmVhbTo6cmVjb25zdHJ1Y3QgYXBwZWFy cyB0byBiZSBpbmFwcHJvcHJpYXRlIGZvciBkaXJlY3QgdXNlIGFzIGEgc3RhY2sgcG9pbnRlciBv ZmZzZXQ7CisgICAgICAgIG1pbWljIHRoZSBub24tcHJvYmUgY2FzZSBhbmQgdXNlIHJlcXVpcmVk UmVnaXN0ZXJDb3VudEZvckV4aXQgZnJvbSBERkdDb21tb25EYXRhIGluc3RlYWQuCisgICAgICAg IChBbHNvLCBzdG9wIHJlZHVuZGFudGx5IHNldHRpbmcgdGhlIHN0YWNrIHBvaW50ZXIgdHdpY2Ug aW4gYSByb3cuKQorCiAyMDE4LTEwLTI5ICBLZWl0aCBNaWxsZXIgIDxrZWl0aF9taWxsZXJAYXBw bGUuY29tPgogCiAgICAgICAgIEpTQyBzaG91bGQgZXhwbGljaXRseSBsaXN0IGl0cyBtb2R1bGVt YXAgZmlsZQpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9Qcm9i ZUNvbnRleHQuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9Qcm9iZUNvbnRl eHQuY3BwCmluZGV4IDJjYTA4NDJkODNiMGVjNWQ0YjM4ZWY3MzQzMmVlOGUyOTllMTJlNzUuLmMx NWEyM2EyNjEwZTRjYjgyMWIxYmQ1Y2MyMzUxNDk5OWI3M2RhYjkgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvUHJvYmVDb250ZXh0LmNwcAorKysgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL1Byb2JlQ29udGV4dC5jcHAKQEAgLTYwLDcgKzYwLDcg QEAgdm9pZCBleGVjdXRlUHJvYmUoU3RhdGUqIHN0YXRlKQogCiAgICAgaWYgKGNvbnRleHQuaGFz V3JpdGVzVG9GbHVzaCgpKSB7CiAgICAgICAgIGNvbnRleHQuc3RhY2soKS5zZXRTYXZlZFN0YWNr UG9pbnRlcihzdGF0ZS0+Y3B1LnNwKCkpOwotICAgICAgICB2b2lkKiBsb3dXYXRlcm1hcmsgPSBj b250ZXh0LnN0YWNrKCkubG93V2F0ZXJtYXJrKHN0YXRlLT5jcHUuc3AoKSk7CisgICAgICAgIHZv aWQqIGxvd1dhdGVybWFyayA9IGNvbnRleHQuc3RhY2soKS5sb3dXYXRlcm1hcmtGcm9tVmlzaXRp bmdEaXJ0eVBhZ2VzKCk7CiAgICAgICAgIHN0YXRlLT5jcHUuc3AoKSA9IHN0ZDo6bWluKGxvd1dh dGVybWFyaywgc3RhdGUtPmNwdS5zcCgpKTsKIAogICAgICAgICBzdGF0ZS0+aW5pdGlhbGl6ZVN0 YWNrRnVuY3Rpb24gPSBmbHVzaERpcnR5U3RhY2tQYWdlczsKZGlmZiAtLWdpdCBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9kZmcvREZHT1NSRXhpdC5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUv ZGZnL0RGR09TUkV4aXQuY3BwCmluZGV4IGIwMjQwMTZjYTRiNmUyZWM5MDgwZmZjNzUzZjQ4ZTll ZTk0NWIzNDIuLjEyYjM0ZTJmNDFiODc5MWFkZGZhMGRhN2E2NGFmOTliMjUwMzg5ZmUgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHT1NSRXhpdC5jcHAKKysrIGIvU291 cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdPU1JFeGl0LmNwcApAQCAtMzk4LDggKzM5OCw4IEBA IHZvaWQgT1NSRXhpdDo6ZXhlY3V0ZU9TUkV4aXQoQ29udGV4dCYgY29udGV4dCkKICAgICAgICAg Ly8gQ29tcHV0ZSB0aGUgdmFsdWUgcmVjb3Zlcmllcy4KICAgICAgICAgT3BlcmFuZHM8VmFsdWVS ZWNvdmVyeT4gb3BlcmFuZHM7CiAgICAgICAgIFZlY3RvcjxVbmRlZmluZWRPcGVyYW5kU3Bhbj4g dW5kZWZpbmVkT3BlcmFuZFNwYW5zOwotICAgICAgICB1bnNpZ25lZCBudW1WYXJpYWJsZXMgPSBk ZmdKSVRDb2RlLT52YXJpYWJsZUV2ZW50U3RyZWFtLnJlY29uc3RydWN0KGNvZGVCbG9jaywgZXhp dC5tX2NvZGVPcmlnaW4sIGRmZ0pJVENvZGUtPm1pbmlmaWVkREZHLCBleGl0Lm1fc3RyZWFtSW5k ZXgsIG9wZXJhbmRzLCAmdW5kZWZpbmVkT3BlcmFuZFNwYW5zKTsKLSAgICAgICAgcHRyZGlmZl90 IHN0YWNrUG9pbnRlck9mZnNldCA9IC1zdGF0aWNfY2FzdDxwdHJkaWZmX3Q+KG51bVZhcmlhYmxl cykgKiBzaXplb2YoUmVnaXN0ZXIpOworICAgICAgICBkZmdKSVRDb2RlLT52YXJpYWJsZUV2ZW50 U3RyZWFtLnJlY29uc3RydWN0KGNvZGVCbG9jaywgZXhpdC5tX2NvZGVPcmlnaW4sIGRmZ0pJVENv ZGUtPm1pbmlmaWVkREZHLCBleGl0Lm1fc3RyZWFtSW5kZXgsIG9wZXJhbmRzLCAmdW5kZWZpbmVk T3BlcmFuZFNwYW5zKTsKKyAgICAgICAgcHRyZGlmZl90IHN0YWNrUG9pbnRlck9mZnNldCA9IC1z dGF0aWNfY2FzdDxwdHJkaWZmX3Q+KGNvZGVCbG9jay0+aml0Q29kZSgpLT5kZmdDb21tb24oKS0+ cmVxdWlyZWRSZWdpc3RlckNvdW50Rm9yRXhpdCkgKiBzaXplb2YoUmVnaXN0ZXIpOwogCiAgICAg ICAgIGV4aXQuZXhpdFN0YXRlID0gYWRvcHRSZWYobmV3IE9TUkV4aXRTdGF0ZShleGl0LCBjb2Rl QmxvY2ssIGJhc2VsaW5lQ29kZUJsb2NrLCBvcGVyYW5kcywgV1RGTW92ZSh1bmRlZmluZWRPcGVy YW5kU3BhbnMpLCByZWNvdmVyeSwgc3RhY2tQb2ludGVyT2Zmc2V0LCBhY3RpdmVUaHJlc2hvbGQs IGFkanVzdGVkVGhyZXNob2xkLCBqdW1wVGFyZ2V0LCBhcnJheVByb2ZpbGUpKTsKIApAQCAtNDQw LDEwICs0NDAsOCBAQCB2b2lkIE9TUkV4aXQ6OmV4ZWN1dGVPU1JFeGl0KENvbnRleHQmIGNvbnRl eHQpCiAgICAgZG8gewogICAgICAgICBhdXRvIGV4dHJhSW5pdGlhbGl6YXRpb25MZXZlbCA9IHN0 YXRpY19jYXN0PEV4dHJhSW5pdGlhbGl6YXRpb25MZXZlbD4oZXhpdFN0YXRlLmV4dHJhSW5pdGlh bGl6YXRpb25MZXZlbCk7CiAKLSAgICAgICAgaWYgKGV4dHJhSW5pdGlhbGl6YXRpb25MZXZlbCA9 PSBFeHRyYUluaXRpYWxpemF0aW9uTGV2ZWw6Ok5vbmUpIHsKLSAgICAgICAgICAgIGNvbnRleHQu c3AoKSA9IGNvbnRleHQuZnA8dWludDhfdCo+KCkgKyBleGl0U3RhdGUuc3RhY2tQb2ludGVyT2Zm c2V0OworICAgICAgICBpZiAoZXh0cmFJbml0aWFsaXphdGlvbkxldmVsID09IEV4dHJhSW5pdGlh bGl6YXRpb25MZXZlbDo6Tm9uZSkKICAgICAgICAgICAgIGJyZWFrOwotICAgICAgICB9CiAKICAg ICAgICAgLy8gQmVnaW4gZXh0cmEgaW5pdGlsaXphdGlvbiBsZXZlbDogU3BlY3VsYXRpb25SZWNv dmVyeQogCg==