Bug 129947 - Fix bugs in 32-bit Structure implementation
Summary: Fix bugs in 32-bit Structure implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 129318 129996
  Show dependency treegraph
 
Reported: 2014-03-07 17:43 PST by Mark Lam
Modified: 2014-03-09 19:29 PDT (History)
7 users (show)

See Also:


Attachments
the patch: still needs a lot of testing. (8.68 KB, patch)
2014-03-07 18:07 PST, Mark Lam
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-03-07 17:43:24 PST
There were some bugs in the 32-bit Structures implementation landed for https://bugs.webkit.org/show_bug.cgi?id=123195 mostly in the form of not loading the Structure from JSCells before using them.  In some cases, this results in crashes.  In other cases, the bug results in incorrect behavior but has gone quietly undetected thus far.  This patch searches for uses of "Structure::" in the llint/, hit/, and dfg/ directories and ensure that the Structure was loaded from the JSCell in use.
Comment 1 Mark Lam 2014-03-07 17:44:55 PST
(In reply to comment #0)
> There were some bugs in the 32-bit Structures implementation landed for https://bugs.webkit.org/show_bug.cgi?id=123195 mostly in the form of not loading the Structure from JSCells before using them.  In some cases, this results in crashes.  In other cases, the bug results in incorrect behavior but has gone quietly undetected thus far.  This patch searches for uses of "Structure::" in the llint/, hit/, and dfg/ directories and ensure that the Structure was loaded from the JSCell in use.

"hit/" ==> "jit/".  Darn spelling auto-correct.
Comment 2 Mark Lam 2014-03-07 17:58:36 PST
<rdar://problem/16268223>
Comment 3 Mark Lam 2014-03-07 18:07:33 PST
Created attachment 226195 [details]
the patch: still needs a lot of testing.
Comment 4 Mark Hahnenberg 2014-03-07 19:03:00 PST
Comment on attachment 226195 [details]
the patch: still needs a lot of testing.

View in context: https://bugs.webkit.org/attachment.cgi?id=226195&action=review

r=me with required changes.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4415
> +            GPRTemporary scratch(this);

This is not correct. You can't start allocating registers after you've already started doing control flow. You need to allocate this GPR up where the local/remote global object GPRs are allocated.
Comment 5 Mark Lam 2014-03-07 22:21:35 PST
Thanks.  Fix has been applied as suggested.  Landed in r165325: <http://trac.webkit.org/r165325>.