wasm: parsing for load and store operators

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
The following set of patches implements parsing and some amount of encoding and validation for loads and stores. It's not finished yet though, as finishing this will require refactoring the asm.js code, which is a larger change.
(Assignee)

Updated

2 years ago
Depends on: 1244405
(Assignee)

Comment 1

2 years ago
Created attachment 8717706 [details] [diff] [review]
wasm-reduce-indentation.patch

Just a cleanup patch. next() was getting big and deeply nested, so I moved it out of the class definition.
Attachment #8717706 - Flags: review?(luke)
(Assignee)

Comment 2

2 years ago
Created attachment 8717707 [details] [diff] [review]
wasm-anonymous-namespaces.patch

This makes all the WasmText.cpp-local classes local.
Attachment #8717707 - Flags: review?(luke)
(Assignee)

Comment 3

2 years ago
Created attachment 8717710 [details] [diff] [review]
wasm-load-store.patch

This adds parsing and some encoding and decoding support for load and store. It's not usable yet, as this will require some significant asm.js refactoring, but this patch gets things started.
Attachment #8717710 - Flags: review?(luke)

Comment 4

2 years ago
Comment on attachment 8717706 [details] [diff] [review]
wasm-reduce-indentation.patch

Review of attachment 8717706 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea
Attachment #8717706 - Flags: review?(luke) → review+

Comment 5

2 years ago
Comment on attachment 8717707 [details] [diff] [review]
wasm-anonymous-namespaces.patch

Review of attachment 8717707 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmText.cpp
@@ +2227,4 @@
>  /*****************************************************************************/
>  // wasm function body serialization
>  
> +namespace {

Normally we don't unnammed namespace our static functions.  I had even thought maybe it wasn't technically valid (to put a static function inside a namespace, but maybe there is a special case for unnamed?).  So if there isn't a big reason, could you remove this and the one below?
Attachment #8717707 - Flags: review?(luke) → review+

Comment 6

2 years ago
Comment on attachment 8717710 [details] [diff] [review]
wasm-load-store.patch

Review of attachment 8717710 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff.

::: js/src/asmjs/Wasm.cpp
@@ +350,5 @@
> +    uint32_t offset, align;
> +    return DecodeExpr(f, ExprType::I32) &&
> +           f.d().readVarU32(&offset) &&
> +           f.d().readVarU32(&align) &&
> +           mozilla::IsPowerOfTwo(align) &&

I'm afraid we need to brake this pretty && chain and report a validation message for !power-of-two alignment.

::: js/src/asmjs/WasmText.cpp
@@ +2095,5 @@
>  }
>  
> +static bool
> +ParseLoadStoreAddress(WasmParseContext& c, WasmAstExpr*& base,
> +                      int32_t& offset, int32_t& align)

The usual SM/Odin style is to always pass outparams by *, not &, since this makes it syntactically evident at the callsite since you pass "&x" instead of "x".

@@ +2116,5 @@
> +          case WasmToken::UnsignedInteger:
> +            offset = val.uint();
> +            break;
> +          default:
> +            return false;

Can you c.ts.generateError() here and a few other places below?  I also forgot to look for this in the parse-constants patch, so perhaps you could fix them up too?

@@ +2132,5 @@
> +          case WasmToken::Index:
> +            align = val.index();
> +            break;
> +          default:
> +            return false;

Here, we fail for WasmToken::UnsignedInteger.  Above, UnsignedInteger is implicitly truncated to uint32.  In ParseConst(), it is assigned to a CheckedInt<uint32_t> and checked for validity.  How about you factor out a TokenStream::matchU32() which does the switch and calls generateError.  That should shorten the code too.
Attachment #8717710 - Flags: review?(luke) → review+
(Assignee)

Comment 8

2 years ago
(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8717710 [details] [diff] [review]
> wasm-load-store.patch
> 
> Review of attachment 8717710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good stuff.
> 
> ::: js/src/asmjs/Wasm.cpp
> @@ +350,5 @@
> > +    uint32_t offset, align;
> > +    return DecodeExpr(f, ExprType::I32) &&
> > +           f.d().readVarU32(&offset) &&
> > +           f.d().readVarU32(&align) &&
> > +           mozilla::IsPowerOfTwo(align) &&
> 
> I'm afraid we need to brake this pretty && chain and report a validation
> message for !power-of-two alignment.

Arg. I somehow dropped this comment. I'll fix this in a separate patch.

> ::: js/src/asmjs/WasmText.cpp
> @@ +2095,5 @@
> >  }
> >  
> > +static bool
> > +ParseLoadStoreAddress(WasmParseContext& c, WasmAstExpr*& base,
> > +                      int32_t& offset, int32_t& align)
> 
> The usual SM/Odin style is to always pass outparams by *, not &, since this
> makes it syntactically evident at the callsite since you pass "&x" instead
> of "x".

Fixed.

> @@ +2116,5 @@
> > +          case WasmToken::UnsignedInteger:
> > +            offset = val.uint();
> > +            break;
> > +          default:
> > +            return false;
> 
> Can you c.ts.generateError() here and a few other places below?  I also
> forgot to look for this in the parse-constants patch, so perhaps you could
> fix them up too?

Fixed, and fixed the constants too.

> @@ +2132,5 @@
> > +          case WasmToken::Index:
> > +            align = val.index();
> > +            break;
> > +          default:
> > +            return false;
> 
> Here, we fail for WasmToken::UnsignedInteger.  Above, UnsignedInteger is
> implicitly truncated to uint32.  In ParseConst(), it is assigned to a
> CheckedInt<uint32_t> and checked for validity.  How about you factor out a
> TokenStream::matchU32() which does the switch and calls generateError.  That
> should shorten the code too.

The UnsignedInteger case getting truncated was in the patch by accident. I've removed that now. Factoring out a matchU32 is tricky because there aren't that many places that need it -- just consts and load offsets, and they need different things. I think the code is pretty clean now, but of course I'm open to further feedback.

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd93d21491ea
https://hg.mozilla.org/mozilla-central/rev/a3b4e84a812f
https://hg.mozilla.org/mozilla-central/rev/5469639356d3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.