141489 2015-02-11 13:36:34 -0800 WebKit crashes at operationPutByIdNonStrictBuildList when logging into http://roxtecbuild.factor10labs.com 2015-04-03 05:17:53 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) iPhone / iPad All RESOLVED FIXED http://roxtecbuild.factor10labs.com InRadar P1 Major --- 143368 1 mmirman mmirman dbates ddkilzer fpizlo ggaren mmirman ossy oldest_to_newest 1068464 0 mmirman 2015-02-11 13:36:34 -0800 Patch forthcoming. rdar://problem/19432916 1070866 1 247009 mmirman 2015-02-20 16:21:46 -0800 Created attachment 247009 removed register r9 from arm callee save register list This should be a conservative change. 1070870 2 247009 fpizlo 2015-02-20 16:27:32 -0800 Comment on attachment 247009 removed register r9 from arm callee save register list View in context: https://bugs.webkit.org/attachment.cgi?id=247009&action=review > Source/JavaScriptCore/ChangeLog:3 > + r9 isn't necessarily a callee save register on ARMv7. So, is it or isn't it? > Source/JavaScriptCore/jit/RegisterSet.cpp:-89 > - result.set(ARMRegisters::r9); Currently, in theory, we could use the callee save list to control two separate things: 1) Deciding which registers don't need to be saved at callsites. 2) Deciding which registers must be saved inside a callee. If it's possible for some of our callers to ever think that r9 is callee-save, then removing it from the list will only introduce more subtle bugs: we will incorrectly fail to save r9 in some cases. So, you need to determine if the iOS ARM64 ABI treat r9 as never-callee-save or sometimes-callee-save. This fix is only correct if the iOS ARM64 ABI treats r9 as never-callee-save. I'd also check if our Linux friends are doing ARM64, and if they are, then you need to CC them. This fix would break them if Linux was r9-always-callee-save or sometimes-callee-save. 1070872 3 ggaren 2015-02-20 16:31:35 -0800 This patch needs a regression test. 1070880 4 247009 fpizlo 2015-02-20 16:38:03 -0800 Comment on attachment 247009 removed register r9 from arm callee save register list I'm going to mark this r- because: 1) As ggaren says, this needs a regression test. 2) If by "isn't necessarily callee-save" you mean that it sometimes might be a callee-save, then this isn't the right fix. It will only introduce other crashes, because in places where we use the callee save list to decide which registers to save inside a callee, this will fail to save r9 and if some callers might expect us to save it them we'll break them. Maybe the right answer is to split the calleeSaveRegisters into a set of things that are guaranteed preserved and a set of things that must be saved. Or, this needs to be conditionalized to still do the right thing on those platforms or configurations where r9 is always (or sometimes) callee save. Or, if you can prove that r9 is actually never callee-save on those iOS ARM64 then maybe this fix is fine so long as we CC our Linux friends to that they can also investigate what to do with r9. 1070882 5 247009 fpizlo 2015-02-20 16:45:31 -0800 Comment on attachment 247009 removed register r9 from arm callee save register list View in context: https://bugs.webkit.org/attachment.cgi?id=247009&action=review >> Source/JavaScriptCore/jit/RegisterSet.cpp:-89 >> - result.set(ARMRegisters::r9); > > Currently, in theory, we could use the callee save list to control two separate things: > > 1) Deciding which registers don't need to be saved at callsites. > 2) Deciding which registers must be saved inside a callee. > > If it's possible for some of our callers to ever think that r9 is callee-save, then removing it from the list will only introduce more subtle bugs: we will incorrectly fail to save r9 in some cases. > > So, you need to determine if the iOS ARM64 ABI treat r9 as never-callee-save or sometimes-callee-save. This fix is only correct if the iOS ARM64 ABI treats r9 as never-callee-save. > > I'd also check if our Linux friends are doing ARM64, and if they are, then you need to CC them. This fix would break them if Linux was r9-always-callee-save or sometimes-callee-save. Oh, this has nothing to do with ARM64. It's ARMv7. 1070883 6 fpizlo 2015-02-20 16:46:48 -0800 Ossy: we're about to remove r9 from the callee save register list for ARMv7 because our documentation says that it's not a callee-save. Can you guys check if it is a callee-save on your ABIs? The documentation for this is very confusing. 1070954 7 ossy 2015-02-21 01:26:12 -0800 (In reply to comment #6) > Ossy: we're about to remove r9 from the callee save register list for ARMv7 > because our documentation says that it's not a callee-save. Can you guys > check if it is a callee-save on your ABIs? The documentation for this is > very confusing. Sure, will check on monday morning (CET timezone) 1070957 8 ossy 2015-02-21 03:13:50 -0800 Something is weird here. ARM traditional doesn't have calleeSaveRegisters() implementation (else case). And then I tried to remove everything from ARM Thumb2 case and add an UNREACHABLE_FOR_PLATFORM() macro, and tests still pass. It seems this code isn't used at all on ARM Linux. Will check it in detail soon. 1070986 9 ossy 2015-02-21 11:50:01 -0800 (In reply to comment #8) > Something is weird here. > > ARM traditional doesn't have calleeSaveRegisters() implementation (else > case). > And then I tried to remove everything from ARM Thumb2 case and add an > UNREACHABLE_FOR_PLATFORM() macro, and tests still pass. It seems this > code isn't used at all on ARM Linux. Will check it in detail soon. Ah, UNREACHABLE_FOR_PLATFORM() is debug only crash. But removing the register list on ARM Thumb2 Linux doesn't cause any regression on the JSC stress tests. 1071050 10 fpizlo 2015-02-21 19:34:07 -0800 (In reply to comment #9) > (In reply to comment #8) > > Something is weird here. > > > > ARM traditional doesn't have calleeSaveRegisters() implementation (else > > case). > > And then I tried to remove everything from ARM Thumb2 case and add an > > UNREACHABLE_FOR_PLATFORM() macro, and tests still pass. It seems this > > code isn't used at all on ARM Linux. Will check it in detail soon. > > Ah, UNREACHABLE_FOR_PLATFORM() is debug only crash. But removing > the register list on ARM Thumb2 Linux doesn't cause any regression > on the JSC stress tests. Ugh. So many things wrong with this picture. 1) Why isn't this crashing in debug? Maybe we don't have good enough test coverage for this path. It is admittedly a rare path, but still, we should be testing it. 2) why isn't UNREACHABLE_FOR_PLATFORM a release assert? I had changed it to be that way and had been using it that way - it's a shortcut for RELEASE_ASSERT_NOT_REACHED that also turns off clang's reachability warnings. 3) you're actually right that currently if we disable FTL then calleeSaveRegisters can totally be the empty set. This is because there are two possible uses of the set: (a) knowing which registers must be preserved inside a caller and (b) knowing which registers a caller can rely on the caller preserving. Without FTL, we happen to only use the set for (b). It's of course safe to assume that the caller preserves nothing, but you risk bad performance. Also this could turn into a very hard to debug issue if we wrote some non-FTL code that needed to use the set for (a). Can you help with this? We should fix these things. Can you confirm that we really don't see debug failures on that unreachable path? 1071066 11 ossy 2015-02-22 06:34:25 -0800 (In reply to comment #10) > Ugh. So many things wrong with this picture. > > 1) Why isn't this crashing in debug? Maybe we don't have good enough test > coverage for this path. It is admittedly a rare path, but still, we should > be testing it. I haven't checked full debug tests, because it is very slow.The problem was that I thought previously it is a release assert. I started a debug test now, let's see the real coverage. > 2) why isn't UNREACHABLE_FOR_PLATFORM a release assert? I had changed it to > be that way and had been using it that way - it's a shortcut for > RELEASE_ASSERT_NOT_REACHED that also turns off clang's reachability > warnings. I agree, it should be release assert, but it isn't. UNREACHABLE_FOR_PLATFORM is CRASH() if COMPILER(CLANG), but ASSERT_NOT_REACHED() otherwise which is simple ((void)0) in release mode. It seems you changed only the clang case to CRASH() in r168459. If you agree, we could change the not-clang case too. I would prefer RELEASE_ASSERT_NOT_REACHED() in both cases, because it has more verbose output in debug mode than a simple CRASH(). > 3) you're actually right that currently if we disable FTL then > calleeSaveRegisters can totally be the empty set. This is because there are > two possible uses of the set: (a) knowing which registers must be preserved > inside a caller and (b) knowing which registers a caller can rely on the > caller preserving. Without FTL, we happen to only use the set for (b). It's > of course safe to assume that the caller preserves nothing, but you risk bad > performance. Also this could turn into a very hard to debug issue if we > wrote some non-FTL code that needed to use the set for (a). Can you help > with this? Now I understand why it doesn't cause any problem now, there is no FTL on 32 bit and the non-FTL code doesn't use the case (a). Sure, I'll check the details of ARM Thumb2 and Traditional on Linux and add proper register list here to avoid possible bugs in the future. > We should fix these things. Can you confirm that we really don't see debug > failures on that unreachable path? I agree, I'm on it. 1071118 12 fpizlo 2015-02-22 19:25:32 -0800 (In reply to comment #11) > (In reply to comment #10) > > Ugh. So many things wrong with this picture. > > > > 1) Why isn't this crashing in debug? Maybe we don't have good enough test > > coverage for this path. It is admittedly a rare path, but still, we should > > be testing it. > > I haven't checked full debug tests, because it is very slow.The > problem was that I thought previously it is a release assert. I > started a debug test now, let's see the real coverage. Gotcha. As an aside, it's for this reason that I think that in core parts of the engine - especially JSC - it's super profitable to have release asserts whenever possible. Running tests on embedded systems is so expensive that sometimes we only get meaningful coverage from release asserts. I'm in favor of any change that turns a debug assert into a release assert if it comes with some evidence that performance is unaffected. > > > 2) why isn't UNREACHABLE_FOR_PLATFORM a release assert? I had changed it to > > be that way and had been using it that way - it's a shortcut for > > RELEASE_ASSERT_NOT_REACHED that also turns off clang's reachability > > warnings. > > I agree, it should be release assert, but it isn't. UNREACHABLE_FOR_PLATFORM > is CRASH() if COMPILER(CLANG), but ASSERT_NOT_REACHED() otherwise which is > simple ((void)0) in release mode. It seems you changed only the clang case > to CRASH() in r168459. If you agree, we could change the not-clang case too. > I would prefer RELEASE_ASSERT_NOT_REACHED() in both cases, because it has > more verbose output in debug mode than a simple CRASH(). I agree with this. It should be RELEASE_ASSERT_NOT_REACHED(). > > > 3) you're actually right that currently if we disable FTL then > > calleeSaveRegisters can totally be the empty set. This is because there are > > two possible uses of the set: (a) knowing which registers must be preserved > > inside a caller and (b) knowing which registers a caller can rely on the > > caller preserving. Without FTL, we happen to only use the set for (b). It's > > of course safe to assume that the caller preserves nothing, but you risk bad > > performance. Also this could turn into a very hard to debug issue if we > > wrote some non-FTL code that needed to use the set for (a). Can you help > > with this? > > Now I understand why it doesn't cause any problem now, there is > no FTL on 32 bit and the non-FTL code doesn't use the case (a). > Sure, I'll check the details of ARM Thumb2 and Traditional on Linux > and add proper register list here to avoid possible bugs in the future. Thanks! > > > We should fix these things. Can you confirm that we really don't see debug > > failures on that unreachable path? > > I agree, I'm on it. 1071188 13 ossy 2015-02-23 06:37:07 -0800 I ran a full debug test on ARM Traditional and Thumb2 too at night, everything work fine, except UNREACHABLE_FOR_PLATFORM() hit in calleeSaveRegisters() on ARM Traditional in ~30% of the tests. So the test coverage seems to be good here. :) And I checked the documentation about r9. AAPCS doensn't specify clearly if it is callee-saved or not, but I think it's safe to consider to be callee-saved on Linux. I haven't found any example for the opposite case. (+info: https://bugs.webkit.org/show_bug.cgi?id=141903#c2 ) Additionally I filed new bug reports to - add ARM traditional implementation of calleeSaveRegisters() - bug141903 - change UNREACHABLE_FOR_PLATFORM() to release assert - bug141904 1071302 14 247139 mmirman 2015-02-23 13:17:40 -0800 Created attachment 247139 removed register r9 from arm callee save register list Added a test case. 1071311 15 247139 msaboff 2015-02-23 13:37:06 -0800 Comment on attachment 247139 removed register r9 from arm callee save register list r=me. Please rename the test "regress-141489.js". Feel free to add a comment to the test that you are checking that r9 is no longer a callee save. 1071337 16 247139 mmirman 2015-02-23 14:12:36 -0800 Comment on attachment 247139 removed register r9 from arm callee save register list Patch landed: http://trac.webkit.org/changeset/180516 1071338 17 mmirman 2015-02-23 14:13:05 -0800 Closing bug. 247009 2015-02-20 16:21:46 -0800 2015-02-23 13:17:40 -0800 removed register r9 from arm callee save register list rdar19432916.patch text/plain 1125 mmirman SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTgwNDYzKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDE1LTAyLTIwICBNYXR0aGV3IE1pcm1hbiAgPG1taXJtYW5AYXBwbGUuY29tPgorCisgICAg ICAgIHI5IGlzbid0IG5lY2Vzc2FyaWx5IGEgY2FsbGVlIHNhdmUgcmVnaXN0ZXIgb24gQVJNdjcu CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDE0ODkK KyAgICAgICAgcmRhcjovL3Byb2JsZW0vMTk0MzI5MTYKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGppdC9SZWdpc3RlclNldC5jcHA6CisgICAgICAg IChKU0M6OlJlZ2lzdGVyU2V0OjpjYWxsZWVTYXZlUmVnaXN0ZXJzKToKKwogMjAxNS0wMi0yMCAg THVjYXMgRm9yc2NobGVyICA8bGZvcnNjaGxlckBhcHBsZS5jb20+CiAKICAgICAgICAgTWVyZ2Ug cjE4MDIzNwpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9SZWdpc3RlclNldC5jcHAK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9SZWdpc3RlclNldC5jcHAJ KHJldmlzaW9uIDE4MDQ2MSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvUmVnaXN0ZXJT ZXQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC04Niw3ICs4Niw2IEBACiAgICAgcmVzdWx0LnNldChB Uk1SZWdpc3RlcnM6OnI1KTsKICAgICByZXN1bHQuc2V0KEFSTVJlZ2lzdGVyczo6cjYpOwogICAg IHJlc3VsdC5zZXQoQVJNUmVnaXN0ZXJzOjpyOCk7Ci0gICAgcmVzdWx0LnNldChBUk1SZWdpc3Rl cnM6OnI5KTsKICAgICByZXN1bHQuc2V0KEFSTVJlZ2lzdGVyczo6cjEwKTsKICAgICByZXN1bHQu c2V0KEFSTVJlZ2lzdGVyczo6cjExKTsKICNlbGlmIENQVShBUk02NCkK 247139 2015-02-23 13:17:40 -0800 2015-02-23 14:12:36 -0800 removed register r9 from arm callee save register list rdar19432916.patch text/plain 2150 mmirman SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTgwNDczKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDE1LTAyLTIwICBNYXR0aGV3IE1pcm1hbiAgPG1taXJtYW5AYXBwbGUuY29tPgorCisgICAg ICAgIHI5IGlzIHZvbGF0aWxlIG9uIEFSTXY3IGZvciBpT1MgMyBhbmQgdXAuIAorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQxNDg5CisgICAgICAgIHJk YXI6Ly9wcm9ibGVtLzE5NDMyOTE2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgKiBqaXQvUmVnaXN0ZXJTZXQuY3BwOiAKKyAgICAgICAgKEpTQzo6UmVn aXN0ZXJTZXQ6OmNhbGxlZVNhdmVSZWdpc3RlcnMpOiByZW1vdmVkIHI5IGZyb20gdGhlIGxpc3Qg b2YgQVJNdjcgY2FsbGVlIHNhdmUgcmVnaXN0ZXJzLgorICAgICAgICAqIHRlc3RzL3N0cmVzcy9h cm12N19yOV9yZWdpc3Rlci5qczogQWRkZWQuCisgICAgICAgIChmb28pOgorCiAyMDE1LTAyLTIw ICBMdWNhcyBGb3JzY2hsZXIgIDxsZm9yc2NobGVyQGFwcGxlLmNvbT4KIAogICAgICAgICBNZXJn ZSByMTgwMjM3CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L1JlZ2lzdGVyU2V0LmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L1JlZ2lzdGVyU2V0LmNw cAkocmV2aXNpb24gMTgwNDYxKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9SZWdpc3Rl clNldC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTg2LDcgKzg2LDYgQEAKICAgICByZXN1bHQuc2V0 KEFSTVJlZ2lzdGVyczo6cjUpOwogICAgIHJlc3VsdC5zZXQoQVJNUmVnaXN0ZXJzOjpyNik7CiAg ICAgcmVzdWx0LnNldChBUk1SZWdpc3RlcnM6OnI4KTsKLSAgICByZXN1bHQuc2V0KEFSTVJlZ2lz dGVyczo6cjkpOwogICAgIHJlc3VsdC5zZXQoQVJNUmVnaXN0ZXJzOjpyMTApOwogICAgIHJlc3Vs dC5zZXQoQVJNUmVnaXN0ZXJzOjpyMTEpOwogI2VsaWYgQ1BVKEFSTTY0KQpJbmRleDogU291cmNl L0phdmFTY3JpcHRDb3JlL3Rlc3RzL3N0cmVzcy9hcm12N19yOV9yZWdpc3Rlci5qcwo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvdGVzdHMvc3RyZXNzL2FybXY3X3I5X3JlZ2lz dGVyLmpzCShyZXZpc2lvbiAwKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3Rlc3RzL3N0cmVz cy9hcm12N19yOV9yZWdpc3Rlci5qcwkod29ya2luZyBjb3B5KQpAQCAtMCwwICsxLDMzIEBACitm dW5jdGlvbiBpZGVudChhKSB7IAorICAgIHJldHVybiBhOyAKK30KKworZnVuY3Rpb24gZm9vKGFy cmF5LG9iaikgeyAKKyAgICB2YXIgYSA9IGFycmF5WzBdOyAKKyAgICB2YXIgYiA9IGFycmF5WzFd OyAKKyAgICB2YXIgYyA9IGFycmF5WzJdOyAKKyAgICBvYmouYSA9IGFycmF5OworICAgIG9iai5i ID0gYXJyYXk7CisgICAgb2JqLmMgPSBhcnJheTsKKyAgICBvYmouZCA9IGFycmF5OworICAgIG9i ai5lID0gYXJyYXk7CisgICAgb2JqLmYgPSBhcnJheTsKKyAgICBvYmouaCA9IGFycmF5OworICAg IHJldHVybiBhKGIoYygxMCkpKTsKK30KK25vSW5saW5lKGZvbyk7CisKK3ZhciBhcnIgPSBbaWRl bnQsaWRlbnQsaWRlbnRdOworCitmb3IgKHZhciBpID0gMDsgaSA8IDEwMDsgaSsrKSB7CisgICAg dmFyIG9iaiA9IHt9OworICAgIGZvciAodmFyIGogPSAwOyBqIDwgMjAwOyBqICsrKSB7CisgICAg ICAgIG9ialsiaiIral0gPSBpOworICAgIH0KKyAgICBmb28oYXJyLCBvYmopOworfQorCitmb3Ig KHZhciBpID0gMDsgaSA8IDEwMDsgaSsrKSB7CisgICAgdmFyIG9iaiA9IHt9OworICAgIGZvbyhh cnIsIG9iaik7Cit9ClwgTm8gbmV3bGluZSBhdCBlbmQgb2YgZmlsZQo=