[wasm] remove faux code objects

[wasm] remove faux code objects

This change decouples direct calls from Handle<Code> by introducing
a wasm-specific relocation info, WASM_DIRECT_CALL, and compiling
direct calls as having a relocatable int32 first operand (the call site). 
 
BUG=

[Mircea Trofin, 2016-06-01 00:07:54]

Description was changed from

==========
[wasm] redo linking to avoid faux code objects (redo)
BUG=
==========

to

==========
[wasm] remove faux code objects

This change decouples direct calls from Handle<Code> by introducing
a wasm-specific relocation info, WASM_DIRECT_CALL, and compiling
direct calls as having a relocatable int32 first operand (the call site). 
 
BUG=
==========

[Mircea Trofin, 2016-06-01 00:07:54]

mtrofin@chromium.org changed reviewers:
+ ahaas@chromium.org, titzer@chromium.org

[Mircea Trofin, 2016-06-01 00:09:20]

This is a refresh of the earlier 1962643004, which I'm closing. I 
preferred starting a new CL since the code changed in the last few 
weeks - so a new CL should be easier to parse.

[Mircea Trofin, 2016-06-02 00:01:15]

gentle reminder.

thanks!

[titzer, 2016-06-02 13:17:07]

titzer@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[titzer, 2016-06-02 13:17:08]

+mstarzinger

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

https://codereview.chromium.org/2021323003/diff/20001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:252: uint32_t RelocInfo::wasm_function_index() {
This function basically assumes that the call site has not been patched to
already refer directly to the address which is being called. If this function is
called after the WASM call is patched, this is going to return that address.

How will this work with serialization/deserialization?

https://codereview.chromium.org/2021323003/diff/20001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2021323003/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:473: case kArchCallCodeObject: {
We should add another enum case here and not overload the CallCodeObject logic.

[ahaas, 2016-06-02 13:46:59]

On 2016/06/02 at 00:01:15, mtrofin wrote:
> gentle reminder.
> 
> thanks!

There is one problem when you avoid the use of placeholder code objects which is
garbage collection. The garbage collector may move code objects, and to update
pointers between code objects these pointers have to be stored in the relocation
info as CodeTargets. In the linking I think you have to set the target address
and change the mode of the relocation info to CodeTarget at the same time, i.e.
no GC should happen between these steps.

[ahaas, 2016-06-02 13:48:28]

https://codereview.chromium.org/2021323003/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2021323003/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:3313: void Link(Isolate* isolate,
std::vector<Handle<Code>>& functions) {
What's the reason why you move the code for linking to wasm-compiler.cc? In my
understanding, wasm-compiler.cc contains code to compile a single wasm function,
and wasm-module.cc contains code to compile a set of functions, i.e. a whole
module. As linking processes a set of functions, I think that linking belongs to
wasm-module.cc.

[Mircea Trofin, 2016-06-02 14:46:21]

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

https://codereview.chromium.org/2021323003/diff/20001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:252: uint32_t RelocInfo::wasm_function_index() {
On 2016/06/02 13:17:08, titzer wrote:
> This function basically assumes that the call site has not been patched to
> already refer directly to the address which is being called. If this function
is
> called after the WASM call is patched, this is going to return that address.
> 
> How will this work with serialization/deserialization?

The statement about the assumption is correct.

Serialization/deserialization are not affected. We can do them either before or
after linking. We just need to avoid linking twice. I validated doing ser/deser
before linking locally.

https://codereview.chromium.org/2021323003/diff/20001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2021323003/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:473: case kArchCallCodeObject: {
On 2016/06/02 13:17:08, titzer wrote:
> We should add another enum case here and not overload the CallCodeObject
logic.

Happy to.

I intend to do the same thing for imports (add a WASM_IMPORT_CALL, traverse
reloc info to patch). This would help with decoupling compilation from
instantiation: compilation gets you a fixed array of code objects, not yet tied
to imports, and with possibly wrong memory size. Instantiation then means deep
cloning the FixedArray of code objects (haven't looked at how.
serialize/deserialize would work, worse case), linking the imports, and patching
the mem size (we probably want, then, to patch mem size with "0" at compile time
or something). 

I believe the only remaining parameter embedded in the compiled code is the
native context, which is fine in the same isolate.

Now the question: do we want to have one kArchWasmCall covering both imports and
internal calls - letting the reloc info distinguish which is which - or do we
want to have 2 codes? I locally have a patch (x64 only) validating we can do the
former (with the current design, where we piggyback on kArchCallCodeObject).
There may be other considerations, though, for having 2 codes.

Thoughts?

https://codereview.chromium.org/2021323003/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2021323003/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:3313: void Link(Isolate* isolate,
std::vector<Handle<Code>>& functions) {
On 2016/06/02 13:48:27, ahaas wrote:
> What's the reason why you move the code for linking to wasm-compiler.cc? In my
> understanding, wasm-compiler.cc contains code to compile a single wasm
function,
> and wasm-module.cc contains code to compile a set of functions, i.e. a whole
> module. As linking processes a set of functions, I think that linking belongs
to
> wasm-module.cc.

I didn't realize that was the separation. I thought module had to do with
instantiation (the term "module" is heavily overloaded through code :))

