NJ merge: adjust asm_ld_imm assertion

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: graydon, Assigned: graydon)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

(Assignee)

Description

9 years ago
Assembler::asm_ld_imm in NativeARM.cpp does the following:

- Writes the literal to *(_nSlot++)
- Writes an offset-load to the literal to *(--_nIns)

Explanation:
------------

- _nSlot is at a lower address and grows upwards towards
  _nIns

- _nIins is at a higher address and grows downwards towards
  _nSlot

- they grow together, and can get as close as touching, but
  must not pass each other

- the instruction being written is a 12-bit offset from "PC"
  packed into a 32-bit word

- PC points to "current instruction + 8"

- but offset is calculated from _nIns - 2. IOW offset is
  just (_nSlot - _nIns)


Diagram:
--------

       imm       load
 [____|____|____|____|____|____|____]
      ^              ^
     nslot          nins


Difference in code:
-------------------

Our code differs from Adobe's only in an assert:

-    NanoAssert(isS12(offset) && (offset < 0));
+    NanoAssert(isS12(offset) && (offset < -8));

Clearly the first assert is not right, as the offset needs
to be at least -8. The second assert seems a little too
tight though, as I think an offset of exactly -8 would still
function, and is probably achievable in rare cases. So I'd
set it to <= -8. But I'm willing to hear an argument to the
contrary.

Any opinions?
(Assignee)

Comment 1

9 years ago
Discussed and reviewed on IRC, set to <= -8, tested, seems ok. Worst case it's just a wrong assert and won't harm any production builds anyway.

http://hg.mozilla.org/tracemonkey/rev/47fddf8e644c

Adobe folks: please either absorb this back or tell us how our logic's off here :)
Whiteboard: fixed-in-tracemonkey

Comment 2

9 years ago
Sounds good to me, will change tamarin code.

Comment 3

9 years ago
sounds ok

Comment 4

9 years ago
http://hg.mozilla.org/mozilla-central/rev/47fddf8e644c
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.