Out-of-line constant pool on Arm: Stage 1 - Free up r7 for use as constant pool pointer register

Out-of-line constant pool on Arm: Stage 1 - Free up r7 for use as constant pool pointer register

First stage of implementing an out-of-line constant pool on Arm.  This CL
frees up register r7 for use as a constant pool pointer in later stages.

BUG=
R=ulan@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16898

[rmcilroy, 2013-07-29 11:59:19]

This is the first stage of the constant pool work we talked about in the recent
email thread. I've added you as reviewers since you were on that thread, but
please feel free to add other reviews if they would be more appropriate.

I've tested this using arm.check, and all tests pass.  I also ran the v8
benchmarks on an Android device. Is there any other tests I could do, or do you
have try-bots set up which I could send this patch to?

[rmcilroy, 2013-07-29 12:06:53]

I also meant to say, I've created a flag for the out-of-line constant pool, but
at present all it does is prevent / allow lithium to use the r7 register during
it's code-gen.  I thought it would be too confusing and not very useful to gate
the other changes on this flag, but if you feel any should be please let me
know.

Also, I've tried to avoid using the ip register in places where it might get
trashed due to long offsets, but please check I'm not using it inappropriately
where I do use it.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (left):

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:2012: __ Push(r5, r4);
I'm not sure what this push, and the subsequent pop (into different registers)
was meant to be doing. Removing them doesn't seem to cause any problems, but
please let me know if there are any subtle reasons why this should be here.

[JF, 2013-07-29 17:20:09]

I can't say I've done a very good review since I don't know the codebase well
enough. This seems like the right first step toward supporting PP.

Did you try running perf results on golem?

One worry I have is an attacker trampling r7, and I was trying to think of ways
to detect bad code generation that would allow this. Do you think that it would
be practical to teach the simulator or QEMU to detect non-PP writes to r7? Not
for this CL, of course. I'm short on other sanity checks at the moment, there
may be a better way.

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (left):

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc#ol...
src/arm/builtins-arm.cc:551: }
I'm not sure I understand this change.

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc#ol...
src/arm/builtins-arm.cc:758: __ mov(r7, Operand(r4));
This should be conditional on the flag being enabled not, otherwise it may look
like a GC root.

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc#ne...
src/arm/builtins-arm.cc:713: // r5-r6, cp may be clobbered
r7 can still be clobbered if the flag doesn't sequester it.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (left):

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:1764: // r5: Heap number to write answer into.
Update comment.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:1819: use_runtime, gc_required, &not_smis, op, mode,
r6, r4, r5, r9);
It may be more readable to mark the last 4 arguments with a comment saying
they're scratch registers (similar comment at other locations).

[ulan, 2013-07-30 09:45:19]

On high level, this looks like a step in right direction. I have some concerns
with "ip" register usage. The proper review will take some time since I have to
verify that we don't accidentally clobber some registers.

Before that, we should get some estimate on how much performance improvement the
whole approach will give us.

I would suggest to first implement a rough prototype and run benchmarks (I will
send instructions on how to run benchmarks and send try jobs via corp mail). If
it gives convincing performance improvement, then we make a plan on how to land
whole thing in stages.

Looping in Rodoplh from ARM.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (left):

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:2012: __ Push(r5, r4);
On 2013/07/29 12:06:53, rmcilroy wrote:
> I'm not sure what this push, and the subsequent pop (into different registers)
> was meant to be doing. Removing them doesn't seem to cause any problems, but
> please let me know if there are any subtle reasons why this should be here.

Yes, I think the pushes and pops are redundant.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:853: __ vldr(d6, ip, HeapNumber::kValueOffset);
You can fold it into vldr(d6, rhs, HeapNumber::kValueOffset - kHeapObjectTag)
and avoid using ip

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:879: __ vldr(d7, ip, HeapNumber::kValueOffset);
Fold this as above.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:950: __ vldr(d7, r3, HeapNumber::kValueOffset);
Fold these too as above.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:3046: __ mov(ip, Operand(-1));  // Push a bad frame
pointer to fail if it is used.
Move it down to the usage line, otherwise ip might be trashed by intermediate
movs.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:3715: // ip = temporary scratch (a.o., for address
calculation)
This is fragile.

https://codereview.chromium.org/21063002/diff/3001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/21063002/diff/3001/src/arm/stub-cache-arm.cc#...
src/arm/stub-cache-arm.cc:873: __ mov(ip,
Operand(ExternalReference::isolate_address(masm->isolate())));
This mov might use ip internally.

