Issue 2071343003: V8. ASM-2-WASM. Validator V2.

Issue 2071343003: V8. ASM-2-WASM. Validator V2. (Closed)

Created:
4 years, 6 months ago by John

Modified:
4 years, 5 months ago

Reviewers:
bradnelson, bradn

CC:
v8-reviews_googlegroups.com

Base URL:
https://chromium.googlesource.com/v8/v8.git@master

Target Ref:
refs/pending/heads/master

Project:
v8

Visibility:
Public.

More Reviews

Description

V8. ASM-2-WASM. Validator V2. This is a rewrite of the ASM validator. This one follows the spec instead of using the AST visitors. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 TEST=cctest/asmjs/test-asm-typer TEST=cctest/asmjs/test-typing-asm LOG=N Committed: https://crrev.com/974f4a8059ba27f1298c0050067eb7a9ca575b81 Cr-Commit-Position: refs/heads/master@{#37694}

Patch Set 1 : entire spec implemented. #

Patch Set 2 : fixes ValidateCase #

Patch Set 3 : Ready to review, missing most tests. #

Patch Set 4 : tests function tables and module exports. #

Total comments: 121

Patch Set 5 : addresses comments. #

Patch Set 6 : tests most error paths #

Patch Set 7 : Adds a few expression validation tests. Fixes some uncovered bugs, and TODOes others. #

Patch Set 8 : Implements the last unittests. #

Patch Set 9 : Adds stack overflow checks #

Patch Set 10 : Fixes breakage due to asm-types relocation. #

Patch Set 11 : fixes heap-use-after-free #

Patch Set 12 : configures the parserinfo before creating the parser. #

Patch Set 13 : removes constexpr #

Patch Set 14 : git pull #

Patch Set 15 : fixes the inability to call foreign functions with integers #

Patch Set 16 : removes references to old asm2wasm files #

Patch Set 17 : Adds a few methods needed by the asm2wasm translator. #

Total comments: 14

Patch Set 18 : addresses comments. #

Patch Set 19 : Moving code around. #

Patch Set 20 : Reverts all changes in test-asm-validator. #

Patch Set 21 : moves test-asm-validator to the asmjs cctest folder. #

Created: 4 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+4693 lines, -2654 lines)			Patch
M	BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/asmjs/OWNERS	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+2 lines, -0 lines	0 comments	Download
A	src/asmjs/asm-typer.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+325 lines, -0 lines	0 comments	Download
A	src/asmjs/asm-typer.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+2585 lines, -0 lines	0 comments	Download
M	src/asmjs/asm-types.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	5 chunks	+7 lines, -3 lines	0 comments	Download
M	src/v8.gyp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+2 lines, -0 lines	0 comments	Download
M	test/cctest/OWNERS	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+0 lines, -2 lines	0 comments	Download
A +	test/cctest/asmjs/OWNERS	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+2 lines, -0 lines	0 comments	Download
A	test/cctest/asmjs/test-asm-typer.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+1766 lines, -0 lines	0 comments	Download
A +	test/cctest/asmjs/test-typing-asm.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	89 chunks	+0 lines, -92 lines	0 comments	Download
M	test/cctest/cctest.gyp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	2 chunks	+2 lines, -1 line	0 comments	Download
D	test/cctest/test-asm-validator.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+0 lines, -2556 lines	0 comments	Download

Messages

Total messages: 28 (16 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

bradnelson

Much prettier! LGTM with lots of observations and nits. https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc File src/wasm/asm-typer.cc (right): https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#newcode46 src/wasm/asm-typer.cc:46: ...

4 years, 6 months ago (2016-06-24 00:19:04 UTC) #12

John

sub 100 comments on 2k+ lines? that's awesome! :) I addressed most of your concerns, ...

4 years, 6 months ago (2016-06-24 17:47:58 UTC) #13

bradn

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc File src/wasm/asm-typer.cc (right): https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#newcode534 src/wasm/asm-typer.cc:534: return AsmType::Int(); // Any type that is not AsmType::None(); ...

4 years, 6 months ago (2016-06-24 18:57:03 UTC) #15

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc
File src/wasm/asm-typer.cc (right):

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:534: return AsmType::Int();  // Any type that is not
AsmType::None();
On 2016/06/24 17:47:56, John wrote:
> On 2016/06/24 00:19:01, bradnelson wrote:
> > Actually, any reason to make this return a type? Why not a boolean?
> 
> Consistency is not usually a good reason, but it is here: all ValidateXXX
> methods return an AsmType* so that the RECURSE macro works for everything. I
> could do something like:
> 
> bool IsFailure(AsmType* type) {
>  return type == AsmType::None();
> }
> 
> bool IsFailure(bool b) { return b; }
> 
> #define RECURSE(call) do {  \
>   auto value = (call);      \
>   if (IsFailure(call)) {    \
>     return AsmType::None(); \
>   }                         \
> } while (0)
> 
> But that would not work because (1) some functions return bool, and if the
> compiler converts AsmType::None() to bool, we will get !false (because
> AsmType::None() != nullptr). That problem is also easily fixed: we just need a
> failure type that can be automatically converted to the right failure value:
> 
> struct failure {
>   failure();
>   /*implicit*/ operator AsmType* () const { return AsmType::None(); }
>   /*implicit*/ operator bool () const { return false; }
> };
> 
> so we could have
> 
> #define RECURSE(call) do { \
>   if (IsFailure(call)) {   \
>     return failure();      \
>   }                        \
> } while (0)
> 
> But, to be honest, I don't like this kind of cleverness in non-test harness
> code. Up to you, though/

