1437733 - JSFunction::u::wasm is confusing and unnecessary

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jim Blandy :jimb]

The members of the `wasm` member of `JSFunction::u` are required to occupy certain offsets within the structure in order to alias other members, yet this aliasing could easily be accomplished by simply reusing existing fields (which already have the correct type) or placing the fields in the same union (which necessarily locates them at the same offset).

Perhaps the intention behind the present structure is to clearly establish the relevant fields for different sorts of functions. But the code does not actually use the fields consistently with this intent: for example, `JSFunction::u::wasm::native_` is only ever written to, and never read; the data stored in it is always retrieved via `JSFunction::u::native::func_`. When I encountered this code this morning, I was bewildered until someone pointed out the required aliasing.

Comment 1

[Jim Blandy :jimb]

Attachment: Merge JSFunction::u::wasm with other members.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8950455 [details] [diff] [review]
Merge JSFunction::u::wasm with other members.

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

Yeah, we switched designs a few times, so indeed the wasm part of the union doesn't seem necessary anymore. Nice cleanup, thanks.

::: js/src/jsfun.h
@@ +127,5 @@
>                  // used if isBuiltinNative(); use the accessor!
>                  const JSJitInfo* jitInfo_;
>                  // asm.js function index, only used if isAsmJSNative().
>                  size_t asmJSFuncIndex_;
> +                // for wasm, A pointer to a fast jit->wasm table entry.

nit: "a" lowercase

@@ +128,5 @@
>                  const JSJitInfo* jitInfo_;
>                  // asm.js function index, only used if isAsmJSNative().
>                  size_t asmJSFuncIndex_;
> +                // for wasm, A pointer to a fast jit->wasm table entry.
> +                void** jitEntry_;

nit: maybe rename wasmJitEntry_ to make it overly explicit?

Comment 3

[Jim Blandy :jimb]

Attachment: Merge JSFunction::u::wasm with other members. r=bbouvier

Revised per comments. Carrying over r=bbouvier.

Comment 4

[Jim Blandy :jimb]

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

Comment 5

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

This got bitrotted by bug 1429206, please provide an updated patch.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8d3617e884
Merge JSFunction::u::wasm with other members. r=bbouvier

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/de8d3617e884