156343 2016-04-07 08:59:30 -0700 [Win] Fix for JSC stress test failures. 2016-04-07 10:39:55 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 peavo webkit-unassigned achristensen bfulgham commit-queue fpizlo keith_miller mark.lam msaboff saam oldest_to_newest 1181689 0 peavo 2016-04-07 08:59:30 -0700 We need to make it clear to MSVC that the method loadPtr(ImplicitAddress address, RegisterID dest) should be used, and not loadPtr(const void* address, RegisterID dest). 1181692 1 275891 peavo 2016-04-07 09:03:56 -0700 Created attachment 275891 Patch 1181693 2 275891 achristensen 2016-04-07 09:10:48 -0700 Comment on attachment 275891 Patch This seems to be used a few other places, but I don't understand how a GPRInfo is being interpreted as a void*. 1181695 3 275891 fpizlo 2016-04-07 09:21:01 -0700 Comment on attachment 275891 Patch Wow, crazy! 1181716 4 peavo 2016-04-07 10:31:34 -0700 Thanks for reviewing :) 1181720 5 peavo 2016-04-07 10:33:32 -0700 Committed r199160: <http://trac.webkit.org/changeset/199160> 1181722 6 fpizlo 2016-04-07 10:39:55 -0700 By the way: the fact that load and friends take const void* instead of AbsoluteAddress and they take GPRReg instead of Address (via the weird ImplicitAddress thing) is a vestige of a past long gone. It would be super cool to replace all cases of the assembler taking void* and change them to take AbsoluteAddress. It would also be cool to get rid of ImplicitAddress and make everyone use Address. It's not obvious why MSVC interpreted the code the way that it did, but I can sort of see it, and it's scary to think just how much our load() and store() methods rely on very specific interpretations of overload resolution to work properly. A RegisterID/GPRReg is just an enum, which coerces to int, and regT0 is a zero-valued enum. We want MSVC to realize that the preferred overload resolution is to coerce to ImplicitAddress, but apparently due to the zeroish nature of regT0, it's treating it as null-like and coercing to void*. Whether that's right or wrong, I don't really care, because I don't like it when the correctness of code relies so much on C++'s arcane overload resolution rules. Therefore, I think that this change is a big win for correctness, and I think we should use Address() explicitly in more places. 275891 2016-04-07 09:03:56 -0700 2016-04-07 09:21:01 -0700 Patch bug-156343-20160407180410.patch text/plain 2049 peavo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTk5MTU2KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDE2LTA0LTA3ICBQZXIgQXJuZSBWb2xsYW4gIDxwZWF2b0BvdXRsb29rLmNvbT4KKworICAg ICAgICBbV2luXSBGaXggZm9yIEpTQyBzdHJlc3MgdGVzdCBmYWlsdXJlcy4KKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NjM0MworCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIG5lZWQgdG8gbWFrZSBpdCBj bGVhciB0byBNU1ZDIHRoYXQgdGhlIG1ldGhvZCBsb2FkUHRyKEltcGxpY2l0QWRkcmVzcyBhZGRy ZXNzLCBSZWdpc3RlcklEIGRlc3QpCisgICAgICAgIHNob3VsZCBiZSB1c2VkLCBhbmQgbm90IGxv YWRQdHIoY29uc3Qgdm9pZCogYWRkcmVzcywgUmVnaXN0ZXJJRCBkZXN0KS4KKworICAgICAgICAq IGppdC9DQ2FsbEhlbHBlcnMuY3BwOgorICAgICAgICAoSlNDOjpDQ2FsbEhlbHBlcnM6OnNldHVw U2hhZG93Q2hpY2tlblBhY2tldCk6CisKIDIwMTYtMDQtMDYgIEJlbmphbWluIFBvdWxhaW4gIDxi cG91bGFpbkBhcHBsZS5jb20+CiAKICAgICAgICAgW0pTQ10gVUludDMyVG9OdW1iZXIgc2hvdWxk IGJlIE5vZGVNdXN0R2VuZXJhdGUKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvQ0Nh bGxIZWxwZXJzLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0ND YWxsSGVscGVycy5jcHAJKHJldmlzaW9uIDE5OTE1NikKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29y ZS9qaXQvQ0NhbGxIZWxwZXJzLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTIsMTYgKzUyLDE2IEBA IHZvaWQgQ0NhbGxIZWxwZXJzOjpsb2dTaGFkb3dDaGlja2VuVGFpbFAKIHZvaWQgQ0NhbGxIZWxw ZXJzOjpzZXR1cFNoYWRvd0NoaWNrZW5QYWNrZXQoKQogewogICAgIG1vdmUoVHJ1c3RlZEltbVB0 cih2bSgpLT5zaGFkb3dDaGlja2VuKCkuYWRkcmVzc09mTG9nQ3Vyc29yKCkpLCBHUFJJbmZvOjpy ZWdUMCk7Ci0gICAgbG9hZFB0cihHUFJJbmZvOjpyZWdUMCwgR1BSSW5mbzo6cmVnVDEpOworICAg IGxvYWRQdHIoQWRkcmVzcyhHUFJJbmZvOjpyZWdUMCksIEdQUkluZm86OnJlZ1QxKTsKICAgICBK dW1wIG9rID0gYnJhbmNoUHRyKEJlbG93LCBHUFJJbmZvOjpyZWdUMSwgVHJ1c3RlZEltbVB0cih2 bSgpLT5zaGFkb3dDaGlja2VuKCkubG9nRW5kKCkpKTsKICAgICBzZXR1cEFyZ3VtZW50c0V4ZWNT dGF0ZSgpOwogICAgIG1vdmUoVHJ1c3RlZEltbVB0cihiaXR3aXNlX2Nhc3Q8dm9pZCo+KG9wZXJh dGlvblByb2Nlc3NTaGFkb3dDaGlja2VuTG9nKSksIEdQUkluZm86Om5vbkFyZ0dQUjApOwogICAg IGNhbGwoR1BSSW5mbzo6bm9uQXJnR1BSMCk7CiAgICAgbW92ZShUcnVzdGVkSW1tUHRyKHZtKCkt PnNoYWRvd0NoaWNrZW4oKS5hZGRyZXNzT2ZMb2dDdXJzb3IoKSksIEdQUkluZm86OnJlZ1QwKTsK LSAgICBsb2FkUHRyKEdQUkluZm86OnJlZ1QwLCBHUFJJbmZvOjpyZWdUMSk7CisgICAgbG9hZFB0 cihBZGRyZXNzKEdQUkluZm86OnJlZ1QwKSwgR1BSSW5mbzo6cmVnVDEpOwogICAgIG9rLmxpbmso dGhpcyk7CiAgICAgYWRkUHRyKFRydXN0ZWRJbW0zMihzaXplb2YoU2hhZG93Q2hpY2tlbjo6UGFj a2V0KSksIEdQUkluZm86OnJlZ1QxLCBHUFJJbmZvOjpyZWdUMik7Ci0gICAgc3RvcmVQdHIoR1BS SW5mbzo6cmVnVDIsIEdQUkluZm86OnJlZ1QwKTsKKyAgICBzdG9yZVB0cihHUFJJbmZvOjpyZWdU MiwgQWRkcmVzcyhHUFJJbmZvOjpyZWdUMCkpOwogfQogCiB9IC8vIG5hbWVzcGFjZSBKU0MK