Closed Bug 1389471 Opened 2 years ago Closed 2 years ago

Preliminaries for Wasm threads


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox57 --- fixed


(Reporter: lth, Assigned: lth)


(Blocks 1 open bug)



(1 file)

Changes to to define a suitable preprocessor macro (ENABLE_WASM_THREAD_OPS) and a TestingFunction to test for presence of threads.

This is small potatoes but I'm breaking this out as a separate bug since several of the other changes depend on it, and it can land independently really.
See comment 0.
Attachment #8896246 - Flags: review?(bbouvier)
Comment on attachment 8896246 [details] [diff] [review]

Review of attachment 8896246 [details] [diff] [review]:

Thanks, LGTM.

::: js/src/builtin/TestingFunctions.cpp
@@ +533,5 @@
> +WasmThreadsSupported(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    bool isSupported = true;

This is temporary that it's always true, right? e.g. for mips/arm64/none codegen, there won't be any atomics instructions any time soon, right?

(And I assume that probably other preconditions will apply here, other than just the ifdef)

::: js/src/
@@ +643,5 @@
> +# In-flight WebAssembly atomic operations, sign extension operations,
> +# and shared memory objects proposal:
> +#

(might be ok to group with the above group of if[NIGHTLY], since it seems to be the common case in this file; although wasm thread ops are muuuuuch more likely to land in release sooner than binary data/simd, so it makes sense to start a new `if` group; I'll leave it to your preference)
Attachment #8896246 - Flags: review?(bbouvier) → review+
Excellent point about the always-trueness of the return value...  Hm.  The right fix is perhaps to engage in some ifdeffery on the level (or similar) so that for ARM64, MIPS, et al we don't risk anything... on the other hand, we MOZ_CRASH for JS atomics on ARM64 and MIPS and could probably do that here too...  Would certainly be cleaner.  Certainly the wasm atomics implementations will crash on those platforms.

At a minimum this should not return true if wasm support is disabled, oy!  Will fix.

Re the new CONFIG block, it was mainly to have a nice point for the comment.  And as you say, this stuff is likely to land soon :)
Pushed by
Build config + testing predicate for wasm thread functionality, v2. r=bbouvier
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.