[danno, 2013-07-30 10:34:29]

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:853: __ vldr(d6, ip, HeapNumber::kValueOffset);
On 2013/07/30 09:45:19, ulan wrote:
> You can fold it into vldr(d6, rhs, HeapNumber::kValueOffset - kHeapObjectTag)
> and avoid using ip

Actually, you can't IIRC, the offset to vldr has to be a multiple of four.
That's why we do all this crazy math of pointers before loading or storing
doubles everywhere in the runtime.

[ulan, 2013-07-30 10:47:46]

> Actually, you can't IIRC, the offset to vldr has to be a multiple of four.
> That's why we do all this crazy math of pointers before loading or storing
> doubles everywhere in the runtime.
You're right. If the offset is not a multiple of 4, __ vldr will emit add/sub
using ip as a temp register. So we cannot avoid using ip.

[rmcilroy, 2013-07-30 10:59:03]

On 2013/07/30 10:47:46, ulan wrote:
> > Actually, you can't IIRC, the offset to vldr has to be a multiple of four.
> > That's why we do all this crazy math of pointers before loading or storing
> > doubles everywhere in the runtime.
> You're right. If the offset is not a multiple of 4, __ vldr will emit add/sub
> using ip as a temp register. So we cannot avoid using ip.

In which case, shall I just fold into the vldr instruction as Ulan suggests,
since this will result in the same code anyway and seems more understandable?

[rmcilroy, 2013-07-30 11:39:44]

Thanks for the reviews.  I'll give Golam a go and report back on the performance
numbers.

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (left):

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc#ol...
src/arm/builtins-arm.cc:551: }
On 2013/07/29 17:20:09, JF wrote:
> I'm not sure I understand this change.

Beforehand, the code would only reloaded r7 with Heap::kUndefinedValueRootIndex
if count_constructors was true, otherwise it would already hold
Heap::kUndefinedValueRootIndex from l465 (The debug code seems to be checking
that r7 hasn't been clobbered between these lines).  Now, 
I have to reload Heap::kUndefinedValueRootIndex unconditionally (since I load it
into r6 above, and r6 is clobbered in between).  Therefore, there is no need for
the conditional on count_constructions or the debug code to check that the reg
wasn't clobbered.

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc#ol...
src/arm/builtins-arm.cc:758: __ mov(r7, Operand(r4));
On 2013/07/29 17:20:09, JF wrote:
> This should be conditional on the flag being enabled not, otherwise it may
look
> like a GC root.

Done.

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/21063002/diff/3001/src/arm/builtins-arm.cc#ne...
src/arm/builtins-arm.cc:713: // r5-r6, cp may be clobbered
On 2013/07/29 17:20:09, JF wrote:
> r7 can still be clobbered if the flag doesn't sequester it.

Done.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (left):

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:1764: // r5: Heap number to write answer into.
On 2013/07/29 17:20:09, JF wrote:
> Update comment.

Done.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:2012: __ Push(r5, r4);
On 2013/07/30 09:45:19, ulan wrote:
> On 2013/07/29 12:06:53, rmcilroy wrote:
> > I'm not sure what this push, and the subsequent pop (into different
registers)
> > was meant to be doing. Removing them doesn't seem to cause any problems, but
> > please let me know if there are any subtle reasons why this should be here.
> 
> Yes, I think the pushes and pops are redundant.

Ack

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:853: __ vldr(d6, ip, HeapNumber::kValueOffset);
On 2013/07/30 10:34:29, danno wrote:
> On 2013/07/30 09:45:19, ulan wrote:
> > You can fold it into vldr(d6, rhs, HeapNumber::kValueOffset -
kHeapObjectTag)
> > and avoid using ip
> 
> Actually, you can't IIRC, the offset to vldr has to be a multiple of four.
> That's why we do all this crazy math of pointers before loading or storing
> doubles everywhere in the runtime.

Folded (which causes vldr "instruction" to generate the extra add/subs).

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:879: __ vldr(d7, ip, HeapNumber::kValueOffset);
On 2013/07/30 09:45:19, ulan wrote:
> Fold this as above.

Done.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:950: __ vldr(d7, r3, HeapNumber::kValueOffset);
On 2013/07/30 09:45:19, ulan wrote:
> Fold these too as above.

Done.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:1819: use_runtime, gc_required, &not_smis, op, mode,
r6, r4, r5, r9);
On 2013/07/29 17:20:09, JF wrote:
> It may be more readable to mark the last 4 arguments with a comment saying
> they're scratch registers (similar comment at other locations).

