Bug 156343 - [Win] Fix for JSC stress test failures.
Summary: [Win] Fix for JSC stress test failures.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-07 08:59 PDT by peavo
Modified: 2016-04-07 10:39 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.00 KB, patch)
2016-04-07 09:03 PDT, peavo
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2016-04-07 08:59:30 PDT
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).
Comment 1 peavo 2016-04-07 09:03:56 PDT
Created attachment 275891 [details]
Patch
Comment 2 Alex Christensen 2016-04-07 09:10:48 PDT
Comment on attachment 275891 [details]
Patch

This seems to be used a few other places, but I don't understand how a GPRInfo is being interpreted as a void*.
Comment 3 Filip Pizlo 2016-04-07 09:21:01 PDT
Comment on attachment 275891 [details]
Patch

Wow, crazy!
Comment 4 peavo 2016-04-07 10:31:34 PDT
Thanks for reviewing :)
Comment 5 peavo 2016-04-07 10:33:32 PDT
Committed r199160: <http://trac.webkit.org/changeset/199160>
Comment 6 Filip Pizlo 2016-04-07 10:39:55 PDT
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.