Bug 18274 - ResolveNode::emitCode() doesn't make a new temporary when dst is 0, leading to incorrect codegen
Summary: ResolveNode::emitCode() doesn't make a new temporary when dst is 0, leading t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-01 19:35 PDT by Cameron Zwarich (cpst)
Modified: 2008-04-02 01:12 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (2.26 KB, patch)
2008-04-01 19:40 PDT, Cameron Zwarich (cpst)
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-04-01 19:35:52 PDT
This JavaScript

var o = { a: function () { return 1; } };
o.a();
o;

generates the following code:

[   0] load		lr1, undefined(@k0)		
[   3] new_object	lr1
[   5] new_func_exp		tr0, f0
[   8] put_prop_id	lr1, a(@id0), tr0
[  12] get_prop_id	tr0, lr1, a(@id0)
[  16] call		lr1, tr0, lr1, 8, 1
[  22] end		lr1

The end call ends up overwriting the local variable o with 1, which is clearly incorrect. Obviously, we don't always want ResolveNode::emitCode() to return a new temporary register if dst is unspecified. However, there are two situations where this causes a bug:

1) The returned register is reused. This occurs in FunctionCallDotNode and FunctionCallBracketNode. I have a patch that fixes this particular problem, and I will post it for review. DoWhileNode and ForNode increment the reference count of the returned register, but only to return it, not to reuse it.

2) The returned register is ignored, requiring dst to be the actual target. This occurs in codegen for IfNode, IfElseNode, WhileNode, and ForInNode. It doesn't cause any realistic problems in any of them, because they are statements. The first three return 0 as it is, and ForInNode returns dst, which still isn't a problem if it doesn't contain the correct value, because nothing will use it once we have completions working.

Should we just accept this behaviour of emitNode and remember to code around it?
Comment 1 Cameron Zwarich (cpst) 2008-04-01 19:40:53 PDT
Created attachment 20289 [details]
Proposed patch
Comment 2 Oliver Hunt 2008-04-01 19:59:16 PDT
Landed r31555