TraceMonkey: The ARM-specific _nSlot pointer should be reset along with _nIns.

RESOLVED FIXED

Status

()

--
major
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jbramley, Assigned: jbramley)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
In Assemler.cpp, _nIns is reset in a few places where error() indicates that something went wrong with the code generation. However, on the ARM platform, there is additional pointer — _nSlot — which is used to write data into the literal pool. However, much of the generated code relies on _nIns and _nSlot being within a certain range of one another, and in practice this means that they must remain on the same page.

underrunProtect ensures that this happens, but when an error() occurs, _nIns is simply reset and _nSlot is not touched. This event will be rare, and indeed it does not occur in the js/tests tests or in trace-tests.js, but when it does happen it will cause data corruption or a crash of some kind.

I have a fix for this but have not yet produced a tidy patch.

Note that it is not easy to reproduce this problem without adding instrumentation to the code to force an error() to occur. As a result, it is not possible to write a JavaScript test in trace-tests.js to catch this. I will, however, upload a patch which can be used to trigger the bug.
(Assignee)

Comment 1

10 years ago
Created attachment 370859 [details] [diff] [review]
A patch that adds instrumentation to allow the bug to be triggered. Not to be commited to the repository!

NOTE: This patch isn't intended to be committed to the repository.

The patch will add instrumentation to Assembler.cpp to allow the _nSlot bug to be triggered. It does this by forcing the error state to be set periodically. I can reliably reproduce this bug by running trace-tests.js with this patch applied and the JIT enabled.
(Assignee)

Comment 2

10 years ago
Created attachment 370879 [details] [diff] [review]
Reset _nSlot when _nIns is reset on ARM.

Note that the patch also modifies the Nativei386 and NativeSparc code. The change is trivial, but I am not able to test the SPARC version.

I have added two functions which Native*.cpp files must implement: recordStartingInstructionPointer and resetInstructionPointer.

Note that you must apply my instrumentation patch (370859) and run trace-tests.js to actually test this, as described in my previous comment.
Attachment #370879 - Flags: review?(vladimir)

Updated

10 years ago
Attachment #370879 - Flags: review?(edwsmith)

Updated

10 years ago
Attachment #370879 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 3

10 years ago
Needs check-in on tracemonkey.
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 4

10 years ago
http://hg.mozilla.org/tracemonkey/rev/f272a9e9dad6
Keywords: checkin-needed → 4xp
Whiteboard: fixed-in-tracemonkey

Updated

10 years ago
Keywords: 4xp

Comment 5

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