Bug 151113

According to the spec, we need to make op_create_this throw when "this" is already "filled".

I'm working on patch for this issue. I've created new op code - op_is_empty to check if thisRegister is empty or unassigned before assign value returned from parent constructor(super). I implemented op_is_empty in LLInt and currently working on implementing it in JITOpcodes & DFG

(In reply to comment #2) > I'm working on patch for this issue. > I've created new op code - op_is_empty to check if thisRegister is empty or > unassigned before assign value returned from parent constructor(super). I > implemented op_is_empty in LLInt and currently working on implementing it in > JITOpcodes & DFG Why do you need a new op code for this? Can't we just use check_tdz?

(In reply to comment #3) > (In reply to comment #2) > > I'm working on patch for this issue. > > I've created new op code - op_is_empty to check if thisRegister is empty or > > unassigned before assign value returned from parent constructor(super). I > > implemented op_is_empty in LLInt and currently working on implementing it in > > JITOpcodes & DFG > > Why do you need a new op code for this? Can't we just use check_tdz? Doing that would introduce a synthetic try/catch around something we expect to often be true.

Created attachment 276044 [details] Patch Upload to test in differen ARCHs, more tests will be in next patch

Created attachment 276047 [details] Patch Fix build for 32bit

Created attachment 276048 [details] Patch Fix builds

Comment on attachment 276048 [details] Patch Attachment 276048 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1122817 New failing tests: js/regress/arrowfunction-call-in-class-constructor.html

Created attachment 276055 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 276048 [details] Patch Attachment 276048 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1122818 New failing tests: js/regress/arrowfunction-call-in-class-constructor.html

Created attachment 276057 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 276048 [details] Patch Attachment 276048 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1122834 New failing tests: js/regress/arrowfunction-call-in-class-constructor.html

Created attachment 276059 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 276048 [details] Patch Attachment 276048 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1122851 New failing tests: js/regress/arrowfunction-call-in-class-constructor.html

Created attachment 276061 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 276086 [details] Patch Fix builds

Created attachment 276090 [details] Patch New patch with new tests

*** Bug 155060 has been marked as a duplicate of this bug. ***

Created attachment 276169 [details] Patch Added more tests

Comment on attachment 276169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276169&action=review LGTM besides my suggestions in abstract interpreter code gen. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:770 > + generator.emitThrowReferenceError(ASCIILiteral("'super()' can't be called more than once in constructor.")); "in constructor" => "in a constructor" > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:967 > + case IsEmpty: I think we can more aggressively constant fold this based on type information as well. We can fold to false if the speculated type for child1 doesn't have SpecEmpty in it. We can fold to true if the speculated type is equal to SpecEmpty. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4405 > + case IsEmpty: { This code is more easily written as a compare instruction. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4410 > + case IsEmpty: { I think the below code is subtly wrong because you just or the ValueTrue/False disregarding junk old values in the register. But, regardless of that, this code is better written as a compare instruction + "or ValueFalse". I believe we use this paradigm in other code in the DFG. > Source/JavaScriptCore/jit/JITOpcodes.cpp:179 > +void JIT::emit_op_is_empty(Instruction* currentInstruction) Ditto with compare instruction. > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:294 > +void JIT::emit_op_is_empty(Instruction* currentInstruction) Ditto with compare instruction. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1214 > +_llint_op_is_empty: Ditto > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1102 > +_llint_op_is_empty: Ditto.

Created attachment 276261 [details] Patch Fix comments

Comment on attachment 276169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276169&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:967 >> + case IsEmpty: > > I think we can more aggressively constant fold this based on type information as well. > We can fold to false if the speculated type for child1 doesn't have SpecEmpty in it. > We can fold to true if the speculated type is equal to SpecEmpty. I'll try to play, hope in with next patch will be fixed this comment >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4405 >> + case IsEmpty: { > > This code is more easily written as a compare instruction. Hope I did in way that you mean, but I don't know if it correct work in 32bit >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4410 >> + case IsEmpty: { > > I think the below code is subtly wrong because you just or the ValueTrue/False disregarding junk old > values in the register. But, regardless of that, this code is better written as a compare instruction > + "or ValueFalse". I believe we use this paradigm in other code in the DFG. Refactored >> Source/JavaScriptCore/jit/JITOpcodes.cpp:179 >> +void JIT::emit_op_is_empty(Instruction* currentInstruction) > > Ditto with compare instruction. Refactored >> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:294 >> +void JIT::emit_op_is_empty(Instruction* currentInstruction) > > Ditto with compare instruction. Refactored >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1214 >> +_llint_op_is_empty: > > Ditto Refactored >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1102 >> +_llint_op_is_empty: > > Ditto. Refactored

Comment on attachment 276261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276261&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4414 > + m_jit.compare64(JITCompiler::Equal, value.gpr(), TrustedImm32(ValueEmpty), result.gpr()); Yup, this is what I meant. I think this is better written as: m_jit.comparePtr/compare64(JITCompiler::Equal, value.gpr(), TrustedImmPtr/TrustedImm32(JSValue()), result.gpr()) > Source/JavaScriptCore/jit/JITOpcodes.cpp:185 > + compare64(Equal, regT0, TrustedImm32(ValueEmpty), regT0); ditto here.

Comment on attachment 276261 [details] Patch Attachment 276261 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1143575 New failing tests: js/class-syntax-super.html

Created attachment 276267 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 276261 [details] Patch Attachment 276261 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1143617 New failing tests: js/class-syntax-super.html

Created attachment 276269 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 276261 [details] Patch Attachment 276261 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1143647 New failing tests: js/class-syntax-super.html

Created attachment 276271 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 276261 [details] Patch Attachment 276261 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1143813 New failing tests: js/class-syntax-super.html

Created attachment 276279 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 276318 [details] Patch fix tests

Created attachment 276338 [details] Patch fix build&tests

Created attachment 276498 [details] Patch Fix comments

Comment on attachment 276261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276261&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4414 >> + m_jit.compare64(JITCompiler::Equal, value.gpr(), TrustedImm32(ValueEmpty), result.gpr()); > > Yup, this is what I meant. > > I think this is better written as: > m_jit.comparePtr/compare64(JITCompiler::Equal, value.gpr(), TrustedImmPtr/TrustedImm32(JSValue()), result.gpr()) done >> Source/JavaScriptCore/jit/JITOpcodes.cpp:185 >> + compare64(Equal, regT0, TrustedImm32(ValueEmpty), regT0); > > ditto here. done

Comment on attachment 276169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276169&action=review >>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:967 >>> + case IsEmpty: >> >> I think we can more aggressively constant fold this based on type information as well. >> We can fold to false if the speculated type for child1 doesn't have SpecEmpty in it. >> We can fold to true if the speculated type is equal to SpecEmpty. > > I'll try to play, hope in with next patch will be fixed this comment It seems that I managed to do this.

Comment on attachment 276498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276498&action=review > Source/JavaScriptCore/ChangeLog:14 > + Current patch implement check if 'super()' was called in constructor > + more than once and raise RuntimeError if 'super()' called second time. > + According to spec we need to raise error just after second super() > + is finished, and before new this is assign > + https://esdiscuss.org/topic/duplicate-super-call-behaviour. > + To implement this behavior was introduced new op code - op_is_empty > + that is used to check if 'this' is empty. I think the phrasing of the changelog would be clearer as: Currently, our implementation checks if 'super()' was called in a constructor more than once and raises a RuntimeError before the second call. According to the spec we need to raise an error just after the second super() is finished and before the new 'this' is assigned https://esdiscuss.org/topic/duplicate-super-call-behaviour. To implement this behavior this patch adds a new op code, op_is_empty, that is used to check if 'this' is empty. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1097 > + if (!(child.m_type & ~SpecEmpty)) { > + setConstant(node, jsBoolean(true)); > + constantWasSet = true; > + break; > + } I think this case is wrong. If the abstract interpreter has no information then child.m_type will be SpecNone (0) and this case will convert the IsEmpty check into a constant. I think a correct condition would be "child.m_type && !(child.m_type & ~SpecEmpty)".

Created attachment 276660 [details] Patch Fix the latest comments

Comment on attachment 276660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276660&action=review r=me with comments. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1101 > + if (!(child.m_type & SpecEmpty)) { You also want this one to check child.m_type && ... > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4419 > + m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(ValueEmpty), result.gpr()); I think this is probably correct, but I think it's way clearer as: m_jit.comparePtr(Equal, value.gpr(), TrustedImm64(JSValue::encode(JSValue())), result.gpr()) > Source/JavaScriptCore/jit/JITOpcodes.cpp:185 > + compare64(Equal, regT0, TrustedImm32(ValueEmpty), regT0); ditto.

Comment on attachment 276498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276498&action=review Thanks for review >> Source/JavaScriptCore/ChangeLog:14 >> + that is used to check if 'this' is empty. > > I think the phrasing of the changelog would be clearer as: > > Currently, our implementation checks if 'super()' was called in a constructor more than once and raises a RuntimeError before the second call. According to the spec we need to raise an error just after the second super() is finished and before the new 'this' is assigned https://esdiscuss.org/topic/duplicate-super-call-behaviour. To implement this behavior this patch adds a new op code, op_is_empty, that is used to check if 'this' is empty. Yeah, fixed. English is not my mother tongue language. Thanks! >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1097 >> + } > > I think this case is wrong. If the abstract interpreter has no information then child.m_type will be SpecNone (0) and this case will convert the IsEmpty check into a constant. I think a correct condition would be "child.m_type && !(child.m_type & ~SpecEmpty)". Done

Comment on attachment 276660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276660&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1101 >> + if (!(child.m_type & SpecEmpty)) { > > You also want this one to check child.m_type && ... Done. What do I need to do to cover this code by tests? >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4419 >> + m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(ValueEmpty), result.gpr()); > > I think this is probably correct, but I think it's way clearer as: > m_jit.comparePtr(Equal, value.gpr(), TrustedImm64(JSValue::encode(JSValue())), result.gpr()) Done by following code m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(JSValue::encode(JSValue())), result.gpr()) >> Source/JavaScriptCore/jit/JITOpcodes.cpp:185 >> + compare64(Equal, regT0, TrustedImm32(ValueEmpty), regT0); > > ditto. Done

Comment on attachment 276660 [details] Patch Thanks for review

Just to document, it was landed in https://trac.webkit.org/changeset/199712

and it caused a serious regression on 32 bit platforms, see bug156740 for details.

Re-opened since this is blocked by bug 156741

Comment on attachment 276660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276660&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1221 > + storei t3, PayloadOffset[cfr, t0, 8] You need to store the Boolean tag as well here.

Created attachment 276749 [details] Patch Fix tests on 32bit

Comment on attachment 276749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276749&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103 > + constantWasSet = true; Doesn't setConstant already assign to this variable for us? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4440 > + m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(JSValue::encode(JSValue())), result.gpr()); This should be TrustedImm64

Comment on attachment 276749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276749&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103 >> + constantWasSet = true; > > Doesn't setConstant already assign to this variable for us? I did as do in another 'case's. Anyway constantWasSet is local variable declared just above of the switch/case construction, but In setConstant we assign m_foundConstants of m_state object. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4440 >> + m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(JSValue::encode(JSValue())), result.gpr()); > > This should be TrustedImm64 There is no function for TrustedImm64 in MacroAssembler.h, only for TrustedImm32. Do I need create new one? https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/assembler/MacroAssembler.h#L998

(In reply to comment #49) > Comment on attachment 276749 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276749&action=review > > >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103 > >> + constantWasSet = true; > > > > Doesn't setConstant already assign to this variable for us? > > I did as do in another 'case's. Anyway constantWasSet is local variable > declared just above of the switch/case construction, but In setConstant we > assign m_foundConstants of m_state object. > > >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4440 > >> + m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(JSValue::encode(JSValue())), result.gpr()); > > > > This should be TrustedImm64 > > There is no function for TrustedImm64 in MacroAssembler.h, only for > TrustedImm32. Do I need create new one? > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/assembler/ > MacroAssembler.h#L998 Right. I think you want TrustedImmPtr which is 64 bits on 64bit architectures.

Created attachment 276848 [details] Patch Fix comments

Ok my advice was wrong. We don't have 64bit immediate compares on the architectures we care about. We just want comparePtr or compare64(Equal, reg, TrustedImm32(0), resultReg) (The imm value is sign extended). And our MASM will do the right thing and convert this into a test of reg on itself.

Created attachment 277388 [details] Patch Double check build

Committed r200102: <http://trac.webkit.org/changeset/200102> Patch is landed and issue can be closed

Comment on attachment 277388 [details] Patch Clear flag on attachment

marking as resolved.