1268725 - Baldr: refactor the internal state out of ExprIter

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

Attachment: wasm-iter-refactor.patch

Here's the iterator refactoring patch we discussed, that removes the internal state from ExprIter and adds out parameters to the read* functions.

The internal state during the early design phase for this code, but as the code grew, it became increasingly awkward.

This patch uses a lot of nullptr argument defaults. These are handy for the Wasm.cpp validation code because it doesn't need most of these values, but they do make the interface more slippery for other users. I'm debating removing these and just making Wasm.cpp declare a bunch of variables that are then unused.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8746851 [details] [diff] [review]
wasm-iter-refactor.patch

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

Yes, that looks really nice!  To further tidy WasmBinaryIterator.h, how about doing that #ifdef DEBUG OpcodeKind Classify(Expr) that is a (non-template) free function defined in WasmBinaryIterator.cpp by 1 big switch, and then use that to assert everywhere in WasmBinaryIterator.h instead of all the isX() functions?

Comment 2

[Dan Gohman [:sunfish]]

Attachment: wasm-iter-refactor.patch

- Implemented the opcode categorizing debug code out of the header file.
 - Rebased on trunk.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: wasm-iter-refactor.patch

And, updated with a few last details from the review. This version includes the out-of-line Classify function as discussed.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: wasm-iter-null-params.patch

This is an additional patch which performs one other refactoring. It removes the default-argument nullptrs in WasmBinaryIterator.h, making the API friendlier for consumers that actually want to use them, and adds explicit nullptr parameters to Wasm.cpp, which is probably always going to be the only API consumer that doesn't want to use the values.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8747317 [details] [diff] [review]
wasm-iter-refactor.patch

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

Lovely!

::: js/src/asmjs/WasmBinaryIterator.cpp
@@ +22,5 @@
> +using namespace js::jit;
> +using namespace js::wasm;
> +
> +#ifdef DEBUG
> +OpcodeKind js::wasm::Classify(Expr expr)

\n after OpcodeKind.  Also, you don't need the js:: so I'd remove.

::: js/src/asmjs/WasmBinaryIterator.h
@@ +33,5 @@
>  enum class LabelKind : uint8_t { Block, Loop, Then, Else };
>  
> +#ifdef DEBUG
> +// Families of opcodes that share a signature and validation logic.
> +enum class OpcodeKind {

How about ExprKind?

@@ +83,3 @@
>  };
>  
> +OpcodeKind Classify(Expr expr);

\n before Classify.  Perhaps with short comment to explain the purpose?

@@ +228,2 @@
>  #ifdef DEBUG
>      Expr expr_;

Unrelated, and feel free to ignore: how about making DebugOnly (and marking ExprIter as MOZ_STACK_CLASS since DebugOnly is MOZ_STACK_CLASS) so we can remove all the #ifdefs?

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8747318 [details] [diff] [review]
wasm-iter-null-params.patch

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

Makes sense given that the goal state here is that Wasm.cpp doesn't do any validation of function bodies (baseline would), so then we could remove Output altogether since all consumers would be doing something.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Had to back this out for build bustage.

Backouts:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a5c0d7430e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eccc01455ff

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d33dcbacb46d4ed78fd63f4793a98f0b08eed016
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=27053292&repo=mozilla-inbound (OS X Tier 2 builders failed to build, so the same thing will likely also happen to Tier 1)

19:23:05     INFO -  gmake[9]: Leaving directory `/home/worker/workspace/build/src/security/nss/lib/libpkix/pkix/checker'
19:23:05     INFO -  cd params; /usr/bin/gmake libs
19:23:05     INFO -  In file included from /home/worker/workspace/build/src/obj-firefox/js/src/Unified_cpp_js_src1.cpp:11:
19:23:05     INFO -  In file included from /home/worker/workspace/build/src/js/src/asmjs/WasmIonCompile.cpp:21:
19:23:05     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinaryIterator.h:223:32: error: expected unqualified-id
19:23:05     INFO -  class ExprIter MOZ_STACK_CLASS : private Policy
19:23:05     INFO -                                 ^
19:23:05     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinaryIterator.h:527:1: error: 'inline' can only appear on functions
19:23:05     INFO -  inline bool
19:23:05     INFO -  ^
19:23:05     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinaryIterator.h:528:1: error: variable templates are a C++14 extension [-Werror,-Wc++14-extensions]
19:23:05     INFO -  ExprIter<Policy>::typeMismatch(ExprType actual, ExprType expected)
19:23:05     INFO -  ^
19:23:05     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinaryIterator.h:528:9: error: expected ';' at end of declaration
19:23:05     INFO -  ExprIter<Policy>::typeMismatch(ExprType actual, ExprType expected)
19:23:05     INFO -          ^
19:23:05     INFO -          ;
19:23:05     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinaryIterator.h:528:9: error: expected unqualified-id
19:23:05     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinaryIterator.h:540:17: error: qualified name refers into a specialization of variable template 'ExprIter'
19:23:05     INFO -  ExprIter<Policy>::checkType(ExprType actual, ExprType expected)
19:23:05     INFO -  ~~~~~~~~~~~~~~~~^
19:23:05     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinaryIterator.h:528:1: note: variable template 'ExprIter' declared here
19:23:05     INFO -  ExprIter<Policy>::typeMismatch(ExprType actual, ExprType expected)
19:23:05     INFO -  ^

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b77d0263a15

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ada16ac63d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb0e0154d5c

Comment 11

[Dan Gohman [:sunfish]]

(In reply to Luke Wagner [:luke] from comment #5)
> Unrelated, and feel free to ignore: how about making DebugOnly (and marking
> ExprIter as MOZ_STACK_CLASS since DebugOnly is MOZ_STACK_CLASS) so we can
> remove all the #ifdefs?

Done. Resubmitting with the MOZ_STACK_CLASS in the correct place relative to the class name.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out again for bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/90382fbdad00

https://treeherder.mozilla.org/logviewer.html#?job_id=27076311&repo=mozilla-inbound

Comment 13

[Dan Gohman [:sunfish]]

Ok, I believe I've really fixed all the MSVC issues now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeac42b93f75

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1931bd636f17
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8a11b8b6bbd

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1931bd636f17
https://hg.mozilla.org/mozilla-central/rev/c8a11b8b6bbd