JSFunction::u::wasm is confusing and unnecessary

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Assignee: nobody → jimb
Attachment #8950455 - Flags: review?(bbouvier)
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?
Attachment #8950455 - Flags: review?(bbouvier) → review+
Revised per comments. Carrying over r=bbouvier.
Attachment #8950455 - Attachment is obsolete: true
Attachment #8950727 - Flags: review+
Blocks: 1434305
Keywords: checkin-needed
Status: NEW → ASSIGNED
This got bitrotted by bug 1429206, please provide an updated patch.
Flags: needinfo?(jimb)
Keywords: checkin-needed
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8d3617e884
Merge JSFunction::u::wasm with other members. r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/de8d3617e884
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(jimb)
You need to log in before you can comment on or make changes to this bug.