I'll move it back.

[Mircea Trofin, 2016-06-02 14:49:16]

On 2016/06/02 13:46:59, ahaas wrote:
> On 2016/06/02 at 00:01:15, mtrofin wrote:
> > gentle reminder.
> > 
> > thanks!
> 
> There is one problem when you avoid the use of placeholder code objects which
is
> garbage collection. The garbage collector may move code objects, and to update
> pointers between code objects these pointers have to be stored in the
relocation
> info as CodeTargets. In the linking I think you have to set the target address
> and change the mode of the relocation info to CodeTarget at the same time,
i.e.
> no GC should happen between these steps.

Good point, thanks! I suppose we should disable GC for the duration of linking
altogether, then.

[Mircea Trofin, 2016-06-05 00:53:33]

Patchset #4 (id:60001) has been deleted

[Mircea Trofin, 2016-06-05 00:53:42]

Patchset #3 (id:40001) has been deleted

[Mircea Trofin, 2016-06-05 00:54:11]

Patchset #3 (id:80001) has been deleted

[titzer, 2016-06-06 07:32:59]

On 2016/06/02 14:49:16, Mircea Trofin wrote:
> On 2016/06/02 13:46:59, ahaas wrote:
> > On 2016/06/02 at 00:01:15, mtrofin wrote:
> > > gentle reminder.
> > > 
> > > thanks!
> > 
> > There is one problem when you avoid the use of placeholder code objects
which
> is
> > garbage collection. The garbage collector may move code objects, and to
update
> > pointers between code objects these pointers have to be stored in the
> relocation
> > info as CodeTargets. In the linking I think you have to set the target
address
> > and change the mode of the relocation info to CodeTarget at the same time,
> i.e.
> > no GC should happen between these steps.
> 
> Good point, thanks! I suppose we should disable GC for the duration of linking
> altogether, then.

That might not be enough. My earlier comment (about sometimes the call address
encodes a WASM function index, and sometimes the direct address) could also
apply to the GC when it is walking code objects for compaction. We generally
don't want code compaction for WASM, but it could happen now, AFAIU.

[Mircea Trofin, 2016-06-06 15:25:07]

On 2016/06/06 07:32:59, titzer wrote:
> On 2016/06/02 14:49:16, Mircea Trofin wrote:
> > On 2016/06/02 13:46:59, ahaas wrote:
> > > On 2016/06/02 at 00:01:15, mtrofin wrote:
> > > > gentle reminder.
> > > > 
> > > > thanks!
> > > 
> > > There is one problem when you avoid the use of placeholder code objects
> which
> > is
> > > garbage collection. The garbage collector may move code objects, and to
> update
> > > pointers between code objects these pointers have to be stored in the
> > relocation
> > > info as CodeTargets. In the linking I think you have to set the target
> address
> > > and change the mode of the relocation info to CodeTarget at the same time,
> > i.e.
> > > no GC should happen between these steps.
> > 
> > Good point, thanks! I suppose we should disable GC for the duration of
linking
> > altogether, then.
> 
> That might not be enough. My earlier comment (about sometimes the call address
> encodes a WASM function index, and sometimes the direct address) could also
> apply to the GC when it is walking code objects for compaction. We generally
> don't want code compaction for WASM, but it could happen now, AFAIU.

