Fix issues with Arm's use of embedded constant pools

Fix issues with Arm's use of embedded constant pools

- Introduce Assembler::DataAlign for table alignment in code object
- Fix several misuses of r8 (alias of the pool pointer register, pp)
- Fix calculation of pp in OSR/handler entry invocation
- Enable missing cases in deserializer
- Fix references to ool constant pools in comments.

R=rmcilroy@chromium.org, michael_dawson@ca.ibm.com
BUG=chromium:497180
LOG=N

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

[rmcilroy, 2015-06-08 11:11:11]

Thanks for the fixes! A couple of comments.

https://codereview.chromium.org/1155673005/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1155673005/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:500: }
I'm not keen on adding this to Align since it makes it possible to insert
non-instructions into the bytestream if something goes wrong with alignment.
Instead, please add a different DataAlign() function (if you could also add a
DCHECK here that pc_offset() is 4 byte aligned that would be great).

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

https://codereview.chromium.org/1155673005/diff/1/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:1359: __ RecordWriteField(map_load_offset,
Cell::kValueOffset, value_scratch,
Could we just use "scratch" here? It looks like it's not needed between 1347 and
1369 (I could be wrong though).

https://codereview.chromium.org/1155673005/diff/1/src/arm/frames-arm.h
File src/arm/frames-arm.h (right):

https://codereview.chromium.org/1155673005/diff/1/src/arm/frames-arm.h#newcode75
src/arm/frames-arm.h:75: : kCalleeSaved);
I'm guessing you are doing this because pp holds a non-tagged pointer? If so,
I'm wondering if there is a better way to do this - kCalleeSaved also includes
the fp which also holds a non-tagged pointer - could you avoid removing the pp
register from kSafepointSavedRegisters somehow by following what's happening for
fp?

[MTBrandyberry, 2015-06-08 13:52:39]

Responses.  Re-testing now.

https://codereview.chromium.org/1155673005/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1155673005/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:500: }
On 2015/06/08 11:11:10, rmcilroy wrote:
> I'm not keen on adding this to Align since it makes it possible to insert
> non-instructions into the bytestream if something goes wrong with alignment.
> Instead, please add a different DataAlign() function (if you could also add a
> DCHECK here that pc_offset() is 4 byte aligned that would be great).

Done.

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

https://codereview.chromium.org/1155673005/diff/1/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:1359: __ RecordWriteField(map_load_offset,
Cell::kValueOffset, value_scratch,
On 2015/06/08 11:11:10, rmcilroy wrote:
> Could we just use "scratch" here? It looks like it's not needed between 1347
and
> 1369 (I could be wrong though).

Good catch. Done.

https://codereview.chromium.org/1155673005/diff/1/src/arm/frames-arm.h
File src/arm/frames-arm.h (right):

https://codereview.chromium.org/1155673005/diff/1/src/arm/frames-arm.h#newcode75
src/arm/frames-arm.h:75: : kCalleeSaved);
On 2015/06/08 11:11:11, rmcilroy wrote:
> I'm guessing you are doing this because pp holds a non-tagged pointer? If so,
> I'm wondering if there is a better way to do this - kCalleeSaved also includes
> the fp which also holds a non-tagged pointer - could you avoid removing the pp
> register from kSafepointSavedRegisters somehow by following what's happening
for
> fp? 

Partly.  It's true that pp doesn't need to be saved in the safepoint since it
isn't tagged.  More important, however, is that pp is saved in the stack frame
where its value will be updated if GC code movement occurs.  Allowing it to
remain in the safepoint causes a potentially stale value to be restored.

fp does not have this same consideration and is currently included in the
safepoint.

[rmcilroy, 2015-06-09 10:10:46]

https://codereview.chromium.org/1155673005/diff/1/src/arm/frames-arm.h
File src/arm/frames-arm.h (right):

