MIPS: Fix 'MIPS: Serializer: serialize internal references via object visitor.'

MIPS: Fix 'MIPS: Serializer: serialize internal references via object visitor.'

BUG=

Committed: https://crrev.com/56d2ee0310972119ec47810ee03a4f7077f7117e
Cr-Commit-Position: refs/heads/master@{#27351}

[paul.l..., 2015-03-19 18:35:42]

paul.lind@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, balazs.kilvady@imgtec.com, danno@chromium.org,
dusan.milosavljevic@imgtec.com, paul.lind@imgtec.com, yangguo@chromium.org

[paul.l..., 2015-03-19 18:35:43]

LGTM, but please fix the nits. 

Yang, can we get your review also please? We can't derive 'encoded' state of
internal ref from the data or address, we have to pass it thru the serializer.
I've not looked at PPC port to see if this is true for them too. I will try to
catch up on that.

https://codereview.chromium.org/1025453003/diff/1/src/mips64/assembler-mips64...
File src/mips64/assembler-mips64-inl.h (right):

https://codereview.chromium.org/1025453003/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64-inl.h:199: // Encoded internal references are
lui/ori load of 48-bit abolute address.
typo, from my original, s/b: absolute. You might fix the original typo too (new
line 274). Also in the assembler-mips-inl.h

https://codereview.chromium.org/1025453003/diff/1/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/1025453003/diff/1/src/serialize.h#newcode321
src/serialize.h:321: // 0x17        Unused (including 0x37, 0x57, 0x77).
Code 0x17 is now used. I suggest you call this Misc, since that is how code 1e
is labeled & handled.

[balazs.kilvady, 2015-03-19 18:56:29]

https://codereview.chromium.org/1025453003/diff/1/src/mips64/assembler-mips64...
File src/mips64/assembler-mips64-inl.h (right):

https://codereview.chromium.org/1025453003/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64-inl.h:199: // Encoded internal references are
lui/ori load of 48-bit abolute address.
On 2015/03/19 18:35:42, paul.l... wrote:
> typo, from my original, s/b: absolute. You might fix the original typo too
(new
> line 274). Also in the assembler-mips-inl.h

Done.

https://codereview.chromium.org/1025453003/diff/1/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/1025453003/diff/1/src/serialize.h#newcode321
src/serialize.h:321: // 0x17        Unused (including 0x37, 0x57, 0x77).
On 2015/03/19 18:35:42, paul.l... wrote:
> Code 0x17 is now used. I suggest you call this Misc, since that is how code 1e
> is labeled & handled.

Done.

[paul.l..., 2015-03-19 19:00:12]

not LGTM. We need arch ports.

[yangguo, 2015-03-19 19:16:32]

yangguo@google.com changed reviewers:
+ yangguo@google.com

[yangguo, 2015-03-19 19:16:33]

Sorry about this! I overlooked this. Introducing a new byte code is the right
thing. However, like Paul mentioned, you need to implement the newly introduced
deserialize_... method on other platforms as well, as empty methods.

And I got some comments.

https://codereview.chromium.org/1025453003/diff/20001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/1025453003/diff/20001/src/serialize.cc#newcod...
src/serialize.cc:1837: rinfo->rmode() == RelocInfo::INTERNAL_REFERENCE
Please do not use a tertiary operator like this. I definitely prefer

sink_->Put(rinfo->rmode() == ... ? ... : ..., "InternalRef");

Or an if-else if you want to preserve the comment strings.

https://codereview.chromium.org/1025453003/diff/20001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/1025453003/diff/20001/src/serialize.h#newcode321
src/serialize.h:321: // 0x17        Misc (including 0x37, 0x57, 0x77).
Please do not use this byte code. I'd like to reserve this in case we add more
object spaces again. Instead, please use 0x1f below.

[paul.l..., 2015-03-19 19:22:25]

On 2015/03/19 19:16:33, yangguo wrote:
> you need to implement the newly introduced
> deserialize_... method on other platforms as well, as empty methods.

We are now planning to add a flag/enum parameter to the existing method
deserialization_set_target_internal_reference_at(), rather than add new method
to every arch. It will still require changes on all arch's. Do you have a
preference on this?

[yangguo, 2015-03-19 19:27:44]

On 2015/03/19 19:22:25, paul.l... wrote:
> On 2015/03/19 19:16:33, yangguo wrote:
> > you need to implement the newly introduced
> > deserialize_... method on other platforms as well, as empty methods.
> 
> We are now planning to add a flag/enum parameter to the existing method
> deserialization_set_target_internal_reference_at(), rather than add new method
> to every arch. It will still require changes on all arch's. Do you have a
> preference on this?

A enum actually sounds nicer. Please do that. You could actually just use
RelocInfo::Mode for that enum.

[balazs.kilvady, 2015-03-20 11:14:42]

https://codereview.chromium.org/1025453003/diff/20001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/1025453003/diff/20001/src/serialize.cc#newcod...
src/serialize.cc:1837: rinfo->rmode() == RelocInfo::INTERNAL_REFERENCE
On 2015/03/19 19:16:33, yangguo wrote:
> Please do not use a tertiary operator like this. I definitely prefer
> 
> sink_->Put(rinfo->rmode() == ... ? ... : ..., "InternalRef");
> 
> Or an if-else if you want to preserve the comment strings.

Done.

https://codereview.chromium.org/1025453003/diff/20001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/1025453003/diff/20001/src/serialize.h#newcode321
src/serialize.h:321: // 0x17        Misc (including 0x37, 0x57, 0x77).
On 2015/03/19 19:16:33, yangguo wrote:
> Please do not use this byte code. I'd like to reserve this in case we add more
> object spaces again. Instead, please use 0x1f below.

Done.

[balazs.kilvady, 2015-03-20 15:54:38]

Yang could you take a look, please if u have time?

[paul.l..., 2015-03-20 16:55:46]

Balázs, there is one nit. I do like how you factored out the common code in the
mips ports. I also see that x87 and ppc have not yet ported
https://codereview.chromium.org/1005183006, so we can't add these changes to
those platform ports. LGTM.

https://codereview.chromium.org/1025453003/diff/40001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/1025453003/diff/40001/src/serialize.h#newcode333
src/serialize.h:333: // 0x1f        Unused (including 0x3f, 0x5f, 0x7f).
This should now be marked as Misc

[Yang, 2015-03-20 17:02:49]

On 2015/03/20 16:55:46, paul.l... wrote:
> Balázs, there is one nit. I do like how you factored out the common code in
the
> mips ports. I also see that x87 and ppc have not yet ported
> https://codereview.chromium.org/1005183006, so we can't add these changes to
> those platform ports. LGTM.
> 
> https://codereview.chromium.org/1025453003/diff/40001/src/serialize.h
> File src/serialize.h (right):
> 
>
https://codereview.chromium.org/1025453003/diff/40001/src/serialize.h#newcode333
> src/serialize.h:333: // 0x1f        Unused (including 0x3f, 0x5f, 0x7f).
> This should now be marked as Misc

LGTM.

[balazs.kilvady, 2015-03-20 17:13:50]

https://codereview.chromium.org/1025453003/diff/40001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/1025453003/diff/40001/src/serialize.h#newcode333
src/serialize.h:333: // 0x1f        Unused (including 0x3f, 0x5f, 0x7f).
On 2015/03/20 16:55:46, paul.l... wrote:
> This should now be marked as Misc

Done.

[balazs.kilvady, 2015-03-20 17:13:59]

The CQ bit was checked by balazs.kilvady@imgtec.com

[balazs.kilvady, 2015-03-20 17:13:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1025453003/#ps60001
(title: "Fix comment.")

[commit-bot: I haz the power, 2015-03-20 17:14:18]

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

[commit-bot: I haz the power, 2015-03-23 07:00:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-23 07:00:30]

Patchset 4 (id:??) landed as
https://crrev.com/56d2ee0310972119ec47810ee03a4f7077f7117e
Cr-Commit-Position: refs/heads/master@{#27351}