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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacob Bramley [:jbramley]]

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.

Comment 1

[Jacob Bramley [:jbramley]]

Attachment: 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.

Comment 2

[Jacob Bramley [:jbramley]]

Attachment: 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.

Comment 3

[Jacob Bramley [:jbramley]]

Needs check-in on tracemonkey.

Comment 4

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/f272a9e9dad6

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f272a9e9dad6