https://codereview.chromium.org/1155673005/diff/1/src/arm/frames-arm.h#newcode75
src/arm/frames-arm.h:75: : kCalleeSaved);
On 2015/06/08 13:52:39, mtbrandyberry wrote:
> On 2015/06/08 11:11:11, rmcilroy wrote:
> > I'm guessing you are doing this because pp holds a non-tagged pointer? If
so,
> > I'm wondering if there is a better way to do this - kCalleeSaved also
includes
> > the fp which also holds a non-tagged pointer - could you avoid removing the
pp
> > register from kSafepointSavedRegisters somehow by following what's happening
> for
> > fp? 
> 
> Partly.  It's true that pp doesn't need to be saved in the safepoint since it
> isn't tagged.  More important, however, is that pp is saved in the stack frame
> where its value will be updated if GC code movement occurs.  Allowing it to
> remain in the safepoint causes a potentially stale value to be restored.
> 
> fp does not have this same consideration and is currently included in the
> safepoint.

OK, but let's make this a bit easier to understand. Could you pull out some
constants to make this:

const RegList kSafepointSavedRegisters = kJSCallerSaved | kCalleeSaved &
~kConstantPointerRegMask;
const int kNumSafepointSavedRegisters = kNumJSCallerSaved + kNumCalleeSaved -
kNumConstantPoolPointerReg;

or similar.

[MTBrandyberry, 2015-06-09 13:26:38]

Uploaded patch set 3.

https://codereview.chromium.org/1155673005/diff/1/src/arm/frames-arm.h
File src/arm/frames-arm.h (right):

https://codereview.chromium.org/1155673005/diff/1/src/arm/frames-arm.h#newcode75
src/arm/frames-arm.h:75: : kCalleeSaved);
On 2015/06/09 10:10:46, rmcilroy wrote:
> On 2015/06/08 13:52:39, mtbrandyberry wrote:
> > On 2015/06/08 11:11:11, rmcilroy wrote:
> > > I'm guessing you are doing this because pp holds a non-tagged pointer? If
> so,
> > > I'm wondering if there is a better way to do this - kCalleeSaved also
> includes
> > > the fp which also holds a non-tagged pointer - could you avoid removing
the
> pp
> > > register from kSafepointSavedRegisters somehow by following what's
happening
> > for
> > > fp? 
> > 
> > Partly.  It's true that pp doesn't need to be saved in the safepoint since
it
> > isn't tagged.  More important, however, is that pp is saved in the stack
frame
> > where its value will be updated if GC code movement occurs.  Allowing it to
> > remain in the safepoint causes a potentially stale value to be restored.
> > 
> > fp does not have this same consideration and is currently included in the
> > safepoint.
> 
> OK, but let's make this a bit easier to understand. Could you pull out some
> constants to make this:
> 
> const RegList kSafepointSavedRegisters = kJSCallerSaved | kCalleeSaved &
> ~kConstantPointerRegMask;
> const int kNumSafepointSavedRegisters = kNumJSCallerSaved + kNumCalleeSaved -
> kNumConstantPoolPointerReg;
> 
> or similar.

Done.

[rmcilroy, 2015-06-09 14:14:43]

A couple of last comments. LGTM once they are addressed.

https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:713: ((1 << kNumSafepointSavedRegisters) - 1) ==
kSafepointSavedRegisters);
Please add a DCHECK here for what is expected when
FLAG_enable_embedded_constant_pool is true (preferably in the same DCHECK)

https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.cc
File src/ppc/assembler-ppc.cc (right):

https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.c...
src/ppc/assembler-ppc.cc:255: void Assembler::DataAlign(int m) {
These are all the same aren't they? Couldn't you just implement this in the
arch-independent assembler.cc?

[MTBrandyberry, 2015-06-09 14:19:08]

https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:713: ((1 << kNumSafepointSavedRegisters) - 1) ==
kSafepointSavedRegisters);
On 2015/06/09 14:14:43, rmcilroy wrote:
> Please add a DCHECK here for what is expected when
> FLAG_enable_embedded_constant_pool is true (preferably in the same DCHECK)