Meh. Kind of like it as is too.
What if we changed None to Error / Failure or something?
That might make it more clear without much change.
I found myself occassionally confusing None and Void.

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:723: auto* initializer_callable =
var_info->type()->AsFunctionType();
On 2016/06/24 17:47:57, John wrote:
> On 2016/06/24 00:19:01, bradnelson wrote:
> > Don't we need to ban FFI ones?
> 
> From function tables? Yes, but the FFI type is not an AsmFunctionType, it is
an
> AsmCallableType. I left a DCHECK behind, just in case.

A confused the inheritance relationship. Makes sense.

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:935: auto* last_statement = fun->body()->last();
On 2016/06/24 17:47:56, John wrote:
> On 2016/06/24 00:19:01, bradnelson wrote:
> > Since we walk this all anyway, maybe no need to do this here (ie do at the
> end)?
> > Could be a TODO if you like.
> 
> That's what I thought, but then validating:
> 
> function foo() {
>   if (1) return 1.0;
> }
> 
> becomes a bit uglier. I decided this was probably better to do this here. Do
you
> want me to change it?

This is ok. Maybe put a TODO / NOTE that this won't cope with things like if the
block structure around returns changes in the ast.
(I suppose you could add a reverse iterator over the statements, but that seems
overkill too).

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:946: for (auto* decl : *fun->scope()->declarations()) {
On 2016/06/24 17:47:56, John wrote:
> On 2016/06/24 00:19:03, bradnelson wrote:
> > Maybe comment this revisits the ones above?
> 
> Does it? This visits the declarations for the current function, which only
> happens once?...

Oops, misread. You're right.

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:1154: AsmType*
AsmTyper::ValidateBreakStatement(BreakStatement* brk_stmt) {
On 2016/06/24 17:47:57, John wrote:
> On 2016/06/24 00:19:02, bradnelson wrote:
> > Starting to feel kind of odd that the statements return types.
> > I suppose it avoid a failure flag on the object, but I wonder if maybe it's
> more
> > confusing... Not sure. Leave it up to you.
> 
> I agree, but see above. There's a solution, but that solution seems like over
> engineering. I don't care one way or the other.

See above.

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:1530: FAIL(unop, "Invalid unary operator.");
On 2016/06/24 17:47:57, John wrote:
> On 2016/06/24 00:19:01, bradnelson wrote:
> > Minor nit, you're inconsistent about periods at the end of errors (probably
> > simpler not).
> 
> I thought I was being consistent... :(
> 
> I like the final '.', so I added the missing ones. For archeology, adding
these
> to the FAIL macro gives you a compiler-verified list of messages without the
> trailing '.':
> 
>     static constexpr char m[] = msg;                           \
>     static constexpr size_t m_size = arraysize(m);             \
>     static constexpr char last_char = msg[m_size - 2];         \
>     static_assert(last_char == '.', "");                       \
> 
> (hooray for static_assert!)

Wow.

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:1645: if (left_as_binop != nullptr && (left_as_binop->op()
== Token::ADD ||
On 2016/06/24 17:47:57, John wrote:
> On 2016/06/24 00:19:01, bradnelson wrote:
> > Maybe a TODO to consider handling this iteratively?
> > I suspect in practice the parser is recursive here, but if you really did
have
> > 2^20 you'd have a pretty deep stack.
> 
> I was going for iterative here, but then I ran into problems because + and -
> have different signatures, and thus whenever you validate a term, you need to
> know if that term is the lhs of a [+-], or a rhs of a +, or a rhs of a -.

Ick. Ok.
I've never seen more than 100s in practice.

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:1946: return base::bits::IsPowerOfTwo32(1 + *value);
On 2016/06/24 17:47:58, John wrote:
> On 2016/06/24 00:19:01, bradnelson wrote:
> > This will do the wrong thing for 2^31, no?
> 
> IsPowerOfTwo32(2 ^ 31) ===
>   return (2 ^ 31) && !((2 ^ 31) & ((2 ^ 31) - 1)) ===
>   return true && !(false)
> 
> So I believe it does the right thing here?

Sorry meant 2^32 - 1

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:2260: auto* call_var_info = Lookup(call_var_proxy->var());
On 2016/06/24 17:47:57, John wrote:
> On 2016/06/24 00:19:03, bradnelson wrote:
> > Isn't this one a DCHECK?
> 
> I don't think so. Why?

Oh wait, nevermind, you're right.

https://codereview.chromium.org/2071343003/diff/200001/test/cctest/test-asm-v...
File test/cctest/test-asm-validator.cc (right):

https://codereview.chromium.org/2071343003/diff/200001/test/cctest/test-asm-v...
test/cctest/test-asm-validator.cc:2525: 
On 2016/06/24 17:47:58, John wrote:
> On 2016/06/24 00:19:03, bradnelson wrote:
> > Maybe put this in test/wasm/test-asm-validator ?
> > The old one needs to go.
> 
> I left a TODO, for now. I'll move the file before landing.

Ok. Yeah better for review.

John

whew, I am done with the unittests. Adding them uncovered some bugs. The ones in ...

4 years, 5 months ago (2016-07-07 16:46:56 UTC) #16

whew, I am done with the unittests. Adding them uncovered some bugs. The ones in
asm-typer.cc were fixed in this CL, but a few were left behind.

I am pondering whether this CL should introduce declaration order enforcement
(i.e., imports and locals, then functions then function tables, then module
export.) There's a trivial way to enforce the order: one just need to store a
pair of <first, last> positions for the elements in each session, and then
complain if those intervals are not disjoint. No sorting needed. :)

This CL also uncovered - to me - some issues we might have with the parser, but
I added a comment whenever I found something funny. I wonder if we should just
change the parser so it does not modify the source code.

Fair warning: I might still add a few moar tests
1) make sure undefined forward references cause a
   validation failure; and
2) make sure forward references work;

But there are enough tests already for me to reasonably believe we're ready to
land.

(I am adding the stack check right now)

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc
File src/wasm/asm-typer.cc (right):

https://codereview.chromium.org/2071343003/diff/200001/src/wasm/asm-typer.cc#...
src/wasm/asm-typer.cc:534: return AsmType::Int();  // Any type that is not
AsmType::None();
On 2016/06/24 18:57:03, bradn wrote:
> On 2016/06/24 17:47:56, John wrote:
> > On 2016/06/24 00:19:01, bradnelson wrote:
> > > Actually, any reason to make this return a type? Why not a boolean?
> > 
> > Consistency is not usually a good reason, but it is here: all ValidateXXX
> > methods return an AsmType* so that the RECURSE macro works for everything. I
> > could do something like:
> > 
> > bool IsFailure(AsmType* type) {
> >  return type == AsmType::None();
> > }
> > 
> > bool IsFailure(bool b) { return b; }
> > 
> > #define RECURSE(call) do {  \
> >   auto value = (call);      \
> >   if (IsFailure(call)) {    \
> >     return AsmType::None(); \
> >   }                         \
> > } while (0)
> > 
> > But that would not work because (1) some functions return bool, and if the
> > compiler converts AsmType::None() to bool, we will get !false (because
> > AsmType::None() != nullptr). That problem is also easily fixed: we just need
a
> > failure type that can be automatically converted to the right failure value:
> > 
> > struct failure {
> >   failure();
> >   /*implicit*/ operator AsmType* () const { return AsmType::None(); }
> >   /*implicit*/ operator bool () const { return false; }
> > };
> > 
> > so we could have
> > 
> > #define RECURSE(call) do { \
> >   if (IsFailure(call)) {   \
> >     return failure();      \
> >   }                        \
> > } while (0)
> > 
> > But, to be honest, I don't like this kind of cleverness in non-test harness
> > code. Up to you, though/
> 
> Meh. Kind of like it as is too.
> What if we changed None to Error / Failure or something?
> That might make it more clear without much change.
> I found myself occassionally confusing None and Void.

