[ffi] Translation + test for int32

[ffi] Translation + test for int32

Also introduces FFIType separate from MachineType for express ffi
signatures.

BUG=v8:4456
Review-Url: https://codereview.chromium.org/2639163004
Cr-Commit-Position: refs/heads/master@{#42612}
Committed: https://chromium.googlesource.com/v8/v8/+/a5913c9a8ec0460c050d10d6d158afc7f23898ad

[mattloring, 2017-01-18 19:26:13]

Description was changed from

==========
[ffi] Translation + test for int32

Also introduces FFIType separate from MachineType for express ffi
signatures.

BUG=v8:4456
==========

to

==========
[ffi] Translation + test for int32

Also introduces FFIType separate from MachineType for express ffi
signatures.

BUG=v8:4456
==========

[mattloring, 2017-01-18 19:26:13]

mattloring@google.com changed reviewers:
+ ofrobots@google.com

[ofrobots, 2017-01-18 20:59:18]

On 2017/01/18 19:27:32, mattloring wrote:

LGTM

[mattloring, 2017-01-18 21:07:30]

mattloring@google.com changed reviewers:
+ bmeurer@chromium.org, ishell@chromium.org, mstarzinger@chromium.org

[Benedikt Meurer, 2017-01-19 05:04:28]

https://codereview.chromium.org/2639163004/diff/1/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2639163004/diff/1/src/ffi/ffi-compiler.cc#new...
src/ffi/ffi-compiler.cc:46: return result;
Nit: Can we have UNREACHABLE(); return nullptr; here, and return directly from
the proper cases above?

https://codereview.chromium.org/2639163004/diff/1/src/ffi/ffi-compiler.cc#new...
src/ffi/ffi-compiler.cc:56: }
Nit: Can we have UNREACHABLE(); return nullptr; here, and return directly from
the proper cases above?

https://codereview.chromium.org/2639163004/diff/1/src/ffi/ffi-compiler.cc#new...
src/ffi/ffi-compiler.cc:65: }
This should probably end with an UNREACHABLE(); return MachineType::None();

[mattloring, 2017-01-19 07:01:18]

https://codereview.chromium.org/2639163004/diff/1/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2639163004/diff/1/src/ffi/ffi-compiler.cc#new...
src/ffi/ffi-compiler.cc:46: return result;
On 2017/01/19 05:04:28, Benedikt Meurer wrote:
> Nit: Can we have UNREACHABLE(); return nullptr; here, and return directly from
> the proper cases above?

Done.

https://codereview.chromium.org/2639163004/diff/1/src/ffi/ffi-compiler.cc#new...
src/ffi/ffi-compiler.cc:56: }
On 2017/01/19 05:04:28, Benedikt Meurer wrote:
> Nit: Can we have UNREACHABLE(); return nullptr; here, and return directly from
> the proper cases above?

Done.

https://codereview.chromium.org/2639163004/diff/1/src/ffi/ffi-compiler.cc#new...
src/ffi/ffi-compiler.cc:65: }
On 2017/01/19 05:04:28, Benedikt Meurer wrote:
> This should probably end with an UNREACHABLE(); return MachineType::None();

Done.

[Michael Starzinger, 2017-01-19 11:37:59]

https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc...
src/ffi/ffi-compiler.cc:41: return SmiFromWord32(node);
On 32-bit architectures the payload of a Smi is a 31-bit signed integer. That
means int32_t cannot always be encoded as a Smi. Please make sure to run your
tests on a 32-bit architecture (e.g. ia32 or arm) as well. What you want here
instead is ChangeInt32ToTagged I think.

[Michael Starzinger, 2017-01-19 12:30:49]

https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc...
src/ffi/ffi-compiler.cc:110: Code::ComputeFlags(Code::FUNCTION),
"js-to-native");
Unrelated to this change, but still important (feel free to fix in a separate
CL): Marking the code objects as kind {FUNCTION} is dangerous. This instructs
the stack-walker to treat it as a full-codegen frame where all stack slots are
considered tagged. This does not hold for the frames at hand. I would advise to
use {BUILTIN} instead. The problem can be reproduced with the following
modification to the {Run_FFI_add7} test case:

  {
    FLAG_allow_natives_syntax = true;
    v8::Local<v8::Value> res =
        CompileRun("var o = { valueOf: function() { %DebugTrace() } }; o;");
    Handle<JSReceiver> param(v8::Utils::OpenHandle(v8::Object::Cast(*res)));
    Handle<Object> args[] = {
        param, isolate->factory()->NewNumber(2),
        isolate->factory()->NewNumber(3), isolate->factory()->NewNumber(4),
        isolate->factory()->NewNumber(5), isolate->factory()->NewNumber(6),
        isolate->factory()->NewNumber(21)};
    USE(Execution::Call(isolate, jsfunc, undefined, arraysize(args), args));
  }

[mattloring, 2017-01-19 18:19:55]

