Closed Bug 1437733 Opened 2 years ago Closed 2 years ago

JSFunction::u::wasm is confusing and unnecessary

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(1 file, 1 obsolete file)

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.