Bug 142527 - "this" should be in TDZ until super is called in the constructor of a derived class
Summary: "this" should be in TDZ until super is called in the constructor of a derived...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 140491
  Show dependency treegraph
 
Reported: 2015-03-10 00:43 PDT by Ryosuke Niwa
Modified: 2015-03-12 20:09 PDT (History)
9 users (show)

See Also:


Attachments
Adds TDZ in LLint and Baseline JIT (21.57 KB, patch)
2015-03-10 02:57 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added a missing @skip (21.58 KB, patch)
2015-03-10 02:58 PDT, Ryosuke Niwa
fpizlo: review-
Details | Formatted Diff | Diff
WIP including DFG (31.34 KB, patch)
2015-03-11 14:28 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
all done (32.53 KB, patch)
2015-03-11 14:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
Updated change log (38.88 KB, patch)
2015-03-12 00:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Benchmark results (52.00 KB, text/plain)
2015-03-12 00:08 PDT, Ryosuke Niwa
no flags Details
Benchmark results (52.63 KB, text/plain)
2015-03-12 00:38 PDT, Ryosuke Niwa
no flags Details
Fixed 32-bit build (38.88 KB, patch)
2015-03-12 00:42 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2015-03-10 00:43:01 PDT
Implement TDZ as an op-code and use it in the class implementation.
Comment 1 Ryosuke Niwa 2015-03-10 02:57:13 PDT
Created attachment 248323 [details]
Adds TDZ in LLint and Baseline JIT
Comment 2 Ryosuke Niwa 2015-03-10 02:58:42 PDT
Created attachment 248324 [details]
Added a missing @skip
Comment 3 Filip Pizlo 2015-03-10 09:19:24 PDT
Comment on attachment 248324 [details]
Added a missing @skip

You should add this opcode to the DFG and FTL as part of this patch.  It's not hard to do, and we should stop this bad habit of not adding new things to the optimizing JITs immediately.
Comment 4 Ryosuke Niwa 2015-03-10 10:09:49 PDT
Okay, but I literally have no idea what I'm supposed to do in DFG and FTL.  I think I need to send this bug to someone in JSC team in that case...
Comment 5 Radar WebKit Bug Importer 2015-03-10 21:59:41 PDT
<rdar://problem/20117192>
Comment 6 Filip Pizlo 2015-03-11 14:28:04 PDT
Created attachment 248449 [details]
WIP including DFG
Comment 7 Filip Pizlo 2015-03-11 14:55:18 PDT
Created attachment 248453 [details]
all done
Comment 8 Ryosuke Niwa 2015-03-12 00:06:24 PDT
Created attachment 248496 [details]
Updated change log
Comment 9 Ryosuke Niwa 2015-03-12 00:08:23 PDT
Created attachment 248497 [details]
Benchmark results

I've ran relevant benchmarks with this patch applied. It doesn't seem to be a regression or progression at r181424.
Comment 10 Ryosuke Niwa 2015-03-12 00:38:42 PDT
Created attachment 248498 [details]
Benchmark results

Oops, the previous benchmark result was wrong.  I wasn't turning on ES6_CLASS_SYNTAX.  I've re-measured it and I still don't see any progression or regression.
Comment 11 Ryosuke Niwa 2015-03-12 00:42:07 PDT
Created attachment 248499 [details]
Fixed 32-bit build
Comment 12 Ryosuke Niwa 2015-03-12 08:20:07 PDT
Comment on attachment 248499 [details]
Fixed 32-bit build

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

> Source/JavaScriptCore/ChangeLog:63
> +        (JSC::DFG::SpeculativeJIT::compile): Speculative the operand to be not empty. OSR exit if the speculation fails.

Speculate*

> Source/JavaScriptCore/ChangeLog:70
> +        (JSC::FTL::LowerDFGToLLVM::compileCheckNotEmpty): OSR exit with "TDZFailure" if the operand is not empty.

if the operand *is* empty.
Comment 13 Mark Hahnenberg 2015-03-12 12:10:48 PDT
Comment on attachment 248499 [details]
Fixed 32-bit build

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

> Source/JavaScriptCore/tests/stress/class-syntax-tdz.js:1
> +//@ skip

Why are we skipping? Does this not mean what I think it means?
Comment 14 Ryosuke Niwa 2015-03-12 17:16:26 PDT
(In reply to comment #13)
> Comment on attachment 248499 [details]
> Fixed 32-bit build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248499&action=review
> 
> > Source/JavaScriptCore/tests/stress/class-syntax-tdz.js:1
> > +//@ skip
> 
> Why are we skipping? Does this not mean what I think it means?

ES6 class syntax is disabled by default.
Comment 15 Mark Hahnenberg 2015-03-12 17:20:41 PDT
Comment on attachment 248499 [details]
Fixed 32-bit build

LGTM.
Comment 16 WebKit Commit Bot 2015-03-12 18:11:47 PDT
Comment on attachment 248499 [details]
Fixed 32-bit build

Clearing flags on attachment: 248499

Committed r181466: <http://trac.webkit.org/changeset/181466>
Comment 17 WebKit Commit Bot 2015-03-12 18:11:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Joseph Pecoraro 2015-03-12 20:09:01 PDT
Comment on attachment 248499 [details]
Fixed 32-bit build

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

> Source/JavaScriptCore/tests/stress/class-syntax-tdz.js:26
> +        throw "Exception not thrown for an unitialized this at iteration " + i;

Typo: "unitialized" => "uninitialized"! But doesn't really matter. Nice patch!