Closed Bug 1412030 Opened 3 years ago Closed 3 years ago

[MIPS] Emit wasm memory access information

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Steps to reproduce:

Run jit_test.py   dist/bin/js wasm/errors.js on mips32



Actual results:

Test failing.


Expected results:

Test passing.
Attached patch bug1412030.patch (obsolete) — Splinter Review
Fixes CodeGenerator to emit memory access information for wasm loads and stores. Includes changes for partial writes to make first memory access to higher memory address in order to ensure that we don't get OOB trap while accessing lower memory. (see 1343981)
Attachment #8922407 - Flags: review?(lhansen)
Comment on attachment 8922407 [details] [diff] [review]
bug1412030.patch

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

Looks plausible.  I appreciate the extra effort to get the unaligned stores right :)

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +2021,5 @@
>          if (byteSize == 4) {
> +            masm.storeFloat32(value, address);
> +        } else {
> +            // For time being storeDouble for mips32 uses two store instructions,
> +            // so we emit only one to get correct beahvir in case of OOB access.behavior.

"correct behavior", and the "behavior" on the end of the line seems like it's left over from an edit.

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +554,5 @@
>  {
>      int16_t lowOffset, hiOffset;
>      Register base;
>  
>      asMasm().computeScaledAddress(src, SecondScratchReg);

As a general comment (for a future patch), you should really be using ScratchRegisterScope and SecondScratchRegisterScope to allocate the scratch registers in the macroassembler, it will make it much less likely that there's a confusion about who owns these registers.
Attachment #8922407 - Flags: review?(lhansen) → review+
Attachment #8922407 - Attachment is obsolete: true
Attachment #8922713 - Flags: review+
Keywords: checkin-needed
Assignee: nobody → dragan.mladjenovic
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db33caee757e
[MIPS] Emit wasm memory access information. r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/db33caee757e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.