https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc...
src/ffi/ffi-compiler.cc:41: return SmiFromWord32(node);
On 2017/01/19 11:37:58, Michael Starzinger wrote:
> On 32-bit architectures the payload of a Smi is a 31-bit signed integer. That
> means int32_t cannot always be encoded as a Smi. Please make sure to run your
> tests on a 32-bit architecture (e.g. ia32 or arm) as well. What you want here
> instead is ChangeInt32ToTagged I think.

That makes sense, thanks!

https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc...
src/ffi/ffi-compiler.cc:110: Code::ComputeFlags(Code::FUNCTION),
"js-to-native");
On 2017/01/19 12:30:49, Michael Starzinger wrote:
> Unrelated to this change, but still important (feel free to fix in a separate
> CL): Marking the code objects as kind {FUNCTION} is dangerous. This instructs
> the stack-walker to treat it as a full-codegen frame where all stack slots are
> considered tagged. This does not hold for the frames at hand. I would advise
to
> use {BUILTIN} instead. The problem can be reproduced with the following
> modification to the {Run_FFI_add7} test case:
> 
>   {
>     FLAG_allow_natives_syntax = true;
>     v8::Local<v8::Value> res =
>         CompileRun("var o = { valueOf: function() { %DebugTrace() } }; o;");
>     Handle<JSReceiver> param(v8::Utils::OpenHandle(v8::Object::Cast(*res)));
>     Handle<Object> args[] = {
>         param, isolate->factory()->NewNumber(2),
>         isolate->factory()->NewNumber(3), isolate->factory()->NewNumber(4),
>         isolate->factory()->NewNumber(5), isolate->factory()->NewNumber(6),
>         isolate->factory()->NewNumber(21)};
>     USE(Execution::Call(isolate, jsfunc, undefined, arraysize(args), args));
>   }

Thanks for catching this. I'll open a new CL to fix.

[mattloring, 2017-01-19 18:44:16]

On 2017/01/19 18:19:55, mattloring wrote:
> https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc
> File src/ffi/ffi-compiler.cc (right):
> 
>
https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc...
> src/ffi/ffi-compiler.cc:41: return SmiFromWord32(node);
> On 2017/01/19 11:37:58, Michael Starzinger wrote:
> > On 32-bit architectures the payload of a Smi is a 31-bit signed integer.
That
> > means int32_t cannot always be encoded as a Smi. Please make sure to run
your
> > tests on a 32-bit architecture (e.g. ia32 or arm) as well. What you want
here
> > instead is ChangeInt32ToTagged I think.
> 
> That makes sense, thanks!
> 
>
https://codereview.chromium.org/2639163004/diff/20001/src/ffi/ffi-compiler.cc...
> src/ffi/ffi-compiler.cc:110: Code::ComputeFlags(Code::FUNCTION),
> "js-to-native");
> On 2017/01/19 12:30:49, Michael Starzinger wrote:
> > Unrelated to this change, but still important (feel free to fix in a
separate
> > CL): Marking the code objects as kind {FUNCTION} is dangerous. This
instructs
> > the stack-walker to treat it as a full-codegen frame where all stack slots
are
> > considered tagged. This does not hold for the frames at hand. I would advise
> to
> > use {BUILTIN} instead. The problem can be reproduced with the following
> > modification to the {Run_FFI_add7} test case:
> > 
> >   {
> >     FLAG_allow_natives_syntax = true;
> >     v8::Local<v8::Value> res =
> >         CompileRun("var o = { valueOf: function() { %DebugTrace() } }; o;");
> >     Handle<JSReceiver> param(v8::Utils::OpenHandle(v8::Object::Cast(*res)));
> >     Handle<Object> args[] = {
> >         param, isolate->factory()->NewNumber(2),
> >         isolate->factory()->NewNumber(3), isolate->factory()->NewNumber(4),
> >         isolate->factory()->NewNumber(5), isolate->factory()->NewNumber(6),
> >         isolate->factory()->NewNumber(21)};
> >     USE(Execution::Call(isolate, jsfunc, undefined, arraysize(args), args));
> >   }
> 
> Thanks for catching this. I'll open a new CL to fix.

Actually, just going to fix that in this CL.

[Michael Starzinger, 2017-01-20 09:11:40]

LGTM from my end.

[Michael Starzinger, 2017-01-20 09:14:21]

https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
File test/cctest/ffi/test-ffi.cc (right):

https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
test/cctest/ffi/test-ffi.cc:197: CompileRun("var o = { valueOf: function() {
%DebugTrace() } }; o;");
nit: You could even add a "return 1" into the "valueOf" function after the
"%DebugTrace" and then check the return value down below in line 206 similar to
the other methods. Just for additional checking.

[mattloring, 2017-01-20 18:12:51]

https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
File test/cctest/ffi/test-ffi.cc (right):

https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
test/cctest/ffi/test-ffi.cc:197: CompileRun("var o = { valueOf: function() {
%DebugTrace() } }; o;");
On 2017/01/20 09:14:21, Michael Starzinger wrote:
> nit: You could even add a "return 1" into the "valueOf" function after the
> "%DebugTrace" and then check the return value down below in line 206 similar
to
> the other methods. Just for additional checking.

Done.