Could you elaborate? Here's my understanding of the world today: call sites are
encoded
 as having reloc info CODE_TARGET, which is understood by GC and resilient
(afaik) 
in face of compaction, but compaction may still happen. How do we avoid code
compaction, 
today, for WASM?

With my change, the call sites are first encoded as WASM_DIRECT_CALL,
when they store the index, and then, at link time, we stop GC, replace the index
with the address and 
change the reloc info to CODE_TARGET, then resume the world. Everything looks,
after linking, just 
like before. What am I missing?

Thanks!

[titzer, 2016-06-06 15:39:10]

On 2016/06/06 15:25:07, Mircea Trofin wrote:
> On 2016/06/06 07:32:59, titzer wrote:
> > On 2016/06/02 14:49:16, Mircea Trofin wrote:
> > > On 2016/06/02 13:46:59, ahaas wrote:
> > > > On 2016/06/02 at 00:01:15, mtrofin wrote:
> > > > > gentle reminder.
> > > > > 
> > > > > thanks!
> > > > 
> > > > There is one problem when you avoid the use of placeholder code objects
> > which
> > > is
> > > > garbage collection. The garbage collector may move code objects, and to
> > update
> > > > pointers between code objects these pointers have to be stored in the
> > > relocation
> > > > info as CodeTargets. In the linking I think you have to set the target
> > address
> > > > and change the mode of the relocation info to CodeTarget at the same
time,
> > > i.e.
> > > > no GC should happen between these steps.
> > > 
> > > Good point, thanks! I suppose we should disable GC for the duration of
> linking
> > > altogether, then.
> > 
> > That might not be enough. My earlier comment (about sometimes the call
address
> > encodes a WASM function index, and sometimes the direct address) could also
> > apply to the GC when it is walking code objects for compaction. We generally
> > don't want code compaction for WASM, but it could happen now, AFAIU.
> 
> Could you elaborate? Here's my understanding of the world today: call sites
are
> encoded
>  as having reloc info CODE_TARGET, which is understood by GC and resilient
> (afaik) 
> in face of compaction, but compaction may still happen. How do we avoid code
> compaction, 
> today, for WASM?

At any point that a GC occurs, it will try to decode the reloc_info for
CODE_TARGET in order to patch the call address. If this occurs for a WASM code
object when the call target incorrectly, the GC will be trying to interpret a
WASM function index as an instruction address and will find to find a code
object. Basically, the GC doesn't know about the two states of encoding.

> 
> With my change, the call sites are first encoded as WASM_DIRECT_CALL,
> when they store the index, and then, at link time, we stop GC, replace the
index
> with the address and 
> change the reloc info to CODE_TARGET, then resume the world. Everything looks,
> after linking, just 
> like before. What am I missing?
> 

A GC could occur before we even started linking, so it could observe addresses
too early.

Generally it's not going be viable to disable compaction or other GC activity
until we have WASM machine code out-of-heap (i.e., not code objects).

> Thanks!

[titzer, 2016-06-06 15:44:52]

On 2016/06/06 15:39:10, titzer wrote:
> On 2016/06/06 15:25:07, Mircea Trofin wrote:
> > On 2016/06/06 07:32:59, titzer wrote:
> > > On 2016/06/02 14:49:16, Mircea Trofin wrote:
> > > > On 2016/06/02 13:46:59, ahaas wrote:
> > > > > On 2016/06/02 at 00:01:15, mtrofin wrote:
> > > > > > gentle reminder.
> > > > > > 
> > > > > > thanks!
> > > > > 
> > > > > There is one problem when you avoid the use of placeholder code
objects
> > > which
> > > > is
> > > > > garbage collection. The garbage collector may move code objects, and
to
> > > update
> > > > > pointers between code objects these pointers have to be stored in the
> > > > relocation
> > > > > info as CodeTargets. In the linking I think you have to set the target
> > > address
> > > > > and change the mode of the relocation info to CodeTarget at the same
> time,
> > > > i.e.
> > > > > no GC should happen between these steps.
> > > > 
> > > > Good point, thanks! I suppose we should disable GC for the duration of
> > linking
> > > > altogether, then.
> > > 
> > > That might not be enough. My earlier comment (about sometimes the call
> address
> > > encodes a WASM function index, and sometimes the direct address) could
also
> > > apply to the GC when it is walking code objects for compaction. We
generally
> > > don't want code compaction for WASM, but it could happen now, AFAIU.
> > 
> > Could you elaborate? Here's my understanding of the world today: call sites
> are
> > encoded
> >  as having reloc info CODE_TARGET, which is understood by GC and resilient
> > (afaik) 
> > in face of compaction, but compaction may still happen. How do we avoid code
> > compaction, 
> > today, for WASM?
> 
> At any point that a GC occurs, it will try to decode the reloc_info for
> CODE_TARGET in order to patch the call address. If this occurs for a WASM code
> object when the call target incorrectly, the GC will be trying to interpret a
> WASM function index as an instruction address and will find to find a code
> object. Basically, the GC doesn't know about the two states of encoding.
> 
> > 
> > With my change, the call sites are first encoded as WASM_DIRECT_CALL,
> > when they store the index, and then, at link time, we stop GC, replace the
> index
> > with the address and 
> > change the reloc info to CODE_TARGET, then resume the world. Everything
looks,
> > after linking, just 
> > like before. What am I missing?
> > 
> 

