Assembler changes for enabling GrowHeap in Wasm

Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=

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

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

[gdeepti1, 2016-03-03 01:36:07]

gdeepti@google.com changed reviewers:
+ bradnelson@chromium.org, titzer@chromium.org, yangguo@chromium.org

[Yang, 2016-03-03 06:21:56]

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode389
src/assembler.h:389: // objects
Can you change the comment to say "To relocate pointers into the wasm heap,
embedded in wasm code"? All reloc info are markers of some sort, so that part is
redundant.

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode517
src/assembler.h:517: static inline bool IsWasmCodeEntry(Mode mode) {
This name is misleading. I would have thought that this would be a pointer to
the entry of a wasm code object, instead into the wasm heap. How about
"IsWasmHeapReference"?

https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
File src/ia32/assembler-ia32-inl.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32-inl.h:105: void RelocInfo::set_target_address(Address
target,
Can we create a separate method to do the wasm heap address patching? This
method, as the name states, is for call targets. It may contain the same code,
yes, but overloading this method to deal with wasm heap addresses is going to
cause a lot of confusion. Maybe call the new method set_wasm_heap_reference or
something.

https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:274: inline explicit Immediate(int x, RelocInfo::Mode
rmode);
If I understand correctly, we use this to embed an address into the wasm heap
with this, right? Can we use the signation Immediate(Address, RelocInfo::Mode)
here? Otherwise you are just going to invite abuse.

https://codereview.chromium.org/1759873002/diff/40001/src/x64/macro-assembler...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/1759873002/diff/40001/src/x64/macro-assembler...
src/x64/macro-assembler-x64.cc:1119: xorl(dst, dst);
so if x == 0, we ignore the relocation mode?

https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
File test/cctest/test-wasm-relocation-disasm-arm.cc (right):

https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
test/cctest/test-wasm-relocation-disasm-arm.cc:81:
reinterpret_cast<byte*>(new_buffer) - reinterpret_cast<byte*>(buffer);
I don't see why the offset between buffers matters here. What you want to test
is that you can patch a new immediate into the code. Why not just do that?

https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
test/cctest/test-wasm-relocation-disasm-arm.cc:88:
it.rinfo()->set_target_address(it.rinfo()->target_address() + offset,
just write something new into here, like a imm_new = 7654321.

[gdeepti1, 2016-03-03 18:00:10]

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode389
src/assembler.h:389: // objects
On 2016/03/03 06:21:56, Yang wrote:
> Can you change the comment to say "To relocate pointers into the wasm heap,
> embedded in wasm code"? All reloc info are markers of some sort, so that part
is
> redundant.

Done.

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode517
src/assembler.h:517: static inline bool IsWasmCodeEntry(Mode mode) {
On 2016/03/03 06:21:56, Yang wrote:
> This name is misleading. I would have thought that this would be a pointer to
> the entry of a wasm code object, instead into the wasm heap. How about
> "IsWasmHeapReference"?

Name was left over from a previous RelocInfo mode I was using, fixed to be
IsWasmHeapReference.

https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:274: inline explicit Immediate(int x, RelocInfo::Mode
rmode);
On 2016/03/03 06:21:56, Yang wrote:
> If I understand correctly, we use this to embed an address into the wasm heap
> with this, right? Can we use the signation Immediate(Address, RelocInfo::Mode)
> here? Otherwise you are just going to invite abuse.

Done.

[titzer, 2016-03-03 18:11:59]

https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
src/arm/assembler-arm-inl.h:69: Address RelocInfo::target_address() {
I think we should add new methods to deal specifically with wasm memory
references, as Yang had suggested elsewhere.

https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
src/arm/assembler-arm-inl.h:104: void RelocInfo::set_target_address(Address
target,
For the wasm memory reference version of this, I think we should have an
"update" style method, where the caller has to pass the old address and old
size. That's because we'll have memory references into the middle of the wasm
memory (i.e. with some non-zero offset from the start). We don't have to do that
case in this CL, but that will happen in a later one.

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode390
src/assembler.h:390: WASM_HEAP_OBJECT,
Can we name this WASM_MEMORY_REFERENCE?

We've been trying to avoid using the term "heap" to refer to the wasm memory.

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode517
src/assembler.h:517: static inline bool IsWasmCodeEntry(Mode mode) {
On 2016/03/03 18:00:10, gdeepti1 wrote:
> On 2016/03/03 06:21:56, Yang wrote:
> > This name is misleading. I would have thought that this would be a pointer
to
> > the entry of a wasm code object, instead into the wasm heap. How about
> > "IsWasmHeapReference"?
> 
> Name was left over from a previous RelocInfo mode I was using, fixed to be
> IsWasmHeapReference.

As above, IsWasmMemoryReference, for consistency.

https://codereview.chromium.org/1759873002/diff/40001/src/x64/macro-assembler...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/1759873002/diff/40001/src/x64/macro-assembler...
src/x64/macro-assembler-x64.cc:1119: xorl(dst, dst);
On 2016/03/03 06:21:56, Yang wrote:
> so if x == 0, we ignore the relocation mode?

I think we shouldn't do any smartness here, since the relocation mode indicates
we might patch the value to be an arbitrary value later.

[gdeepti1, 2016-03-03 22:59:16]

https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
src/arm/assembler-arm-inl.h:69: Address RelocInfo::target_address() {
On 2016/03/03 18:11:59, titzer wrote:
> I think we should add new methods to deal specifically with wasm memory
> references, as Yang had suggested elsewhere.

Done.

https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
src/arm/assembler-arm-inl.h:104: void RelocInfo::set_target_address(Address
target,
On 2016/03/03 18:11:59, titzer wrote:
> For the wasm memory reference version of this, I think we should have an
> "update" style method, where the caller has to pass the old address and old
> size. That's because we'll have memory references into the middle of the wasm
> memory (i.e. with some non-zero offset from the start). We don't have to do
that
> case in this CL, but that will happen in a later one.

Added a wasm memory reference version for now. I'll add the changes for updating
references in a follow up CL.

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode390
src/assembler.h:390: WASM_HEAP_OBJECT,
On 2016/03/03 18:11:59, titzer wrote:
> Can we name this WASM_MEMORY_REFERENCE?
> 
> We've been trying to avoid using the term "heap" to refer to the wasm memory.

Done.

https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
File src/ia32/assembler-ia32-inl.h (right):

https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32-inl.h:105: void RelocInfo::set_target_address(Address
target,
On 2016/03/03 06:21:56, Yang wrote:
> Can we create a separate method to do the wasm heap address patching? This
> method, as the name states, is for call targets. It may contain the same code,
> yes, but overloading this method to deal with wasm heap addresses is going to
> cause a lot of confusion. Maybe call the new method set_wasm_heap_reference or
> something.

Done.

https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
File test/cctest/test-wasm-relocation-disasm-arm.cc (right):

https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
test/cctest/test-wasm-relocation-disasm-arm.cc:81:
reinterpret_cast<byte*>(new_buffer) - reinterpret_cast<byte*>(buffer);
On 2016/03/03 06:21:56, Yang wrote:
> I don't see why the offset between buffers matters here. What you want to test
> is that you can patch a new immediate into the code. Why not just do that?

The context I guess is updating references by an offset when the memory buffers
for wasm are changed and that all the references are updated by the correct
offset. I see why it doesn't make as much sense here. Updated it to get rid of
the buffers.

https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
test/cctest/test-wasm-relocation-disasm-arm.cc:88:
it.rinfo()->set_target_address(it.rinfo()->target_address() + offset,
On 2016/03/03 06:21:56, Yang wrote:
> just write something new into here, like a imm_new = 7654321.

references updated by an offset for now.

[gdeepti1, 2016-03-04 01:09:49]

Patchset #7 (id:120001) has been deleted

[Yang, 2016-03-07 06:37:42]

On 2016/03/03 22:59:16, gdeepti1 wrote:
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> File src/arm/assembler-arm-inl.h (right):
> 
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> src/arm/assembler-arm-inl.h:69: Address RelocInfo::target_address() {
> On 2016/03/03 18:11:59, titzer wrote:
> > I think we should add new methods to deal specifically with wasm memory
> > references, as Yang had suggested elsewhere.
> 
> Done.
> 
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> src/arm/assembler-arm-inl.h:104: void RelocInfo::set_target_address(Address
> target,
> On 2016/03/03 18:11:59, titzer wrote:
> > For the wasm memory reference version of this, I think we should have an
> > "update" style method, where the caller has to pass the old address and old
> > size. That's because we'll have memory references into the middle of the
wasm
> > memory (i.e. with some non-zero offset from the start). We don't have to do
> that
> > case in this CL, but that will happen in a later one.
> 
> Added a wasm memory reference version for now. I'll add the changes for
updating
> references in a follow up CL.
> 
> https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
> File src/assembler.h (right):
> 
>
https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode390
> src/assembler.h:390: WASM_HEAP_OBJECT,
> On 2016/03/03 18:11:59, titzer wrote:
> > Can we name this WASM_MEMORY_REFERENCE?
> > 
> > We've been trying to avoid using the term "heap" to refer to the wasm
memory.
> 
> Done.
> 
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> File src/ia32/assembler-ia32-inl.h (right):
> 
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> src/ia32/assembler-ia32-inl.h:105: void RelocInfo::set_target_address(Address
> target,
> On 2016/03/03 06:21:56, Yang wrote:
> > Can we create a separate method to do the wasm heap address patching? This
> > method, as the name states, is for call targets. It may contain the same
code,
> > yes, but overloading this method to deal with wasm heap addresses is going
to
> > cause a lot of confusion. Maybe call the new method set_wasm_heap_reference
or
> > something.
> 
> Done.
> 
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> File test/cctest/test-wasm-relocation-disasm-arm.cc (right):
> 
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> test/cctest/test-wasm-relocation-disasm-arm.cc:81:
> reinterpret_cast<byte*>(new_buffer) - reinterpret_cast<byte*>(buffer);
> On 2016/03/03 06:21:56, Yang wrote:
> > I don't see why the offset between buffers matters here. What you want to
test
> > is that you can patch a new immediate into the code. Why not just do that?
> 
> The context I guess is updating references by an offset when the memory
buffers
> for wasm are changed and that all the references are updated by the correct
> offset. I see why it doesn't make as much sense here. Updated it to get rid of
> the buffers.
> 
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> test/cctest/test-wasm-relocation-disasm-arm.cc:88:
> it.rinfo()->set_target_address(it.rinfo()->target_address() + offset,
> On 2016/03/03 06:21:56, Yang wrote:
> > just write something new into here, like a imm_new = 7654321.
> 
> references updated by an offset for now.

Hasn't this CL been replaced by now? Can you close it?

[gdeepti1, 2016-03-07 06:48:46]

On 2016/03/07 06:37:42, Yang wrote:
> On 2016/03/03 22:59:16, gdeepti1 wrote:
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > File src/arm/assembler-arm-inl.h (right):
> > 
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > src/arm/assembler-arm-inl.h:69: Address RelocInfo::target_address() {
> > On 2016/03/03 18:11:59, titzer wrote:
> > > I think we should add new methods to deal specifically with wasm memory
> > > references, as Yang had suggested elsewhere.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > src/arm/assembler-arm-inl.h:104: void RelocInfo::set_target_address(Address
> > target,
> > On 2016/03/03 18:11:59, titzer wrote:
> > > For the wasm memory reference version of this, I think we should have an
> > > "update" style method, where the caller has to pass the old address and
old
> > > size. That's because we'll have memory references into the middle of the
> wasm
> > > memory (i.e. with some non-zero offset from the start). We don't have to
do
> > that
> > > case in this CL, but that will happen in a later one.
> > 
> > Added a wasm memory reference version for now. I'll add the changes for
> updating
> > references in a follow up CL.
> > 
> > https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
> > File src/assembler.h (right):
> > 
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode390
> > src/assembler.h:390: WASM_HEAP_OBJECT,
> > On 2016/03/03 18:11:59, titzer wrote:
> > > Can we name this WASM_MEMORY_REFERENCE?
> > > 
> > > We've been trying to avoid using the term "heap" to refer to the wasm
> memory.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> > File src/ia32/assembler-ia32-inl.h (right):
> > 
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> > src/ia32/assembler-ia32-inl.h:105: void
RelocInfo::set_target_address(Address
> > target,
> > On 2016/03/03 06:21:56, Yang wrote:
> > > Can we create a separate method to do the wasm heap address patching? This
> > > method, as the name states, is for call targets. It may contain the same
> code,
> > > yes, but overloading this method to deal with wasm heap addresses is going
> to
> > > cause a lot of confusion. Maybe call the new method
set_wasm_heap_reference
> or
> > > something.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > File test/cctest/test-wasm-relocation-disasm-arm.cc (right):
> > 
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > test/cctest/test-wasm-relocation-disasm-arm.cc:81:
> > reinterpret_cast<byte*>(new_buffer) - reinterpret_cast<byte*>(buffer);
> > On 2016/03/03 06:21:56, Yang wrote:
> > > I don't see why the offset between buffers matters here. What you want to
> test
> > > is that you can patch a new immediate into the code. Why not just do that?
> > 
> > The context I guess is updating references by an offset when the memory
> buffers
> > for wasm are changed and that all the references are updated by the correct
> > offset. I see why it doesn't make as much sense here. Updated it to get rid
of
> > the buffers.
> > 
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > test/cctest/test-wasm-relocation-disasm-arm.cc:88:
> > it.rinfo()->set_target_address(it.rinfo()->target_address() + offset,
> > On 2016/03/03 06:21:56, Yang wrote:
> > > just write something new into here, like a imm_new = 7654321.
> > 
> > references updated by an offset for now.
> 
> Hasn't this CL been replaced by now? Can you close it?

I was planning to keep the assembler and compiler changes separate. If this
change lands, I can merge to keep the other one just for the compiler changes.

[Yang, 2016-03-07 06:50:13]

On 2016/03/07 06:48:46, gdeepti1 wrote:
> On 2016/03/07 06:37:42, Yang wrote:
> > On 2016/03/03 22:59:16, gdeepti1 wrote:
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > File src/arm/assembler-arm-inl.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > src/arm/assembler-arm-inl.h:69: Address RelocInfo::target_address() {
> > > On 2016/03/03 18:11:59, titzer wrote:
> > > > I think we should add new methods to deal specifically with wasm memory
> > > > references, as Yang had suggested elsewhere.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > src/arm/assembler-arm-inl.h:104: void
RelocInfo::set_target_address(Address
> > > target,
> > > On 2016/03/03 18:11:59, titzer wrote:
> > > > For the wasm memory reference version of this, I think we should have an
> > > > "update" style method, where the caller has to pass the old address and
> old
> > > > size. That's because we'll have memory references into the middle of the
> > wasm
> > > > memory (i.e. with some non-zero offset from the start). We don't have to
> do
> > > that
> > > > case in this CL, but that will happen in a later one.
> > > 
> > > Added a wasm memory reference version for now. I'll add the changes for
> > updating
> > > references in a follow up CL.
> > > 
> > > https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
> > > File src/assembler.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode390
> > > src/assembler.h:390: WASM_HEAP_OBJECT,
> > > On 2016/03/03 18:11:59, titzer wrote:
> > > > Can we name this WASM_MEMORY_REFERENCE?
> > > > 
> > > > We've been trying to avoid using the term "heap" to refer to the wasm
> > memory.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> > > File src/ia32/assembler-ia32-inl.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> > > src/ia32/assembler-ia32-inl.h:105: void
> RelocInfo::set_target_address(Address
> > > target,
> > > On 2016/03/03 06:21:56, Yang wrote:
> > > > Can we create a separate method to do the wasm heap address patching?
This
> > > > method, as the name states, is for call targets. It may contain the same
> > code,
> > > > yes, but overloading this method to deal with wasm heap addresses is
going
> > to
> > > > cause a lot of confusion. Maybe call the new method
> set_wasm_heap_reference
> > or
> > > > something.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > File test/cctest/test-wasm-relocation-disasm-arm.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > test/cctest/test-wasm-relocation-disasm-arm.cc:81:
> > > reinterpret_cast<byte*>(new_buffer) - reinterpret_cast<byte*>(buffer);
> > > On 2016/03/03 06:21:56, Yang wrote:
> > > > I don't see why the offset between buffers matters here. What you want
to
> > test
> > > > is that you can patch a new immediate into the code. Why not just do
that?
> > > 
> > > The context I guess is updating references by an offset when the memory
> > buffers
> > > for wasm are changed and that all the references are updated by the
correct
> > > offset. I see why it doesn't make as much sense here. Updated it to get
rid
> of
> > > the buffers.
> > > 
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > test/cctest/test-wasm-relocation-disasm-arm.cc:88:
> > > it.rinfo()->set_target_address(it.rinfo()->target_address() + offset,
> > > On 2016/03/03 06:21:56, Yang wrote:
> > > > just write something new into here, like a imm_new = 7654321.
> > > 
> > > references updated by an offset for now.
> > 
> > Hasn't this CL been replaced by now? Can you close it?
> 
> I was planning to keep the assembler and compiler changes separate. If this
> change lands, I can merge to keep the other one just for the compiler changes.

The CL description is outdated. And please take a look at my comments in the
other CL.

[Yang, 2016-03-07 06:51:09]

On 2016/03/07 06:50:13, Yang wrote:
> On 2016/03/07 06:48:46, gdeepti1 wrote:
> > On 2016/03/07 06:37:42, Yang wrote:
> > > On 2016/03/03 22:59:16, gdeepti1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > > File src/arm/assembler-arm-inl.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > > src/arm/assembler-arm-inl.h:69: Address RelocInfo::target_address() {
> > > > On 2016/03/03 18:11:59, titzer wrote:
> > > > > I think we should add new methods to deal specifically with wasm
memory
> > > > > references, as Yang had suggested elsewhere.
> > > > 
> > > > Done.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > > src/arm/assembler-arm-inl.h:104: void
> RelocInfo::set_target_address(Address
> > > > target,
> > > > On 2016/03/03 18:11:59, titzer wrote:
> > > > > For the wasm memory reference version of this, I think we should have
an
> > > > > "update" style method, where the caller has to pass the old address
and
> > old
> > > > > size. That's because we'll have memory references into the middle of
the
> > > wasm
> > > > > memory (i.e. with some non-zero offset from the start). We don't have
to
> > do
> > > > that
> > > > > case in this CL, but that will happen in a later one.
> > > > 
> > > > Added a wasm memory reference version for now. I'll add the changes for
> > > updating
> > > > references in a follow up CL.
> > > > 
> > > > https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
> > > > File src/assembler.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode390
> > > > src/assembler.h:390: WASM_HEAP_OBJECT,
> > > > On 2016/03/03 18:11:59, titzer wrote:
> > > > > Can we name this WASM_MEMORY_REFERENCE?
> > > > > 
> > > > > We've been trying to avoid using the term "heap" to refer to the wasm
> > > memory.
> > > > 
> > > > Done.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> > > > File src/ia32/assembler-ia32-inl.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> > > > src/ia32/assembler-ia32-inl.h:105: void
> > RelocInfo::set_target_address(Address
> > > > target,
> > > > On 2016/03/03 06:21:56, Yang wrote:
> > > > > Can we create a separate method to do the wasm heap address patching?
> This
> > > > > method, as the name states, is for call targets. It may contain the
same
> > > code,
> > > > > yes, but overloading this method to deal with wasm heap addresses is
> going
> > > to
> > > > > cause a lot of confusion. Maybe call the new method
> > set_wasm_heap_reference
> > > or
> > > > > something.
> > > > 
> > > > Done.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > > File test/cctest/test-wasm-relocation-disasm-arm.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > > test/cctest/test-wasm-relocation-disasm-arm.cc:81:
> > > > reinterpret_cast<byte*>(new_buffer) - reinterpret_cast<byte*>(buffer);
> > > > On 2016/03/03 06:21:56, Yang wrote:
> > > > > I don't see why the offset between buffers matters here. What you want
> to
> > > test
> > > > > is that you can patch a new immediate into the code. Why not just do
> that?
> > > > 
> > > > The context I guess is updating references by an offset when the memory
> > > buffers
> > > > for wasm are changed and that all the references are updated by the
> correct
> > > > offset. I see why it doesn't make as much sense here. Updated it to get
> rid
> > of
> > > > the buffers.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > > test/cctest/test-wasm-relocation-disasm-arm.cc:88:
> > > > it.rinfo()->set_target_address(it.rinfo()->target_address() + offset,
> > > > On 2016/03/03 06:21:56, Yang wrote:
> > > > > just write something new into here, like a imm_new = 7654321.
> > > > 
> > > > references updated by an offset for now.
> > > 
> > > Hasn't this CL been replaced by now? Can you close it?
> > 
> > I was planning to keep the assembler and compiler changes separate. If this
> > change lands, I can merge to keep the other one just for the compiler
changes.
> 
> The CL description is outdated. And please take a look at my comments in the
> other CL.

Btw you could have branched off this CL when creating that other one. That way
you would have gotten a CL that is dependent on this one, but does not include
the redundant changes.

[titzer, 2016-03-07 19:39:24]

https://codereview.chromium.org/1759873002/diff/140001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1759873002/diff/140001/src/assembler.h#newcod...
src/assembler.h:388: // To relocate pointers into the wasm heap embedded in wasm
code
s/wasm heap/wasm memory/

https://codereview.chromium.org/1759873002/diff/140001/src/assembler.h#newcod...
src/assembler.h:580: INLINE(void update_wasm_memory_reference(
As per previous comments, let's do an update interface like:

void update_wasm_memory_reference(
  Address old_base,
  Address new_base,
  size_t old_size,
  size_t new_size,
  ICacheFlushMode mode)

Then we can keep the relative calculation hidden inside and even assert that the
old_base was within the old bounds.

[gdeepti1, 2016-03-08 03:38:02]

On 2016/03/07 06:51:09, Yang wrote:
> On 2016/03/07 06:50:13, Yang wrote:
> > On 2016/03/07 06:48:46, gdeepti1 wrote:
> > > On 2016/03/07 06:37:42, Yang wrote:
> > > > On 2016/03/03 22:59:16, gdeepti1 wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > > > File src/arm/assembler-arm-inl.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > > > src/arm/assembler-arm-inl.h:69: Address RelocInfo::target_address() {
> > > > > On 2016/03/03 18:11:59, titzer wrote:
> > > > > > I think we should add new methods to deal specifically with wasm
> memory
> > > > > > references, as Yang had suggested elsewhere.
> > > > > 
> > > > > Done.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/arm/assembler-arm-i...
> > > > > src/arm/assembler-arm-inl.h:104: void
> > RelocInfo::set_target_address(Address
> > > > > target,
> > > > > On 2016/03/03 18:11:59, titzer wrote:
> > > > > > For the wasm memory reference version of this, I think we should
have
> an
> > > > > > "update" style method, where the caller has to pass the old address
> and
> > > old
> > > > > > size. That's because we'll have memory references into the middle of
> the
> > > > wasm
> > > > > > memory (i.e. with some non-zero offset from the start). We don't
have
> to
> > > do
> > > > > that
> > > > > > case in this CL, but that will happen in a later one.
> > > > > 
> > > > > Added a wasm memory reference version for now. I'll add the changes
for
> > > > updating
> > > > > references in a follow up CL.
> > > > > 
> > > > > https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h
> > > > > File src/assembler.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/assembler.h#newcode390
> > > > > src/assembler.h:390: WASM_HEAP_OBJECT,
> > > > > On 2016/03/03 18:11:59, titzer wrote:
> > > > > > Can we name this WASM_MEMORY_REFERENCE?
> > > > > > 
> > > > > > We've been trying to avoid using the term "heap" to refer to the
wasm
> > > > memory.
> > > > > 
> > > > > Done.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> > > > > File src/ia32/assembler-ia32-inl.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/src/ia32/assembler-ia32...
> > > > > src/ia32/assembler-ia32-inl.h:105: void
> > > RelocInfo::set_target_address(Address
> > > > > target,
> > > > > On 2016/03/03 06:21:56, Yang wrote:
> > > > > > Can we create a separate method to do the wasm heap address
patching?
> > This
> > > > > > method, as the name states, is for call targets. It may contain the
> same
> > > > code,
> > > > > > yes, but overloading this method to deal with wasm heap addresses is
> > going
> > > > to
> > > > > > cause a lot of confusion. Maybe call the new method
> > > set_wasm_heap_reference
> > > > or
> > > > > > something.
> > > > > 
> > > > > Done.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > > > File test/cctest/test-wasm-relocation-disasm-arm.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > > > test/cctest/test-wasm-relocation-disasm-arm.cc:81:
> > > > > reinterpret_cast<byte*>(new_buffer) - reinterpret_cast<byte*>(buffer);
> > > > > On 2016/03/03 06:21:56, Yang wrote:
> > > > > > I don't see why the offset between buffers matters here. What you
want
> > to
> > > > test
> > > > > > is that you can patch a new immediate into the code. Why not just do
> > that?
> > > > > 
> > > > > The context I guess is updating references by an offset when the
memory
> > > > buffers
> > > > > for wasm are changed and that all the references are updated by the
> > correct
> > > > > offset. I see why it doesn't make as much sense here. Updated it to
get
> > rid
> > > of
> > > > > the buffers.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1759873002/diff/40001/test/cctest/test-wasm-r...
> > > > > test/cctest/test-wasm-relocation-disasm-arm.cc:88:
> > > > > it.rinfo()->set_target_address(it.rinfo()->target_address() + offset,
> > > > > On 2016/03/03 06:21:56, Yang wrote:
> > > > > > just write something new into here, like a imm_new = 7654321.
> > > > > 
> > > > > references updated by an offset for now.
> > > > 
> > > > Hasn't this CL been replaced by now? Can you close it?
> > > 
> > > I was planning to keep the assembler and compiler changes separate. If
this
> > > change lands, I can merge to keep the other one just for the compiler
> changes.
> > 
> > The CL description is outdated. And please take a look at my comments in the
> > other CL.
> 
> Btw you could have branched off this CL when creating that other one. That way
> you would have gotten a CL that is dependent on this one, but does not include
> the redundant changes.

Thanks for the pointer, I've done that now so that the compiler CL doesn't
include the assembler changes.

[gdeepti1, 2016-03-08 03:38:51]

https://codereview.chromium.org/1759873002/diff/140001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1759873002/diff/140001/src/assembler.h#newcod...
src/assembler.h:388: // To relocate pointers into the wasm heap embedded in wasm
code
On 2016/03/07 19:39:23, titzer wrote:
> s/wasm heap/wasm memory/

Done.

https://codereview.chromium.org/1759873002/diff/140001/src/assembler.h#newcod...
src/assembler.h:580: INLINE(void update_wasm_memory_reference(
On 2016/03/07 19:39:23, titzer wrote:
> As per previous comments, let's do an update interface like:
> 
> void update_wasm_memory_reference(
>   Address old_base,
>   Address new_base,
>   size_t old_size,
>   size_t new_size,
>   ICacheFlushMode mode)
> 
> Then we can keep the relative calculation hidden inside and even assert that
the
> old_base was within the old bounds.

Misunderstood previous review comment, updated the interface now as above with
additional checks.

[Yang, 2016-03-08 05:41:50]

https://codereview.chromium.org/1759873002/diff/180001/src/arm64/assembler-ar...
File src/arm64/assembler-arm64-inl.h (right):

https://codereview.chromium.org/1759873002/diff/180001/src/arm64/assembler-ar...
src/arm64/assembler-arm64-inl.h:52: Assembler::set_target_address_at(isolate_,
pc_, host_, updated_reference,
There doesn't seem to be anything platform dependent here. Would it be possible
to move this to src/assembler.cc?

https://codereview.chromium.org/1759873002/diff/180001/src/x64/macro-assemble...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/1759873002/diff/180001/src/x64/macro-assemble...
src/x64/macro-assembler-x64.cc:1121: movl(dst,
Immediate(static_cast<int32_t>(x), rmode));
Can you elaborate on where this is being used? I see at least two problems with
this:
- If x is 0, then no reloc info is emitted. There will be no way to use reloc
info to find this place again, and even if we can find it, there is no way to
replace the xorl with something other than setting dst to 0. I already noted
this previously.
- If x is below 32 bits, there will be no way to set it to a larger value
anymore.
All of this looks like premature optimization, and you even had to introduce a
new Immediate constructor for this.

[gdeepti1, 2016-03-08 06:03:21]

Description was changed from

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_HEAP_OBJECT as a marker for wasm code objects that
need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=
==========

to

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=
==========

[gdeepti1, 2016-03-09 04:01:43]

https://codereview.chromium.org/1759873002/diff/180001/src/arm64/assembler-ar...
File src/arm64/assembler-arm64-inl.h (right):

https://codereview.chromium.org/1759873002/diff/180001/src/arm64/assembler-ar...
src/arm64/assembler-arm64-inl.h:52: Assembler::set_target_address_at(isolate_,
pc_, host_, updated_reference,
On 2016/03/08 05:41:50, Yang wrote:
> There doesn't seem to be anything platform dependent here. Would it be
possible
> to move this to src/assembler.cc?

Added it in platform specific headers because that seemed to be the case for all
other INLINE functions defined in assembler.h. Would you prefer that it was
changed to a regular function and moved to assembler.cc?

https://codereview.chromium.org/1759873002/diff/180001/src/x64/macro-assemble...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/1759873002/diff/180001/src/x64/macro-assemble...
src/x64/macro-assembler-x64.cc:1121: movl(dst,
Immediate(static_cast<int32_t>(x), rmode));
On 2016/03/08 05:41:50, Yang wrote:
> Can you elaborate on where this is being used? I see at least two problems
with
> this:
> - If x is 0, then no reloc info is emitted. There will be no way to use reloc
> info to find this place again, and even if we can find it, there is no way to
> replace the xorl with something other than setting dst to 0. I already noted
> this previously.
> - If x is below 32 bits, there will be no way to set it to a larger value
> anymore.
> All of this looks like premature optimization, and you even had to introduce a
> new Immediate constructor for this.

The specific case this is being used for is when in AssembleMove in the x64 code
generator when source is a constant with RelocInfo::Mode WASM_MEMORY_REFERENCE.
As the immediate in this case should be a pointer constant within the bounds of
the wasm memory buffer, it should realistically never be 0. 
 - If x is 0, I agree it is non intuitive to silently drop relocation mode that
is being passed in. Fixed to remove special case. 
 - For the specific case of wasm memory references I'm assuming it would be
possible to update to a larger value even if the current value is below 32 bits
on an update. Changed to use movq. For cases where relocation information is
anything else, this function mirrors the Set(Register dst, int64_t x) above this
function and is modified only to record relocation mode. 

The new Immediate constructor is used to be able to pass in relocation mode from
the compiler to the assembler, other ways to pass it to the assembler and record
relocation mode seemed messy to me.

[gdeepti1, 2016-03-11 03:55:51]

https://codereview.chromium.org/1759873002/diff/180001/src/arm64/assembler-ar...
File src/arm64/assembler-arm64-inl.h (right):

https://codereview.chromium.org/1759873002/diff/180001/src/arm64/assembler-ar...
src/arm64/assembler-arm64-inl.h:52: Assembler::set_target_address_at(isolate_,
pc_, host_, updated_reference,
On 2016/03/09 04:01:43, gdeepti1 wrote:
> On 2016/03/08 05:41:50, Yang wrote:
> > There doesn't seem to be anything platform dependent here. Would it be
> possible
> > to move this to src/assembler.cc?
> 
> Added it in platform specific headers because that seemed to be the case for
all
> other INLINE functions defined in assembler.h. Would you prefer that it was
> changed to a regular function and moved to assembler.cc? 

Update from previous comment - the bounds checking in this function was
incorrect for ia32/x64 because target_address_at/set_target_address_at methods
use pc relative addresses. Fixed in the latest patch, this does make the
implementation slightly different for ia32/x64 vs arm/arm64.

[bradn, 2016-03-13 18:20:58]

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

[bradn, 2016-03-13 18:20:58]

ping?

[titzer, 2016-03-14 15:23:39]

Looking mostly good.

https://codereview.chromium.org/1759873002/diff/220001/src/arm/assembler-arm-...
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/1759873002/diff/220001/src/arm/assembler-arm-...
src/arm/assembler-arm-inl.h:125: DCHECK(old_size <= new_size);
No need to assert this, I think.

https://codereview.chromium.org/1759873002/diff/220001/src/arm/assembler-arm-...
src/arm/assembler-arm-inl.h:128: Address updated_reference =
wasm_memory_reference() + (new_base - old_base);
This might read a bit clearer as:

new_base + (wasm_memory_reference() - old_base)

https://codereview.chromium.org/1759873002/diff/220001/src/arm64/assembler-ar...
File src/arm64/assembler-arm64-inl.h (right):

https://codereview.chromium.org/1759873002/diff/220001/src/arm64/assembler-ar...
src/arm64/assembler-arm64-inl.h:48: DCHECK(old_size <= new_size);
Same comments as on arm.

https://codereview.chromium.org/1759873002/diff/220001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/1759873002/diff/220001/src/x64/assembler-x64....
src/x64/assembler-x64.h:337: explicit Immediate(int32_t value, RelocInfo::Mode
rmode)
AFAICT we don't need this until we have updateable lengths, which will be 32 bit
quantities. Can we do this change in a separate CL?

https://codereview.chromium.org/1759873002/diff/220001/src/x64/macro-assemble...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/1759873002/diff/220001/src/x64/macro-assemble...
src/x64/macro-assembler-x64.cc:1122: movl(dst,
Immediate(static_cast<int32_t>(x), rmode));
You probably want to DCHECK that there is no relocation mode in this case?

https://codereview.chromium.org/1759873002/diff/220001/test/cctest/test-wasm-...
File test/cctest/test-wasm-relocation-disasm-arm.cc (right):

https://codereview.chromium.org/1759873002/diff/220001/test/cctest/test-wasm-...
test/cctest/test-wasm-relocation-disasm-arm.cc:1: // Copyright 2016 the V8
project authors. All rights reserved.
Can we rename this file to be closer to what it does? It runs the code, so maybe
"test-run-wasm-relocation-arm64.cc"?

Also, can you use the shorter copyright header?

[gdeepti1, 2016-03-14 20:08:14]

https://codereview.chromium.org/1759873002/diff/220001/src/arm/assembler-arm-...
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/1759873002/diff/220001/src/arm/assembler-arm-...
src/arm/assembler-arm-inl.h:125: DCHECK(old_size <= new_size);
On 2016/03/14 15:23:38, titzer wrote:
> No need to assert this, I think.

Done.

https://codereview.chromium.org/1759873002/diff/220001/src/arm/assembler-arm-...
src/arm/assembler-arm-inl.h:128: Address updated_reference =
wasm_memory_reference() + (new_base - old_base);
On 2016/03/14 15:23:38, titzer wrote:
> This might read a bit clearer as:
> 
> new_base + (wasm_memory_reference() - old_base)

Done.

https://codereview.chromium.org/1759873002/diff/220001/src/arm64/assembler-ar...
File src/arm64/assembler-arm64-inl.h (right):

https://codereview.chromium.org/1759873002/diff/220001/src/arm64/assembler-ar...
src/arm64/assembler-arm64-inl.h:48: DCHECK(old_size <= new_size);
On 2016/03/14 15:23:38, titzer wrote:
> Same comments as on arm. 

Fixed for all architectures.

https://codereview.chromium.org/1759873002/diff/220001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/1759873002/diff/220001/src/x64/assembler-x64....
src/x64/assembler-x64.h:337: explicit Immediate(int32_t value, RelocInfo::Mode
rmode)
On 2016/03/14 15:23:38, titzer wrote:
> AFAICT we don't need this until we have updateable lengths, which will be 32
bit
> quantities. Can we do this change in a separate CL?

Sounds good, removed from this CL.

https://codereview.chromium.org/1759873002/diff/220001/src/x64/macro-assemble...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/1759873002/diff/220001/src/x64/macro-assemble...
src/x64/macro-assembler-x64.cc:1122: movl(dst,
Immediate(static_cast<int32_t>(x), rmode));
On 2016/03/14 15:23:38, titzer wrote:
> You probably want to DCHECK that there is no relocation mode in this case?

Done.

https://codereview.chromium.org/1759873002/diff/220001/test/cctest/test-wasm-...
File test/cctest/test-wasm-relocation-disasm-arm.cc (right):

https://codereview.chromium.org/1759873002/diff/220001/test/cctest/test-wasm-...
test/cctest/test-wasm-relocation-disasm-arm.cc:1: // Copyright 2016 the V8
project authors. All rights reserved.
On 2016/03/14 15:23:38, titzer wrote:
> Can we rename this file to be closer to what it does? It runs the code, so
maybe
> "test-run-wasm-relocation-arm64.cc"?
> 
> Also, can you use the shorter copyright header?

Done.

[Yang, 2016-03-15 07:44:18]

LGTM if comments are addressed.

https://codereview.chromium.org/1759873002/diff/240001/src/arm/assembler-arm-...
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/1759873002/diff/240001/src/arm/assembler-arm-...
src/arm/assembler-arm-inl.h:126: wasm_memory_reference() <= old_base +
old_size);
Should this really be a "less-equals" or just a "less"? I can't imagine the
reference pointing to outside the memory (which old_base + old_size would be).

Same for the compare below and on other platform ports.

https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64....
src/x64/assembler-x64.h:342: RelocInfo::Mode rmode() { return rmode_; }
This cannot be anything other than NONE32. Why do we need this change?

https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64....
src/x64/assembler-x64.h:1753: if (!RelocInfo::IsNone(x.rmode_)) {
This seems like dead code.

[gdeepti1, 2016-03-16 06:05:26]

https://codereview.chromium.org/1759873002/diff/240001/src/arm/assembler-arm-...
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/1759873002/diff/240001/src/arm/assembler-arm-...
src/arm/assembler-arm-inl.h:126: wasm_memory_reference() <= old_base +
old_size);
On 2016/03/15 07:44:18, Yang wrote:
> Should this really be a "less-equals" or just a "less"? I can't imagine the
> reference pointing to outside the memory (which old_base + old_size would be).
> 
> Same for the compare below and on other platform ports.

Fixed below and on other platform ports.

https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64....
src/x64/assembler-x64.h:342: RelocInfo::Mode rmode() { return rmode_; }
On 2016/03/15 07:44:18, Yang wrote:
> This cannot be anything other than NONE32. Why do we need this change?

Left over from Immediate constructor, removed now.

https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64....
src/x64/assembler-x64.h:1753: if (!RelocInfo::IsNone(x.rmode_)) {
On 2016/03/15 07:44:18, Yang wrote:
> This seems like dead code.

Done.

[Yang, 2016-03-16 06:11:43]

On 2016/03/16 06:05:26, gdeepti1 wrote:
>
https://codereview.chromium.org/1759873002/diff/240001/src/arm/assembler-arm-...
> File src/arm/assembler-arm-inl.h (right):
> 
>
https://codereview.chromium.org/1759873002/diff/240001/src/arm/assembler-arm-...
> src/arm/assembler-arm-inl.h:126: wasm_memory_reference() <= old_base +
> old_size);
> On 2016/03/15 07:44:18, Yang wrote:
> > Should this really be a "less-equals" or just a "less"? I can't imagine the
> > reference pointing to outside the memory (which old_base + old_size would
be).
> > 
> > Same for the compare below and on other platform ports.
> 
> Fixed below and on other platform ports.
> 
> https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64.h
> File src/x64/assembler-x64.h (right):
> 
>
https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64....
> src/x64/assembler-x64.h:342: RelocInfo::Mode rmode() { return rmode_; }
> On 2016/03/15 07:44:18, Yang wrote:
> > This cannot be anything other than NONE32. Why do we need this change?
> 
> Left over from Immediate constructor, removed now.
> 
>
https://codereview.chromium.org/1759873002/diff/240001/src/x64/assembler-x64....
> src/x64/assembler-x64.h:1753: if (!RelocInfo::IsNone(x.rmode_)) {
> On 2016/03/15 07:44:18, Yang wrote:
> > This seems like dead code.
> 
> Done.

LGTM.

[titzer, 2016-03-16 09:26:59]

lgtm

[Marija Antic, 2016-03-16 12:50:51]

marija.antic@imgtec.com changed reviewers:
+ marija.antic@imgtec.com

[Marija Antic, 2016-03-16 12:50:53]

https://codereview.chromium.org/1759873002/diff/260001/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):

https://codereview.chromium.org/1759873002/diff/260001/test/cctest/cctest.gyp...
test/cctest/cctest.gyp:272: 'test-macro-assembler-mips64.cc'
Tests ported for MIPS https://codereview.chromium.org/1775443002/

[gdeepti1, 2016-03-16 17:07:38]

The CQ bit was checked by gdeepti@google.com

[commit-bot: I haz the power, 2016-03-16 17:07:46]

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

[commit-bot: I haz the power, 2016-03-16 17:10:09]

Description was changed from

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=
==========

to

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=
==========

[commit-bot: I haz the power, 2016-03-16 17:10:11]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-03-16 17:11:52]

Description was changed from

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=
==========

to

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=

Committed: https://crrev.com/cc815b69c17da368107ed77306a5bb161170c834
Cr-Commit-Position: refs/heads/master@{#34831}
==========

[commit-bot: I haz the power, 2016-03-16 17:11:54]

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

[Michael Achenbach, 2016-03-16 17:27:27]

A revert of this CL (patchset #13 id:260001) has been created in
https://codereview.chromium.org/1808823002/ by machenbach@chromium.org.

The reason for reverting is: Breaks compile:
https://build.chromium.org/p/client.v8/builders/V8%20Mac64/builds/7740

Probably had outdated tryjobs.

[gdeepti1, 2016-03-16 17:49:11]

Description was changed from

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=

Committed: https://crrev.com/cc815b69c17da368107ed77306a5bb161170c834
Cr-Commit-Position: refs/heads/master@{#34831}
==========

to

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=

Committed: https://crrev.com/cc815b69c17da368107ed77306a5bb161170c834
Cr-Commit-Position: refs/heads/master@{#34831}
==========

[gdeepti1, 2016-03-16 17:49:34]

The CQ bit was checked by gdeepti@google.com

[gdeepti1, 2016-03-16 17:49:35]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1759873002/#ps280001
(title: "Fix compile")

[commit-bot: I haz the power, 2016-03-16 17:49:38]

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

[commit-bot: I haz the power, 2016-03-16 18:13:50]

Description was changed from

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=

Committed: https://crrev.com/cc815b69c17da368107ed77306a5bb161170c834
Cr-Commit-Position: refs/heads/master@{#34831}
==========

to

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=

Committed: https://crrev.com/cc815b69c17da368107ed77306a5bb161170c834
Cr-Commit-Position: refs/heads/master@{#34831}
==========

[commit-bot: I haz the power, 2016-03-16 18:13:52]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-03-16 18:15:56]

Description was changed from

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=

Committed: https://crrev.com/cc815b69c17da368107ed77306a5bb161170c834
Cr-Commit-Position: refs/heads/master@{#34831}
==========

to

==========
Assembler changes for enabling GrowHeap in Wasm
 - New RelocInfo mode WASM_MEMORY_REFERENCE as a marker for wasm code objects
that need to be relocated on a heap change
 - RelocInfo mode recorded for immediates that use the memory buffer as base
 - Tests to verify address patching works

BUG=

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

Committed: https://crrev.com/e1a7c1e76c150713339bc5ba4ce84d7eb6a5420a
Cr-Commit-Position: refs/heads/master@{#34836}
==========

[commit-bot: I haz the power, 2016-03-16 18:15:58]

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