I added them as scratch register arguments, which I think is clearer.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:3046: __ mov(ip, Operand(-1));  // Push a bad frame
pointer to fail if it is used.
On 2013/07/30 09:45:19, ulan wrote:
> Move it down to the usage line, otherwise ip might be trashed by intermediate
> movs.

Done.

https://codereview.chromium.org/21063002/diff/3001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:3715: // ip = temporary scratch (a.o., for address
calculation)
On 2013/07/30 09:45:19, ulan wrote:
> This is fragile.

Replaced use of ip with r0 (and restore r0 after loop).

[Rodolph Perfetta, 2013-07-30 15:19:08]

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:1381: 
assert that left, right, ip, scratch1 and scratch2 are not aliased

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:1605: 
assert the various scratches are not aliased.

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:1804: 
assert scratchx do not alias with left and right.

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:5688: __ tst(ip, Operand(kStringEncodingMask));
if you want to use ip you need  to make sure kStringEncodingMask fits in an
inline arm constant: 8 bits shifted by a even number.

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

https://codereview.chromium.org/21063002/diff/16001/src/arm/full-codegen-arm....
src/arm/full-codegen-arm.cc:4093: __ push(separator);    // Push separator to
reuse register as a scratch.
If the comment above is correct then r4 is available and you don't need to push
onto the stack.

https://codereview.chromium.org/21063002/diff/16001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/21063002/diff/16001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:2312: mov(r3, Operand(next_address));
it looks like r9 is available here. It would save you reloading next_address
later

[rmcilroy, 2013-07-30 17:12:37]

Thanks for the review Rodolph.  In relation to your question about whether this
is purely for security reasons - the current patch is only the first of a number
of stages - the final set of changes should improve security and maintainability
for the constant pool - it probably won't improve performance (though I'm hoping
it won't regress performance).

As for this first step, removing the use of a register obviously won't improve
performance, but I'm hoping not to regress performance if at all possible. 
Thanks to your suggestions this CL should hopefully be closer to performance
neutral now!

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:1381: 
On 2013/07/30 15:19:08, Rodolph Perfetta wrote:
> assert that left, right, ip, scratch1 and scratch2 are not aliased

Done.

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:1605: 
On 2013/07/30 15:19:08, Rodolph Perfetta wrote:
> assert the various scratches are not aliased.

Done.

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:1804: 
On 2013/07/30 15:19:08, Rodolph Perfetta wrote:
> assert scratchx do not alias with left and right.

Done.

https://codereview.chromium.org/21063002/diff/16001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:5688: __ tst(ip, Operand(kStringEncodingMask));
On 2013/07/30 15:19:08, Rodolph Perfetta wrote:
> if you want to use ip you need  to make sure kStringEncodingMask fits in an
> inline arm constant: 8 bits shifted by a even number.

Done.

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

https://codereview.chromium.org/21063002/diff/16001/src/arm/full-codegen-arm....
src/arm/full-codegen-arm.cc:4093: __ push(separator);    // Push separator to
reuse register as a scratch.
On 2013/07/30 15:19:08, Rodolph Perfetta wrote:
> If the comment above is correct then r4 is available and you don't need to
push
> onto the stack.

Nice catch! I spent ages looking for a free reg here, don't know how I missed
r4.

https://codereview.chromium.org/21063002/diff/16001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/21063002/diff/16001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:2312: mov(r3, Operand(next_address));
On 2013/07/30 15:19:08, Rodolph Perfetta wrote:
> it looks like r9 is available here. It would save you reloading next_address
> later

I wasn't entirely sure on whether r9 was allowed here since it's used as a
lithium scratch register, and isn't used by any other MacroAssembler methods.
Doing so didn't seem to cause issues though, so I've made this change assuming
it's ok.

[ulan, 2013-08-06 12:29:09]

LGTM

https://codereview.chromium.org/21063002/diff/26001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/21063002/diff/26001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:2389: counters->transcendental_cache_hit(), 1,
scratch0, scratch1);
We're using scratch1 and r4 close to one another. Maybe use explicit range:

scratch1 = no_reg
...
scratch1 = r4?

https://codereview.chromium.org/21063002/diff/26001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:3715: // r4 = address of parameter map (tagged)
r4 = address of parameter map (tagged) and
r4 = address of new object + Heap::kArgumentsObjectSize (tagged)

https://codereview.chromium.org/21063002/diff/26001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:3732: __ sub(r0, r4,
Operand(Heap::kArgumentsObjectSize));
Please add a comment that we are restoring r0.