Sorry, I missed that you are changing the relocation mode. I suppose that will
work, but I am still nervous about disabling GC during linking.

[Mircea Trofin, 2016-06-06 15:50:36]

On 2016/06/06 15:39:10, titzer wrote:
> On 2016/06/06 15:25:07, Mircea Trofin wrote:
> > On 2016/06/06 07:32:59, titzer wrote:
> > > On 2016/06/02 14:49:16, Mircea Trofin wrote:
> > > > On 2016/06/02 13:46:59, ahaas wrote:
> > > > > On 2016/06/02 at 00:01:15, mtrofin wrote:
> > > > > > gentle reminder.
> > > > > > 
> > > > > > thanks!
> > > > > 
> > > > > There is one problem when you avoid the use of placeholder code
objects
> > > which
> > > > is
> > > > > garbage collection. The garbage collector may move code objects, and
to
> > > update
> > > > > pointers between code objects these pointers have to be stored in the
> > > > relocation
> > > > > info as CodeTargets. In the linking I think you have to set the target
> > > address
> > > > > and change the mode of the relocation info to CodeTarget at the same
> time,
> > > > i.e.
> > > > > no GC should happen between these steps.
> > > > 
> > > > Good point, thanks! I suppose we should disable GC for the duration of
> > linking
> > > > altogether, then.
> > > 
> > > That might not be enough. My earlier comment (about sometimes the call
> address
> > > encodes a WASM function index, and sometimes the direct address) could
also
> > > apply to the GC when it is walking code objects for compaction. We
generally
> > > don't want code compaction for WASM, but it could happen now, AFAIU.
> > 
> > Could you elaborate? Here's my understanding of the world today: call sites
> are
> > encoded
> >  as having reloc info CODE_TARGET, which is understood by GC and resilient
> > (afaik) 
> > in face of compaction, but compaction may still happen. How do we avoid code
> > compaction, 
> > today, for WASM?
> 
> At any point that a GC occurs, it will try to decode the reloc_info for
> CODE_TARGET in order to patch the call address. If this occurs for a WASM code
> object when the call target incorrectly, the GC will be trying to interpret a
> WASM function index as an instruction address and will find to find a code
> object. Basically, the GC doesn't know about the two states of encoding.

Yes, but the call target isn't a CODE_TARGET when it encodes an index. It is
a WASM_DIRECT_CALL. How would the GC see it?

BTW, today, for regular (non-wasm) CODE_TARGETs, we also store an index 
there, for a while, marked as CODE_TARGET, and we avoid GC issues the same way.
See Factory::NewCode. We disable heap allocation, then copy the bytes (Code
object
is invalid, CODE_TARGETs are indexes) and then relocate.
If anything, the mechanics in my change further avoid any GC issues by
completely
hiding the indexes from GC until the code is patched. 

> 
> > 
> > With my change, the call sites are first encoded as WASM_DIRECT_CALL,
> > when they store the index, and then, at link time, we stop GC, replace the
> index
> > with the address and 
> > change the reloc info to CODE_TARGET, then resume the world. Everything
looks,
> > after linking, just 
> > like before. What am I missing?
> > 
> 
> A GC could occur before we even started linking, so it could observe addresses
> too early.
> 

