V8. ASM-2-WASM. New type system.

V8. ASM-2-WASM. New type system.

This CL introduces the new type system for the ASM
type-checker/validator.

BUG=

Committed: https://crrev.com/73eacf6b90dc7eb080c9c1416279cbd4f27df0ba
Cr-Commit-Position: refs/heads/master@{#36942}

[John, 2016-06-08 22:31:29]

jpp@chromium.org changed reviewers:
+ bradnelson@chromium.org

[bradnelson, 2016-06-08 23:42:48]

Description was changed from

==========
V8. ASM-2-WASM. New type system.

This CL introduces the new type sustem for the ASM
type-checker/validator.

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

to

==========
V8. ASM-2-WASM. New type system.

This CL introduces the new type system for the ASM
type-checker/validator.

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

[bradnelson, 2016-06-10 05:59:52]

lgtm with optional suggestions.

https://codereview.chromium.org/2045703007/diff/1/src/typing-asm.h
File src/typing-asm.h (right):

https://codereview.chromium.org/2045703007/diff/1/src/typing-asm.h#newcode27
src/typing-asm.h:27: // using Type = wasm::AsmType;
Leftover?

https://codereview.chromium.org/2045703007/diff/1/src/wasm/asm-types.cc
File src/wasm/asm-types.cc (right):

https://codereview.chromium.org/2045703007/diff/1/src/wasm/asm-types.cc#newco...
src/wasm/asm-types.cc:49: AsmValueType* tavt = that->AsValueType();
Maybe comment that IsA only relates basic types?

https://codereview.chromium.org/2045703007/diff/1/test/unittests/wasm/asm-typ...
File test/unittests/wasm/asm-types-unittest.cc (right):

https://codereview.chromium.org/2045703007/diff/1/test/unittests/wasm/asm-typ...
test/unittests/wasm/asm-types-unittest.cc:151: EXPECT_EQ(total_types,
seen_types.size());
Sanity check that there's at least one (ie that the macros didn't fall over?)

https://codereview.chromium.org/2045703007/diff/1/test/unittests/wasm/asm-typ...
test/unittests/wasm/asm-types-unittest.cc:166: parent_mask32 =
2 clever by half :-)

[John, 2016-06-13 14:20:47]

https://codereview.chromium.org/2045703007/diff/1/src/typing-asm.h
File src/typing-asm.h (right):

https://codereview.chromium.org/2045703007/diff/1/src/typing-asm.h#newcode27
src/typing-asm.h:27: // using Type = wasm::AsmType;
On 2016/06/10 05:59:51, bradnelson wrote:
> Leftover?

Done.

https://codereview.chromium.org/2045703007/diff/1/src/wasm/asm-types.cc
File src/wasm/asm-types.cc (right):

https://codereview.chromium.org/2045703007/diff/1/src/wasm/asm-types.cc#newco...
src/wasm/asm-types.cc:49: AsmValueType* tavt = that->AsValueType();
On 2016/06/10 05:59:51, bradnelson wrote:
> Maybe comment that IsA only relates basic types?

Done.

https://codereview.chromium.org/2045703007/diff/1/test/unittests/wasm/asm-typ...
File test/unittests/wasm/asm-types-unittest.cc (right):

https://codereview.chromium.org/2045703007/diff/1/test/unittests/wasm/asm-typ...
test/unittests/wasm/asm-types-unittest.cc:151: EXPECT_EQ(total_types,
seen_types.size());
On 2016/06/10 05:59:52, bradnelson wrote:
> Sanity check that there's at least one (ie that the macros didn't fall over?)

Done.

https://codereview.chromium.org/2045703007/diff/1/test/unittests/wasm/asm-typ...
test/unittests/wasm/asm-types-unittest.cc:166: parent_mask32 =
On 2016/06/10 05:59:52, bradnelson wrote:
> 2 clever by half :-)

Yeah, I don't know how this works -- found this code on stack overflow, and it
seems to work... <shame />

[John, 2016-06-13 14:21:47]

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

[ahaas, 2016-06-13 16:27:03]

lgtm with nits

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc
File src/wasm/asm-types.cc (right):

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:32: return
reinterpret_cast<AsmCallableType*>(this)->Name();
AsCallableType()->Name();

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:45: return that == this;
Could you add a comment which explains why callable types can never be exactly
the same? Or are AsmCallableType objects unique?

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:59: return that == this;
If you say that IsA is only meaningful for basic types, could you put an
UNREACHABLE here?

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:121: return AsmType::FloatishDoubleQ();
Is it intentional that you say FloatishDoubleQ here, but above in LoadType you
say FloatQ?

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:140: ret += "...";
Is it intentional that this is "..." and not ", ..."?

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.h
File src/wasm/asm-types.h (right):

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.h#ne...
src/wasm/asm-types.h:62: typedef uint32_t bitset_t;
is it necessary/helpful to introduce this type here?

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.h#ne...
src/wasm/asm-types.h:65: static constexpr uint32_t kAsmValueTypeTag = 1u << 0;
Why do you shift by 0?