OK

https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.cc
File src/ppc/assembler-ppc.cc (right):

https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.c...
src/ppc/assembler-ppc.cc:255: void Assembler::DataAlign(int m) {
On 2015/06/09 14:14:43, rmcilroy wrote:
> These are all the same aren't they? Couldn't you just implement this in the
> arch-independent assembler.cc?

I didn't see a precedent for services like this in AssemblerBase.  I'd be happy
to put it there.

[MTBrandyberry, 2015-06-09 14:23:38]

On 2015/06/09 14:19:08, mtbrandyberry wrote:
>
https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
> File src/arm/macro-assembler-arm.cc (right):
> 
>
https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
> src/arm/macro-assembler-arm.cc:713: ((1 << kNumSafepointSavedRegisters) - 1)
==
> kSafepointSavedRegisters);
> On 2015/06/09 14:14:43, rmcilroy wrote:
> > Please add a DCHECK here for what is expected when
> > FLAG_enable_embedded_constant_pool is true (preferably in the same DCHECK)
> 
> OK
> 
> https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.cc
> File src/ppc/assembler-ppc.cc (right):
> 
>
https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.c...
> src/ppc/assembler-ppc.cc:255: void Assembler::DataAlign(int m) {
> On 2015/06/09 14:14:43, rmcilroy wrote:
> > These are all the same aren't they? Couldn't you just implement this in the
> > arch-independent assembler.cc?
> 
> I didn't see a precedent for services like this in AssemblerBase.  I'd be
happy
> to put it there.

Or are you just suggesting that it stay in Assembler, but the implementation be
moved to src/assembler.cc?

[MTBrandyberry, 2015-06-09 15:18:38]

The CQ bit was checked by mbrandy@us.ibm.com

[MTBrandyberry, 2015-06-09 15:18:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1155673005/#ps80001
(title: "Move DataAlign implementation to common code")

[commit-bot: I haz the power, 2015-06-09 15:18:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155673005/80001

[rmcilroy, 2015-06-09 15:20:02]

On 2015/06/09 14:23:38, mtbrandyberry wrote:
> On 2015/06/09 14:19:08, mtbrandyberry wrote:
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
> > File src/arm/macro-assembler-arm.cc (right):
> > 
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
> > src/arm/macro-assembler-arm.cc:713: ((1 << kNumSafepointSavedRegisters) - 1)
> ==
> > kSafepointSavedRegisters);
> > On 2015/06/09 14:14:43, rmcilroy wrote:
> > > Please add a DCHECK here for what is expected when
> > > FLAG_enable_embedded_constant_pool is true (preferably in the same DCHECK)
> > 
> > OK
> > 
> >
https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.cc
> > File src/ppc/assembler-ppc.cc (right):
> > 
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.c...
> > src/ppc/assembler-ppc.cc:255: void Assembler::DataAlign(int m) {
> > On 2015/06/09 14:14:43, rmcilroy wrote:
> > > These are all the same aren't they? Couldn't you just implement this in
the
> > > arch-independent assembler.cc?
> > 
> > I didn't see a precedent for services like this in AssemblerBase.  I'd be
> happy
> > to put it there.
> 
> Or are you just suggesting that it stay in Assembler, but the implementation
be
> moved to src/assembler.cc?

Actually maybe moving it to AssemblerBase would be a bit strange since you're
right there is no president for it. Could we just implement it as a helper in
ConstantPoolBuilder to avoid reimplementing it everywhere (I think
ConstantPoolBuilder should have enough access to call assm->pc_offset() and
assm->db() which should be enough).

[MTBrandyberry, 2015-06-09 15:34:50]

On 2015/06/09 15:20:02, rmcilroy wrote:
> On 2015/06/09 14:23:38, mtbrandyberry wrote:
> > On 2015/06/09 14:19:08, mtbrandyberry wrote:
> > >
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
> > > File src/arm/macro-assembler-arm.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
> > > src/arm/macro-assembler-arm.cc:713: ((1 << kNumSafepointSavedRegisters) -
1)
> > ==
> > > kSafepointSavedRegisters);
> > > On 2015/06/09 14:14:43, rmcilroy wrote:
> > > > Please add a DCHECK here for what is expected when
> > > > FLAG_enable_embedded_constant_pool is true (preferably in the same
DCHECK)
> > > 
> > > OK
> > > 
> > >
> https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.cc
> > > File src/ppc/assembler-ppc.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.c...
> > > src/ppc/assembler-ppc.cc:255: void Assembler::DataAlign(int m) {
> > > On 2015/06/09 14:14:43, rmcilroy wrote:
> > > > These are all the same aren't they? Couldn't you just implement this in
> the
> > > > arch-independent assembler.cc?
> > > 
> > > I didn't see a precedent for services like this in AssemblerBase.  I'd be
> > happy
> > > to put it there.
> > 
> > Or are you just suggesting that it stay in Assembler, but the implementation
> be
> > moved to src/assembler.cc?
> 
> Actually maybe moving it to AssemblerBase would be a bit strange since you're
> right there is no president for it. Could we just implement it as a helper in
> ConstantPoolBuilder to avoid reimplementing it everywhere (I think
> ConstantPoolBuilder should have enough access to call assm->pc_offset() and
> assm->db() which should be enough).

There is a set of routines with common implementations defined in assembler.cc. 
I added it there prior to seeing your response.  I guess it depends on whether
we think DataAlign will be useful outside of the constant pool builder.

[rmcilroy, 2015-06-09 15:41:04]

On 2015/06/09 15:34:50, mtbrandyberry wrote:
> On 2015/06/09 15:20:02, rmcilroy wrote:
> > On 2015/06/09 14:23:38, mtbrandyberry wrote:
> > > On 2015/06/09 14:19:08, mtbrandyberry wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
> > > > File src/arm/macro-assembler-arm.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/arm/macro-assembler...
> > > > src/arm/macro-assembler-arm.cc:713: ((1 << kNumSafepointSavedRegisters)
-
> 1)
> > > ==
> > > > kSafepointSavedRegisters);
> > > > On 2015/06/09 14:14:43, rmcilroy wrote:
> > > > > Please add a DCHECK here for what is expected when
> > > > > FLAG_enable_embedded_constant_pool is true (preferably in the same
> DCHECK)
> > > > 
> > > > OK
> > > > 
> > > >
> >
https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.cc
> > > > File src/ppc/assembler-ppc.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1155673005/diff/60001/src/ppc/assembler-ppc.c...
> > > > src/ppc/assembler-ppc.cc:255: void Assembler::DataAlign(int m) {
> > > > On 2015/06/09 14:14:43, rmcilroy wrote:
> > > > > These are all the same aren't they? Couldn't you just implement this
in
> > the
> > > > > arch-independent assembler.cc?
> > > > 
> > > > I didn't see a precedent for services like this in AssemblerBase.  I'd
be
> > > happy
> > > > to put it there.
> > > 
> > > Or are you just suggesting that it stay in Assembler, but the
implementation
> > be
> > > moved to src/assembler.cc?
> > 
> > Actually maybe moving it to AssemblerBase would be a bit strange since
you're
> > right there is no president for it. Could we just implement it as a helper
in
> > ConstantPoolBuilder to avoid reimplementing it everywhere (I think
> > ConstantPoolBuilder should have enough access to call assm->pc_offset() and
> > assm->db() which should be enough).
> 
> There is a set of routines with common implementations defined in
assembler.cc. 
> I added it there prior to seeing your response.  I guess it depends on whether
> we think DataAlign will be useful outside of the constant pool builder.

This works too. LGTM.

[commit-bot: I haz the power, 2015-06-09 16:00:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-09 16:01:07]

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