But the state of the world when we start linking should be consistent: the
handles to
the code objects are always correct.

> Generally it's not going be viable to disable compaction or other GC activity
> until we have WASM machine code out-of-heap (i.e., not code objects).
> 
> > Thanks!

[titzer, 2016-06-06 15:56:55]

On 2016/06/06 15:50:36, Mircea Trofin wrote:
> On 2016/06/06 15:39:10, titzer wrote:
> > On 2016/06/06 15:25:07, Mircea Trofin wrote:
> > > On 2016/06/06 07:32:59, titzer wrote:
> > > > On 2016/06/02 14:49:16, Mircea Trofin wrote:
> > > > > On 2016/06/02 13:46:59, ahaas wrote:
> > > > > > On 2016/06/02 at 00:01:15, mtrofin wrote:
> > > > > > > gentle reminder.
> > > > > > > 
> > > > > > > thanks!
> > > > > > 
> > > > > > There is one problem when you avoid the use of placeholder code
> objects
> > > > which
> > > > > is
> > > > > > garbage collection. The garbage collector may move code objects, and
> to
> > > > update
> > > > > > pointers between code objects these pointers have to be stored in
the
> > > > > relocation
> > > > > > info as CodeTargets. In the linking I think you have to set the
target
> > > > address
> > > > > > and change the mode of the relocation info to CodeTarget at the same
> > time,
> > > > > i.e.
> > > > > > no GC should happen between these steps.
> > > > > 
> > > > > Good point, thanks! I suppose we should disable GC for the duration of
> > > linking
> > > > > altogether, then.
> > > > 
> > > > That might not be enough. My earlier comment (about sometimes the call
> > address
> > > > encodes a WASM function index, and sometimes the direct address) could
> also
> > > > apply to the GC when it is walking code objects for compaction. We
> generally
> > > > don't want code compaction for WASM, but it could happen now, AFAIU.
> > > 
> > > Could you elaborate? Here's my understanding of the world today: call
sites
> > are
> > > encoded
> > >  as having reloc info CODE_TARGET, which is understood by GC and resilient
> > > (afaik) 
> > > in face of compaction, but compaction may still happen. How do we avoid
code
> > > compaction, 
> > > today, for WASM?
> > 
> > At any point that a GC occurs, it will try to decode the reloc_info for
> > CODE_TARGET in order to patch the call address. If this occurs for a WASM
code
> > object when the call target incorrectly, the GC will be trying to interpret
a
> > WASM function index as an instruction address and will find to find a code
> > object. Basically, the GC doesn't know about the two states of encoding.
> 
> Yes, but the call target isn't a CODE_TARGET when it encodes an index. It is
> a WASM_DIRECT_CALL. How would the GC see it?
> 
> BTW, today, for regular (non-wasm) CODE_TARGETs, we also store an index 
> there, for a while, marked as CODE_TARGET, and we avoid GC issues the same
way.
> See Factory::NewCode. We disable heap allocation, then copy the bytes (Code
> object
> is invalid, CODE_TARGETs are indexes) and then relocate.
> If anything, the mechanics in my change further avoid any GC issues by
> completely
> hiding the indexes from GC until the code is patched. 
> 

That's right, but this internal buffer is just raw_bytes and it doesn't have
encoded heap references in it. (It may have, however, encoded pointers to handle
locations that are undone during the copy).

See my earlier comment. I missed that you were proposing to change the
relocation mode afterwards.

> > 
> > > 
> > > With my change, the call sites are first encoded as WASM_DIRECT_CALL,
> > > when they store the index, and then, at link time, we stop GC, replace the
> > index
> > > with the address and 
> > > change the reloc info to CODE_TARGET, then resume the world. Everything
> looks,
> > > after linking, just 
> > > like before. What am I missing?
> > > 
> > 
> > A GC could occur before we even started linking, so it could observe
addresses
> > too early.
> > 
> 
> But the state of the world when we start linking should be consistent: the
> handles to
> the code objects are always correct.

Right.

> 
> > Generally it's not going be viable to disable compaction or other GC
activity
> > until we have WASM machine code out-of-heap (i.e., not code objects).
> > 
> > > Thanks!