Closed Bug 1268725 Opened 4 years ago Closed 4 years ago

Baldr: refactor the internal state out of ExprIter

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch wasm-iter-refactor.patch (obsolete) — Splinter Review
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.
Attachment #8746851 - Flags: feedback?(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?
Attachment #8746851 - Flags: feedback?(luke) → feedback+
Attached patch wasm-iter-refactor.patch (obsolete) — Splinter Review
- Implemented the opcode categorizing debug code out of the header file.
 - Rebased on trunk.
Attachment #8746851 - Attachment is obsolete: true
And, updated with a few last details from the review. This version includes the out-of-line Classify function as discussed.
Attachment #8747302 - Attachment is obsolete: true
Attachment #8747317 - Flags: review?(luke)
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.
Attachment #8747318 - Flags: review?(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?
Attachment #8747317 - Flags: review?(luke) → review+
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.
Attachment #8747318 - Flags: review?(luke) → review+
Duplicate of this bug: 1269223
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 -  ^
Flags: needinfo?(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.
Flags: needinfo?(sunfish)
Ok, I believe I've really fixed all the MSVC issues now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeac42b93f75
Flags: needinfo?(sunfish)
https://hg.mozilla.org/mozilla-central/rev/1931bd636f17
https://hg.mozilla.org/mozilla-central/rev/c8a11b8b6bbd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.