Failure() makes sense in the context of validation, but it does not make sense
(to me) to have failure type. In a way, None() represents nullptr -- which I try
not to use for AsmType* variables. Will think a bit more about this.

bradn

still lgtm https://codereview.chromium.org/2071343003/diff/460001/src/asmjs/asm-types.h File src/asmjs/asm-types.h (right): https://codereview.chromium.org/2071343003/diff/460001/src/asmjs/asm-types.h#newcode52 src/asmjs/asm-types.h:52: /* DESIGN FLAW: FloatishDoubleQ should be a ...

4 years, 5 months ago (2016-07-11 17:55:34 UTC) #17

John

There'll be another patch moving test-asm-validator elsewhere more appropriate. https://codereview.chromium.org/2071343003/diff/460001/src/asmjs/asm-types.h File src/asmjs/asm-types.h (right): https://codereview.chromium.org/2071343003/diff/460001/src/asmjs/asm-types.h#newcode52 src/asmjs/asm-types.h:52: ...

4 years, 5 months ago (2016-07-12 21:13:15 UTC) #18

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-12 21:58:49 UTC) #23

commit-bot: I haz the power

4 years, 5 months ago (2016-07-12 23:12:24 UTC) #28

Message was sent while issue was closed.

Patchset 21 (id:??) landed as
https://crrev.com/974f4a8059ba27f1298c0050067eb7a9ca575b81
Cr-Commit-Position: refs/heads/master@{#37694}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages