Closed Bug 1406336 Opened 2 years ago Closed 2 years ago

Clean up compile-time address arithmetic in 32-bit code generators

Categories

(Core :: JavaScript Engine: JIT, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(1 file)

Plenty of 32-bit code generation code performs its own address arithmetic to compute the addresses in memory of the two words of a 64-bit value.  This code can usefully be centralized into assembler utility routines.  Once that's done, the new routines can use checked arithmetic, as an extra guard against overflow attacks.

Also, remove the special case for negative offsets for load64 from the 32-bit assemblers; it is wrong, but that code is never exercises so far as I can tell (and nbp agreed with that).
As discussed on IRC.  Fixes load64() on 32-bit platforms, and then tries to centralize address arithmetic into two new overloaded functions, LowWord and HighWord, which are defined on 32-bit systems and can operate on Address and BaseAddress cross-platform, and on Operand on x86.

Is the MOZ_ALWAYS_TRUE() in LowWord and HighWord desirable, or should we just rely on the MOZ_ASSERT in CheckedInt::value() to catch problems?

This patch does not kill all the ad-hoc arithmetic in the JIT, only the easier cases - after this, things tend to get more special-purpose.  Discuss.
Attachment #8915989 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8915989 [details] [diff] [review]
bug1406336-jit-address-arithmetic.patch

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

Really nice, thanks for fixing this issues!

::: js/src/jit/Registers.h
@@ +136,3 @@
>  }
>  
>  #if defined(JS_NUNBOX32)

nit: change this "#ifdef" to be "#if JS_BITS_PER_WORD == 32".

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +873,2 @@
>      size_t storeOffset = size();
> +    movl(value.low, LowWord(dstAddr));

follow-up nit: You have assertions for wasmLoadI64, maybe adding the same here?
Attachment #8915989 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Comment on attachment 8915989 [details] [diff] [review]
> bug1406336-jit-address-arithmetic.patch
> 
> ::: js/src/jit/x86/MacroAssembler-x86.cpp
> @@ +873,2 @@
> >      size_t storeOffset = size();
> > +    movl(value.low, LowWord(dstAddr));
> 
> follow-up nit: You have assertions for wasmLoadI64, maybe adding the same
> here?

Not necessary I think?  The dest is always a memory address, asserted just above.  And no registers are altered.
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd2fefd471b
JS Jits: Fix load64() on 32-bit platforms, and centralize address arithmetic. r=nbp
https://hg.mozilla.org/mozilla-central/rev/8dd2fefd471b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.