Bug 142071 – BytecodeGenerator::constLocal() behaves identically to BytecodeGenerator::local() for the purposes of its one caller

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Filip Pizlo]

So, remove constLocal().

The behavioral differences are:

- constLocal() doesn't have a special case for "this" that overrides other checks like the shouldOptimizeLocals() check.  But the one user of constLocal() is for the "const x" expression, and "const this" doesn't parse.

- constLocal() won't createArgumentsIfNecessary() for "arguments".  But it's harmless if it does, since its one user assigns to the local.

So, we can remove constLocal() and make its one caller use local() instead.

[Filip Pizlo]

Created attachment 247483 [details]
the patch

[Mark Lam]

Comment on attachment 247483 [details]
the patch

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

Missing ChangeLog.  The text in your bugzilla description is good to have in the ChangeLog.

r=me with ChangeLog added.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4
> +*  Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2012, 2013, 2015 Apple Inc. All rights reserved.

nit: You can collapse this to:
 * Copyright (C) 2003-2009, 2012-2013, 2015 Apple Inc. All rights reserved.

[Filip Pizlo]

Created attachment 247484 [details]
the patch

And now, with a ChangeLog!

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/180723

[Alexey Proskuryakov]

This broke Windows tests:

** The following JSC stress test failures have been introduced:
	stress/const-arguments.js.always-trigger-copy-phase
	stress/const-arguments.js.default
	stress/const-arguments.js.dfg-eager
	stress/const-arguments.js.dfg-eager-no-cjit-validate
	stress/const-arguments.js.no-cjit-validate-phases
	stress/const-arguments.js.no-llint

[Filip Pizlo]

(In reply to comment #5)
> This broke Windows tests:
> 
> ** The following JSC stress test failures have been introduced:
> 	stress/const-arguments.js.always-trigger-copy-phase
> 	stress/const-arguments.js.default
> 	stress/const-arguments.js.dfg-eager
> 	stress/const-arguments.js.dfg-eager-no-cjit-validate
> 	stress/const-arguments.js.no-cjit-validate-phases
> 	stress/const-arguments.js.no-llint

It might have broken other things too.  I'm looking.

[Brent Fulgham]

This broke the iOS build and Windows debug tests.

This should be rolled out if it cannot be fixed ASAP.

[Filip Pizlo]

(In reply to comment #7)
> This broke the iOS build and Windows debug tests.
> 
> This should be rolled out if it cannot be fixed ASAP.

I'm also seeing lots of breakage.  I agree with rollout.

[Filip Pizlo]

Rolled out in http://trac.webkit.org/changeset/180732

[Filip Pizlo]

It looks like there is some bizarre behavior here.  I'll have to address it a different way in my refactoring in bug 141174.