[rmcilroy, 2013-08-06 13:23:25]

Rebased and addressed Ulan's comments.

https://codereview.chromium.org/21063002/diff/26001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/21063002/diff/26001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:2389: counters->transcendental_cache_hit(), 1,
scratch0, scratch1);
On 2013/08/06 12:29:09, ulan wrote:
> We're using scratch1 and r4 close to one another. Maybe use explicit range:
> 
> scratch1 = no_reg
> ...
> scratch1 = r4?

Done.

https://codereview.chromium.org/21063002/diff/26001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:3715: // r4 = address of parameter map (tagged)
On 2013/08/06 12:29:09, ulan wrote:
> r4 = address of parameter map (tagged) and
> r4 = address of new object + Heap::kArgumentsObjectSize (tagged)

Done.

https://codereview.chromium.org/21063002/diff/26001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:3732: __ sub(r0, r4,
Operand(Heap::kArgumentsObjectSize));
On 2013/08/06 12:29:09, ulan wrote:
> Please add a comment that we are restoring r0.

Done.

[ulan, 2013-09-10 11:45:31]

Ross, could you rebase it again? I will land it for you.

[rmcilroy, 2013-09-12 14:15:10]

Rebased and up to date.  Please land if you are happy with this Ulan.

Also, when this lands, could you pass the word around the Munich office to avoid
using register r7 in arm assembly from now on please.

[rmcilroy, 2013-09-12 15:36:12]

On 2013/09/12 14:15:10, rmcilroy wrote:
> Rebased and up to date.  Please land if you are happy with this Ulan.
> 
> Also, when this lands, could you pass the word around the Munich office to
avoid
> using register r7 in arm assembly from now on please.

I could, but r8 (which is used as the context register) and r10 (roots register)
aren't renamed, so didn't want to do this here.  Ulan / Daniel, do you have any
thoughts on doing this?

[ulan, 2013-09-13 08:52:18]

On 2013/09/12 15:36:12, rmcilroy wrote:
> On 2013/09/12 14:15:10, rmcilroy wrote:
> > Rebased and up to date.  Please land if you are happy with this Ulan.
> > 
> > Also, when this lands, could you pass the word around the Munich office to
> avoid
> > using register r7 in arm assembly from now on please.
> 
> I could, but r8 (which is used as the context register) and r10 (roots
register)
> aren't renamed, so didn't want to do this here.  Ulan / Daniel, do you have
any
> thoughts on doing this?
Let's be consistent with r8 for now.

I ran tests before landing and got this failure:

v8/out/android_arm.release/d8 --test --stress-opt --always-opt
--nobreak-on-abort --nodead-code-elimination --nofold-constants
v8/test/mjsunit/mjsunit.js v8/test/mjsunit/md5.js
============ Stress 1/5 ============
============ Stress 2/5 ============
============ Stress 3/5 ============
============ Stress 4/5 ============
/data/local/tmp/v8/test/mjsunit/mjsunit.js:174: Failure: expected
<"9de517715e52d2ea967d2e47058b7b12"> found <"1b8719c72d5d8bfd06e096ef6c6288c5">
    throw new MjsUnitAssertionError(message);
          ^

The base revision r16687 ran successfully. But the test might be flaky. I am
looking into the test.

[rmcilroy_google, 2013-09-13 09:47:03]

On 2013/09/13 08:52:18, ulan wrote:
> On 2013/09/12 15:36:12, rmcilroy wrote:
> > On 2013/09/12 14:15:10, rmcilroy wrote:
> > > Rebased and up to date.  Please land if you are happy with this Ulan.
> > > 
> > > Also, when this lands, could you pass the word around the Munich office to
> > avoid
> > > using register r7 in arm assembly from now on please.
> > 
> > I could, but r8 (which is used as the context register) and r10 (roots
> register)
> > aren't renamed, so didn't want to do this here.  Ulan / Daniel, do you have
> any
> > thoughts on doing this?
> Let's be consistent with r8 for now.
> 
> I ran tests before landing and got this failure:
> 
> v8/out/android_arm.release/d8 --test --stress-opt --always-opt
> --nobreak-on-abort --nodead-code-elimination --nofold-constants
> v8/test/mjsunit/mjsunit.js v8/test/mjsunit/md5.js
> ============ Stress 1/5 ============
> ============ Stress 2/5 ============
> ============ Stress 3/5 ============
> ============ Stress 4/5 ============
> /data/local/tmp/v8/test/mjsunit/mjsunit.js:174: Failure: expected
> <"9de517715e52d2ea967d2e47058b7b12"> found
<"1b8719c72d5d8bfd06e096ef6c6288c5">
>     throw new MjsUnitAssertionError(message);
>           ^
> 
> The base revision r16687 ran successfully. But the test might be flaky. I am
> looking into the test.

