Bug 129947

Summary: Fix bugs in 32-bit Structure implementation
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mhahnenberg, mmirman, msaboff, oliver, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 129318, 129996    
Attachments:
Description Flags
the patch: still needs a lot of testing. mhahnenberg: review+

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>.