https://codereview.chromium.org/2045703007/diff/20001/test/unittests/wasm/asm...
File test/unittests/wasm/asm-types-unittest.cc (right):

https://codereview.chromium.org/2045703007/diff/20001/test/unittests/wasm/asm...
test/unittests/wasm/asm-types-unittest.cc:229: Function(Type::Int)(Type::Int,
Type::Int),
If you duplicate the line "Function(Type::Int)(Type::Int, Type::Int),", the test
still passes. Is this behavior intentional?

[John, 2016-06-13 17:32:27]

thanks for the reviews and comments.

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc
File src/wasm/asm-types.cc (right):

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:32: return
reinterpret_cast<AsmCallableType*>(this)->Name();
On 2016/06/13 16:27:03, ahaas wrote:
> AsCallableType()->Name();

Done.

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:45: return that == this;
On 2016/06/13 16:27:03, ahaas wrote:
> Could you add a comment which explains why callable types can never be exactly
> the same? Or are AsmCallableType objects unique?

IsExactly is only supposed to be used when querying a type's inheritance.
Callable types are supposed to be inspected with the ValidateCall() method --
see below.

This method (and its sibling IsA) are going to be used for type-checking
assignmets, and in asm.js only ints, floats, and doubles are assignable.

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:59: return that == this;
On 2016/06/13 16:27:03, ahaas wrote:
> If you say that IsA is only meaningful for basic types, could you put an
> UNREACHABLE here?

This is a good question, and I don't know the answer to it yet. Quite possibly,
yes, so I left a TODO behind.

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:121: return AsmType::FloatishDoubleQ();
On 2016/06/13 16:27:03, ahaas wrote:
> Is it intentional that you say FloatishDoubleQ here, but above in LoadType you
> say FloatQ?

yes, http://asmjs.org/spec/latest/#heap-view-types

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.cc#n...
src/wasm/asm-types.cc:140: ret += "...";
On 2016/06/13 16:27:03, ahaas wrote:
> Is it intentional that this is "..." and not ", ..."?

yes, http://asmjs.org/spec/latest/#global-types , variadic function types. Min
and Max are the only variadic function in Asm.js, that's why I did not implement
generic handling for asm.js' variadic functions.

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.h
File src/wasm/asm-types.h (right):

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.h#ne...
src/wasm/asm-types.h:62: typedef uint32_t bitset_t;
On 2016/06/13 16:27:03, ahaas wrote:
> is it necessary/helpful to introduce this type here?

It is a personal decision: I personally prefer typedefing stuff to convey more
semantic information. Given the LGTM, I am assuming you were just inquiring, and
not requesting a change. If you want me to change, let me know and I will send a
CL.

https://codereview.chromium.org/2045703007/diff/20001/src/wasm/asm-types.h#ne...
src/wasm/asm-types.h:65: static constexpr uint32_t kAsmValueTypeTag = 1u << 0;
On 2016/06/13 16:27:03, ahaas wrote:
> Why do you shift by 0?

Because this is a sequence of related constants, and all are left-shifted, but
also because the number 1u represents (to me) the constant 1, and not the
bitmask 0000...0001. Reverted.

https://codereview.chromium.org/2045703007/diff/20001/test/unittests/wasm/asm...
File test/unittests/wasm/asm-types-unittest.cc (right):

https://codereview.chromium.org/2045703007/diff/20001/test/unittests/wasm/asm...
test/unittests/wasm/asm-types-unittest.cc:229: Function(Type::Int)(Type::Int,
Type::Int),
On 2016/06/13 16:27:03, ahaas wrote:
> If you duplicate the line "Function(Type::Int)(Type::Int, Type::Int),", the
test
> still passes. Is this behavior intentional?

Yes, because (as I mentioned before) function types are not expected to be
tested for equality. Right now, IsExactly does the "sane" (for some definition
of sane) thing, and return true if, and only if, "this" matches the parameter.

[John, 2016-06-13 17:50:21]

The CQ bit was checked by jpp@chromium.org

[John, 2016-06-13 17:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2045703007/#ps40001
(title: "Addesses comments")

[commit-bot: I haz the power, 2016-06-13 17:50:28]

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

[commit-bot: I haz the power, 2016-06-13 17:52:59]

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

[commit-bot: I haz the power, 2016-06-13 17:53:00]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)

