TraceMonkey: Fixes for patched branches on ARM.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jbramley, Assigned: jbramley)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 370427 [details] [diff] [review]
Fix branch patching behaviour for corner-cases.

* For conditional branches where _nSlot and _nIns are not on the same page, a branch is emitted to jump over a literal in the instruction stream. However, the generated branch jumped to the wrong address as the PC on ARM is 8 bytes ahead of the current instruction address.
  - I don't think this code is ever run, but I don't yet understand the code well enough to be sure. If it is run, it will only occur on corner cases, so it will be hard to trigger.

* A check ensured that _nIns and _nSlot are on the same page, but _nIns is pre-decremented before being written to so it will start off on a different page. I have corrected this check.
  - Note that there is some code elsewhere that deliberately wastes a word at the top of the page because something is checking or writing to _nIns before decrementing it. That could be this check, though I haven't yet investigated it and there may be others.

* The branch generation code in B_cond_chk confused me as it isn't clear what the generated instructions were from the source. I added a comment to explain the different types of branch that it can emit, and it makes sense to leave it in the code for others to benefit from, so that is also in my patch.

* If the branch target is passed as 0, the real target is unknown. If _nIns is within 32MB of 0, B_cond_chk would attempt to insert a simple B instruction, and this would fail later when the branch was patched. This doesn't happen for me on Linux, but it is quite possible that it could happen on other platforms.

* I've also added a couple of asserts in case an unsupported (conditional) patching request is made.
(Assignee)

Updated

9 years ago
Assignee: general → Jacob.Bramley
(Assignee)

Updated

9 years ago
Attachment #370427 - Attachment filename: yyy → patch
Attachment #370427 - Flags: review?(vladimir)
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

9 years ago
Created attachment 384612 [details] [diff] [review]
Re-synchronization with tree.
Attachment #370427 - Attachment is obsolete: true
Attachment #384612 - Flags: review?(vladimir)
Attachment #370427 - Flags: review?(vladimir)
Attachment #384612 - Flags: review?(vladimir) → review+
(Assignee)

Updated

9 years ago
Whiteboard: fixed-in-tracemonkey

Comment 2

9 years ago
http://hg.mozilla.org/mozilla-central/rev/1788ec6fdc1f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.