Bug 19076 - SquirrelFish: RegisterFile can be corrupted if implictly reenter global scope with no declared vars
Summary: SquirrelFish: RegisterFile can be corrupted if implictly reenter global scope...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Blocker
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-15 05:02 PDT by Oliver Hunt
Modified: 2008-05-16 17:28 PDT (History)
3 users (show)

See Also:


Attachments
a fix (5.88 KB, patch)
2008-05-15 05:19 PDT, Oliver Hunt
ggaren: review+
Details | Formatted Diff | Diff
A better fix, which despite being technically superior is slower. I hate GCC. (8.94 KB, patch)
2008-05-16 00:04 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2008-05-15 05:02:23 PDT
Seen on http://informationliberation.com/ - I cannot workout how to trigger this myself, but the issue is when generating globalcode we use the existence of globals to tell us that we've initialised "this" in the global scope.  But if we have re-entered and no variables have been defined yet we re-add this, resulting in us trying to increase the number of globals in the containing scope, leading to badness.
Comment 1 Oliver Hunt 2008-05-15 05:19:12 PDT
Created attachment 21161 [details]
a fix

This fixes the bug by not using the symboltable for the global this reference, so we don't need to track initialisation as carefully.  My system registers it as a 0.1-0.2% regression though :-/
Comment 2 Cameron Zwarich (cpst) 2008-05-15 06:12:00 PDT
Now that Oliver rolled out r33483, this patch is a 0.5% progression for me, bringing us to our best time in quite a while.
Comment 3 Oliver Hunt 2008-05-15 06:13:13 PDT
Comment on attachment 21161 [details]
a fix

My system also registers this as a progression so r?
Comment 4 Geoffrey Garen 2008-05-15 09:55:11 PDT
Comment on attachment 21161 [details]
a fix

It's a little squirrely to have a variable named "m_thisRegister" that's only valid if we happen to be compiling program code.

I'd recommend setting m_thisRegister when compiling function code and eval code as well. Then, "thisRegister()" can always return m_thisRegister, which makes a lot more sense.

Eventually, we'll won't put "this" in the symbol table at all.

r=me
Comment 5 Oliver Hunt 2008-05-15 12:46:35 PDT
(In reply to comment #4)
> (From update of attachment 21161 [details] [edit])
> It's a little squirrely to have a variable named "m_thisRegister" that's only
> valid if we happen to be compiling program code.
> 
> I'd recommend setting m_thisRegister when compiling function code and eval code
> as well. Then, "thisRegister()" can always return m_thisRegister, which makes a
> lot more sense.
> 
> Eventually, we'll won't put "this" in the symbol table at all.
> 
> r=me
> 

I agree, but that cost of retrieving the this register for function/evalnode codegenerator was a regression -- but that was before we rolled out the patch o' badness
Comment 6 Oliver Hunt 2008-05-16 00:04:22 PDT
Created attachment 21196 [details]
A better fix, which despite being technically superior is slower.  I hate GCC.
Comment 7 Oliver Hunt 2008-05-16 03:34:44 PDT
Test case:
<body onload="document.write('<script' + '>' + 'var x = 0; var y = 1; var z = 2;' + '</script' + '>');">
</body>
Comment 8 Oliver Hunt 2008-05-16 14:53:11 PDT
I have a fix which ggaren is okay with, but it's causing a webkit regression, i'm trying to work out how.
Comment 9 Oliver Hunt 2008-05-16 17:28:41 PDT
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/VM/CodeGenerator.cpp
	M	JavaScriptCore/VM/CodeGenerator.h
	M	JavaScriptCore/VM/Machine.cpp
	M	JavaScriptCore/kjs/ExecState.h
	M	JavaScriptCore/kjs/JSGlobalObject.cpp
	M	JavaScriptCore/kjs/JSGlobalObject.h
	M	JavaScriptCore/kjs/nodes.cpp
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/js/direct-entry-to-function-code-expected.txt
	A	LayoutTests/fast/js/direct-entry-to-function-code.html
Committed r33541