Hmm, weird, I can't repo this on my Android device or on the arm simulator. 
Which device are you testing this on?

[ulan, 2013-09-13 09:49:42]

On 2013/09/13 09:47:03, rmcilroy_google wrote:
> On 2013/09/13 08:52:18, ulan wrote:
> > On 2013/09/12 15:36:12, rmcilroy wrote:
> > > On 2013/09/12 14:15:10, rmcilroy wrote:
> > > > Rebased and up to date.  Please land if you are happy with this Ulan.
> > > > 
> > > > Also, when this lands, could you pass the word around the Munich office
to
> > > avoid
> > > > using register r7 in arm assembly from now on please.
> > > 
> > > I could, but r8 (which is used as the context register) and r10 (roots
> > register)
> > > aren't renamed, so didn't want to do this here.  Ulan / Daniel, do you
have
> > any
> > > thoughts on doing this?
> > Let's be consistent with r8 for now.
> > 
> > I ran tests before landing and got this failure:
> > 
> > v8/out/android_arm.release/d8 --test --stress-opt --always-opt
> > --nobreak-on-abort --nodead-code-elimination --nofold-constants
> > v8/test/mjsunit/mjsunit.js v8/test/mjsunit/md5.js
> > ============ Stress 1/5 ============
> > ============ Stress 2/5 ============
> > ============ Stress 3/5 ============
> > ============ Stress 4/5 ============
> > /data/local/tmp/v8/test/mjsunit/mjsunit.js:174: Failure: expected
> > <"9de517715e52d2ea967d2e47058b7b12"> found
> <"1b8719c72d5d8bfd06e096ef6c6288c5">
> >     throw new MjsUnitAssertionError(message);
> >           ^
> > 
> > The base revision r16687 ran successfully. But the test might be flaky. I am
> > looking into the test.
> 
> Hmm, weird, I can't repo this on my Android device or on the arm simulator. 
> Which device are you testing this on?

I test on nexus 4. It seem to depend on random hash seed baked into snapshot.
After rebuilding, I cannot repro anymore (with the old build it failed on every
run). Will try different seeds. It might be the problem with ToT, not this CL.

[ulan, 2013-09-13 12:06:26]

https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:503: r9,
This can clobber r9, so the use below seems to be invalid.

[rmcilroy_google, 2013-09-13 12:16:43]

https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:503: r9,
On 2013/09/13 12:06:27, ulan wrote:
> This can clobber r9, so the use below seems to be invalid.

But this is never called on the same codepath as below (L496 never falls through
since it's an unconditional branch, so the only entry point to "only_change_map"
is the branch on L443, and the code block immediately branches to "done" on
L508, so it never hits the use of r9 from L486-L494 and L516-L540).

[ulan, 2013-09-13 12:55:48]

On 2013/09/13 12:16:43, rmcilroy_google wrote:
> https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc
> File src/arm/codegen-arm.cc (right):
> 
>
https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc#ne...
> src/arm/codegen-arm.cc:503: r9,
> On 2013/09/13 12:06:27, ulan wrote:
> > This can clobber r9, so the use below seems to be invalid.
> 
> But this is never called on the same codepath as below (L496 never falls
through
> since it's an unconditional branch, so the only entry point to
"only_change_map"
> is the branch on L443, and the code block immediately branches to "done" on
> L508, so it never hits the use of r9 from L486-L494 and L516-L540).

You're right. There must be a bug somewhere else. I can reproduce the flaky test
failure when running 5 instances in a loop for ~10 minutes.

[rmcilroy, 2013-09-13 13:45:37]

On 2013/09/13 12:55:48, ulan wrote:
> On 2013/09/13 12:16:43, rmcilroy_google wrote:
> > https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc
> > File src/arm/codegen-arm.cc (right):
> > 
> >
>
https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc#ne...
> > src/arm/codegen-arm.cc:503: r9,
> > On 2013/09/13 12:06:27, ulan wrote:
> > > This can clobber r9, so the use below seems to be invalid.
> > 
> > But this is never called on the same codepath as below (L496 never falls
> through
> > since it's an unconditional branch, so the only entry point to
> "only_change_map"
> > is the branch on L443, and the code block immediately branches to "done" on
> > L508, so it never hits the use of r9 from L486-L494 and L516-L540).
> 
> You're right. There must be a bug somewhere else. I can reproduce the flaky
test
> failure when running 5 instances in a loop for ~10 minutes.

