Bug 70403 - bytecompiler sometimes generates incorrect bytecode for put_by_id
Summary: bytecompiler sometimes generates incorrect bytecode for put_by_id
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-19 01:34 PDT by Zheng Liu
Modified: 2011-10-21 02:10 PDT (History)
6 users (show)

See Also:

Attachments
A simple patch to copy the rvalue before put. (1.47 KB, patch)
2011-10-19 01:46 PDT, Zheng Liu 		no flags Details | Formatted Diff | Diff
Fix (1.57 KB, patch)
2011-10-19 05:16 PDT, Zheng Liu 		no flags Details | Formatted Diff | Diff
Correct format. (2.69 KB, patch)
2011-10-20 19:03 PDT, Zheng Liu 		no flags Details | Formatted Diff | Diff
Fix ChangeLog (2.41 KB, patch)
2011-10-20 19:52 PDT, Zheng Liu 		ggaren: review-
Details | Formatted Diff | Diff
Added test at LayoutTests/fast/js. (4.66 KB, patch)
2011-10-21 00:58 PDT, Zheng Liu 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Zheng Liu 2011-10-19 01:34:37 PDT 
Bytecompiler sometimes generates incorrect bytecode for put_by_id.

case:(a same case of dot)
function f(a,i,j) {
  a.__defineSetter__('x',
      function(v) {
          this['z']=v;
          i='CLOBBERED';
      });
  j['y']=(a['x']=i);
  print(j.y);
};

f({},'RVAL',{});

expected: 'RVAL',  got: 'CLOBBERED'


Reason:
[  34] mov		 r4, "y"(@k1)
[  37] put_by_val	 r-9, "x"(@k0), r-8
[  41] put_by_val	 r3, r4, r-8

r-8 is clobbered in this case.
Expression (a['x']=i) should not forward r-8.
Comment 1 Zheng Liu 2011-10-19 01:46:32 PDT 
Created attachment 111577 [details]
A simple patch to copy the rvalue before put.
Comment 2 Zheng Liu 2011-10-19 05:16:44 PDT 
Created attachment 111596 [details]
Fix

Don't copy when the result is to be ignored.
Comment 3 Zheng Liu 2011-10-20 19:03:04 PDT 
Created attachment 111888 [details]
Correct format.
Comment 4 Zheng Liu 2011-10-20 19:52:12 PDT 
Created attachment 111891 [details]
Fix ChangeLog
Comment 5 Filip Pizlo 2011-10-20 21:46:00 PDT 
Great catch!  Took me a while to see what was going on.

Can you add a LayoutTest and include it in this patch?
Comment 6 Geoffrey Garen 2011-10-20 23:00:24 PDT 
Comment on attachment 111891 [details]
Fix ChangeLog

r-, but this patch looks ready to go once it has a layout test. See http://www.webkit.org/quality/testwriting.html.
Comment 7 Zheng Liu 2011-10-21 00:58:58 PDT 
Created attachment 111919 [details]
Added test at LayoutTests/fast/js.
Comment 8 Filip Pizlo 2011-10-21 00:59:47 PDT 
Comment on attachment 111919 [details]
Added test at LayoutTests/fast/js.

Looks great, r=me.
Comment 9 WebKit Review Bot 2011-10-21 02:10:34 PDT 
Comment on attachment 111919 [details]
Added test at LayoutTests/fast/js.

Clearing flags on attachment: 111919

Committed r98091: <http://trac.webkit.org/changeset/98091>
Comment 10 WebKit Review Bot 2011-10-21 02:10:40 PDT 
All reviewed patches have been landed.  Closing bug.