Fix variable decl register collision on ARM.

Fix variable decl register collision on ARM.

BUG=chromium:455207
LOG=n
R=ulan@chromium.org

Committed: https://crrev.com/bb869374fe8a9714dba9de218db330f68ca3d4f2
Cr-Commit-Position: refs/heads/master@{#30249}

[Jarin, 2015-03-27 17:09:18]

Could you take a look, please?

[Dmitry Lomov (no reviews), 2015-03-28 10:37:05]

dslomov@chromium.org changed reviewers:
+ dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-03-28 10:37:06]

lgtm, thanks for looking into this!

[Dmitry Lomov (no reviews), 2015-03-28 10:41:18]

I have second thoughts

https://codereview.chromium.org/1040703003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1040703003/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:868: __ str(r0, StackOperand(variable));
Actually there are more usages of ip below this, see line 876.
I am a bit surprised that this change fixes anything, since currently variables
that need hole-init are not stack allocated

[Benedikt Meurer, 2015-03-29 09:07:03]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-03-29 09:07:04]

https://codereview.chromium.org/1040703003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1040703003/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:877: __ str(ip, ContextOperand(cp,
variable->index()));
As noted by Dmitry, this will also cause trouble if variable->index() is too
big.

[Jarin, 2015-03-30 05:54:50]

https://codereview.chromium.org/1040703003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1040703003/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:868: __ str(r0, StackOperand(variable));
On 2015/03/28 10:41:18, Dmitry Lomov (chromium) wrote:
> Actually there are more usages of ip below this, see line 876.
> I am a bit surprised that this change fixes anything, since currently
variables
> that need hole-init are not stack allocated

Well, the test case is not really valid code (it re-assigns a const variable),
so it might well be that we get a bit confused. However, I have verified that
this change "fixes" the regression test.

https://codereview.chromium.org/1040703003/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:877: __ str(ip, ContextOperand(cp,
variable->index()));
On 2015/03/29 09:07:04, Benedikt Meurer (OOO) wrote:
> As noted by Dmitry, this will also cause trouble if variable->index() is too
> big.

Done.

[Dmitry Lomov (no reviews), 2015-03-30 11:24:17]

https://codereview.chromium.org/1040703003/diff/20001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-455207.js (right):

https://codereview.chromium.org/1040703003/diff/20001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-455207.js:12: assertThrows(function() { eval(s);
});
Also add a test for non-legacy const or let:


function f() {
  'use strict';
  function g() { return x; }
  assertThrows(g);
  let x = 1;
}
f();

[Michael Starzinger, 2015-08-19 10:51:27]

LGTM. ClusterFuzz also discovered this now. How about we land this fix instead
of having second thoughts for four months? :)

[Jarin, 2015-08-19 11:32:03]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2015-08-19 11:32:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dslomov@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1040703003/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-19 11:32:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1040703003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1040703003/40001

[commit-bot: I haz the power, 2015-08-19 12:50:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-19 12:50:24]

Patchset 3 (id:??) landed as
https://crrev.com/bb869374fe8a9714dba9de218db330f68ca3d4f2
Cr-Commit-Position: refs/heads/master@{#30249}