Can you send me the command lines you are using to run the tests and I'll try
and repo it and look into it too.

[ulan, 2013-09-13 13:58:22]

> Can you send me the command lines you are using to run
Yep, sent them via chat.

I think I found it:

https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:484: __ ldr(r4, FieldMemOperand(r2,
JSObject::kElementsOffset));
FieldMemOperand(r2, JSObject::kElementsOffset) is overwritten above in line 473.

[Rodolph Perfetta, 2013-09-13 14:14:40]

DBC

https://codereview.chromium.org/21063002/diff/82001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/21063002/diff/82001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:1344: mov(cp, Operand(Smi::FromInt(0)));  //
Indicates no context.
I don't know if this is related to your test flakyness, but here I don't believe
you can corrupt the context pointer. Look in full-codegen-arm.cc when
PushTryHandler is used

https://codereview.chromium.org/21063002/diff/82001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.h (right):

https://codereview.chromium.org/21063002/diff/82001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.h:48: const Register pp = { 7 };  // Constant pool
pointer.
nit: why don't you use kRegister_r7_Code like in assembler-arm.h? It makes it
easier to look for r7 aliases.

[ulan, 2013-09-13 14:20:47]

https://codereview.chromium.org/21063002/diff/82001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/21063002/diff/82001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:484: __ ldr(r4, FieldMemOperand(r2,
JSObject::kElementsOffset));
Moving this up to line 454 seems to have fixed the flaky failure.

https://codereview.chromium.org/21063002/diff/82001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/21063002/diff/82001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:1344: mov(cp, Operand(Smi::FromInt(0)));  //
Indicates no context.
On 2013/09/13 14:14:40, Rodolph Perfetta wrote:
> I don't know if this is related to your test flakyness, but here I don't
believe
> you can corrupt the context pointer. Look in full-codegen-arm.cc when
> PushTryHandler is used
I also had this suspicion, but this case seems to be fine because PushTryHandler
with kind = JS_ENTRY is called only in JSEntryStub::GenerateBody and cp is later
overwrite in JSEntryTrampoline.

[rmcilroy_google, 2013-09-13 14:52:57]

Thanks for finding that issue Ulan! It does seem to have fixed it (I've been
running your crash script for 10mins and no crashes, where before it crashed
after 2mins).

https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/21063002/diff/68001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:484: __ ldr(r4, FieldMemOperand(r2,
JSObject::kElementsOffset));
On 2013/09/13 13:58:22, ulan wrote:
> FieldMemOperand(r2, JSObject::kElementsOffset) is overwritten above in line
473.

Ahh, you're right, nice spot!  Fixed by moving up.

https://codereview.chromium.org/21063002/diff/82001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/21063002/diff/82001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:484: __ ldr(r4, FieldMemOperand(r2,
JSObject::kElementsOffset));
On 2013/09/13 14:20:48, ulan wrote:
> Moving this up to line 454 seems to have fixed the flaky failure.

Done.

https://codereview.chromium.org/21063002/diff/82001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.h (right):

https://codereview.chromium.org/21063002/diff/82001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.h:48: const Register pp = { 7 };  // Constant pool
pointer.
On 2013/09/13 14:14:40, Rodolph Perfetta wrote:
> nit: why don't you use kRegister_r7_Code like in assembler-arm.h? It makes it
> easier to look for r7 aliases.

Done (and done for cp and kRootRegister below).

[rmcilroy_google, 2013-09-13 14:52:58]

Thanks for finding that issue Ulan! It does seem to have fixed it (I've been
running your crash script for 10mins and no crashes, where before it crashed
after 2mins).

[ulan, 2013-09-23 12:57:13]

I am going to land this. Got conflict during rebase and changed register:

https://chromiumcodereview.appspot.com/21063002/diff/94001/src/arm/stub-cache...
File src/arm/stub-cache-arm.cc (right):

https://chromiumcodereview.appspot.com/21063002/diff/94001/src/arm/stub-cache...
src/arm/stub-cache-arm.cc:877: __ mov(r5,
Operand(ExternalReference::isolate_address(masm->isolate())));
Changed this to r5 from ip, since r5 is reloaded later.

[ulan, 2013-09-23 15:01:50]

Committed patchset #12 manually as r16898 (presubmit successful).