[mattloring, 2017-01-20 18:13:01]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 18:13:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 18:40:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 18:40:15]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)

[mattloring, 2017-01-20 19:59:49]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 20:00:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 20:29:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 20:29:26]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)

[mattloring, 2017-01-20 20:32:33]

Patchset #6 (id:100001) has been deleted

[mattloring, 2017-01-20 21:23:03]

On 2017/01/20 18:12:51, mattloring wrote:
>
https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
> File test/cctest/ffi/test-ffi.cc (right):
> 
>
https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
> test/cctest/ffi/test-ffi.cc:197: CompileRun("var o = { valueOf: function() {
> %DebugTrace() } }; o;");
> On 2017/01/20 09:14:21, Michael Starzinger wrote:
> > nit: You could even add a "return 1" into the "valueOf" function after the
> > "%DebugTrace" and then check the return value down below in line 206 similar
> to
> > the other methods. Just for additional checking.
> 
> Done.

I am in the process of trying to get access to an Arm machine to debug this. If
anyone has thoughts on these failures that would be helpful as well.

[Michael Starzinger, 2017-01-23 13:20:29]

On 2017/01/20 21:23:03, mattloring wrote:
> On 2017/01/20 18:12:51, mattloring wrote:
> >
>
https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
> > File test/cctest/ffi/test-ffi.cc (right):
> > 
> >
>
https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
> > test/cctest/ffi/test-ffi.cc:197: CompileRun("var o = { valueOf: function() {
> > %DebugTrace() } }; o;");
> > On 2017/01/20 09:14:21, Michael Starzinger wrote:
> > > nit: You could even add a "return 1" into the "valueOf" function after the
> > > "%DebugTrace" and then check the return value down below in line 206
similar
> > to
> > > the other methods. Just for additional checking.
> > 
> > Done.
> 
> I am in the process of trying to get access to an Arm machine to debug this.
If
> anyone has thoughts on these failures that would be helpful as well.

You don't need ARM hardware to reproduce this, just running the V8-internal
simulator is good enough. If you compile the "arm.debug" configuration on your
machine, it will automatically run all generated code through the simulator. In
fact, the try-jobs do exactly the same. I can reproduce the issue at hand
locally just fine.

[mattloring, 2017-01-23 22:08:58]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 22:09:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 22:39:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 22:39:58]

Dry run: This issue passed the CQ dry run.

[mattloring, 2017-01-23 23:50:37]

On 2017/01/23 13:20:29, Michael Starzinger wrote:
> On 2017/01/20 21:23:03, mattloring wrote:
> > On 2017/01/20 18:12:51, mattloring wrote:
> > >
> >
>
https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
> > > File test/cctest/ffi/test-ffi.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2639163004/diff/60001/test/cctest/ffi/test-ff...
> > > test/cctest/ffi/test-ffi.cc:197: CompileRun("var o = { valueOf: function()
{
> > > %DebugTrace() } }; o;");
> > > On 2017/01/20 09:14:21, Michael Starzinger wrote:
> > > > nit: You could even add a "return 1" into the "valueOf" function after
the
> > > > "%DebugTrace" and then check the return value down below in line 206
> similar
> > > to
> > > > the other methods. Just for additional checking.
> > > 
> > > Done.
> > 
> > I am in the process of trying to get access to an Arm machine to debug this.
> If
> > anyone has thoughts on these failures that would be helpful as well.
> 
> You don't need ARM hardware to reproduce this, just running the V8-internal
> simulator is good enough. If you compile the "arm.debug" configuration on your
> machine, it will automatically run all generated code through the simulator.
In
> fact, the try-jobs do exactly the same. I can reproduce the issue at hand
> locally just fine.

Things are fixed on arm. It looks like the simulator for arm does not support
calls with more than 6 arguments. Is this expected or an error on my end?

[mattloring, 2017-01-23 23:50:44]

The CQ bit was checked by mattloring@google.com

[mattloring, 2017-01-23 23:50:44]

The patchset sent to the CQ was uploaded after l-g-t-m from ofrobots@google.com,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2639163004/#ps120001
(title: "Arm fixes")

[commit-bot: I haz the power, 2017-01-23 23:50:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 23:52:52]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485215444314480,
"parent_rev": "6eba6b4b89db81bc359dc041c62d1277f24bb8d4", "commit_rev":
"a5913c9a8ec0460c050d10d6d158afc7f23898ad"}

[commit-bot: I haz the power, 2017-01-23 23:52:57]

Description was changed from

==========
[ffi] Translation + test for int32

Also introduces FFIType separate from MachineType for express ffi
signatures.

BUG=v8:4456
==========

to

==========
[ffi] Translation + test for int32

Also introduces FFIType separate from MachineType for express ffi
signatures.

BUG=v8:4456

Review-Url: https://codereview.chromium.org/2639163004
Cr-Commit-Position: refs/heads/master@{#42612}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5913c9a8ec0460c050d10d6d158afc7f23...
==========

[commit-bot: I haz the power, 2017-01-23 23:52:58]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/a5913c9a8ec0460c050d10d6d158afc7f23...