[John, 2016-06-13 20:12:56]

The CQ bit was checked by jpp@chromium.org

[John, 2016-06-13 20:12:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2045703007/#ps160001
(title: "vs require unsigned zero.")

[commit-bot: I haz the power, 2016-06-13 20:13:02]

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

[commit-bot: I haz the power, 2016-06-13 20:41:18]

Description was changed from

==========
V8. ASM-2-WASM. New type system.

This CL introduces the new type system for the ASM
type-checker/validator.

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

to

==========
V8. ASM-2-WASM. New type system.

This CL introduces the new type system for the ASM
type-checker/validator.

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

[commit-bot: I haz the power, 2016-06-13 20:41:20]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-13 20:45:01]

Description was changed from

==========
V8. ASM-2-WASM. New type system.

This CL introduces the new type system for the ASM
type-checker/validator.

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

to

==========
V8. ASM-2-WASM. New type system.

This CL introduces the new type system for the ASM
type-checker/validator.

BUG=

Committed: https://crrev.com/73eacf6b90dc7eb080c9c1416279cbd4f27df0ba
Cr-Commit-Position: refs/heads/master@{#36942}
==========

[commit-bot: I haz the power, 2016-06-13 20:45:02]

Patchset 9 (id:??) landed as
https://crrev.com/73eacf6b90dc7eb080c9c1416279cbd4f27df0ba
Cr-Commit-Position: refs/heads/master@{#36942}

[Michael Achenbach, 2016-06-14 12:36:51]

Is this using any c++11 library features? This CL broke our gn bot that despite
using a sysroot for building, links against a too new version of the stl:

https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20swarming%20s...

[Michael Achenbach, 2016-06-14 12:46:46]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2016-06-14 12:46:47]

See also the list here:
https://chromium-cpp.appspot.com/

And this discussion (listing a few things not available in libstdc++4.6):
https://groups.google.com/a/chromium.org/forum/#!topic/cxx/vCxo4tZNd_M

We need to find out if we don't use the wheezy sysroot properly for parts of the
sources. Otherwise I thought that the features you used are all whitelisted.

Please don't bake in this CL to deeply yet until this is sorted out.

[Michael Achenbach, 2016-06-14 13:36:36]

On 2016/06/14 12:46:47, Michael Achenbach wrote:
> See also the list here:
> https://chromium-cpp.appspot.com/
> 
> And this discussion (listing a few things not available in libstdc++4.6):
> https://groups.google.com/a/chromium.org/forum/#!topic/cxx/vCxo4tZNd_M
> 
> We need to find out if we don't use the wheezy sysroot properly for parts of
the
> sources. Otherwise I thought that the features you used are all whitelisted.
> 
> Please don't bake in this CL to deeply yet until this is sorted out.

Never mind. Fix coming: https://codereview.chromium.org/2066823002/

[titzer, 2016-06-30 08:34:52]

On 2016/06/14 13:36:36, Michael Achenbach wrote:
> On 2016/06/14 12:46:47, Michael Achenbach wrote:
> > See also the list here:
> > https://chromium-cpp.appspot.com/
> > 
> > And this discussion (listing a few things not available in libstdc++4.6):
> > https://groups.google.com/a/chromium.org/forum/#!topic/cxx/vCxo4tZNd_M
> > 
> > We need to find out if we don't use the wheezy sysroot properly for parts of
> the
> > sources. Otherwise I thought that the features you used are all whitelisted.
> > 
> > Please don't bake in this CL to deeply yet until this is sorted out.
> 
> Never mind. Fix coming: https://codereview.chromium.org/2066823002/

Can we reland this CL now?

[Michael Achenbach, 2016-06-30 08:42:45]

On 2016/06/30 08:34:52, titzer wrote:
> On 2016/06/14 13:36:36, Michael Achenbach wrote:
> > On 2016/06/14 12:46:47, Michael Achenbach wrote:
> > > See also the list here:
> > > https://chromium-cpp.appspot.com/
> > > 
> > > And this discussion (listing a few things not available in libstdc++4.6):
> > > https://groups.google.com/a/chromium.org/forum/#!topic/cxx/vCxo4tZNd_M
> > > 
> > > We need to find out if we don't use the wheezy sysroot properly for parts
of
> > the
> > > sources. Otherwise I thought that the features you used are all
whitelisted.
> > > 
> > > Please don't bake in this CL to deeply yet until this is sorted out.
> > 
> > Never mind. Fix coming: https://codereview.chromium.org/2066823002/
> 
> Can we reland this CL now?

I never reverted it.