Closed Bug 1441436 Opened 2 years ago Closed 5 months ago

ARM64: Simulator should incorporate icache checking

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: lth, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

On ARM and MIPS the instruction-level simulators incorporate a mode for checking that we handle the instruction cache properly when we compile and patch.  We might wish to incorporate this for ARM64 as well.

Work items, roughly:

- existing patch points are under control of JS_SIMULATOR_ARM macro
- some obvious code in JitInterruptHandler that must be adapted
- some obvious code in ExecutableAllocator.h
- some shell support (command line flags, as in --arm-sim-icache-checks)
- needs support in the simulator engine
Blocks: 1445162
No longer blocks: Fennec-ARM64

I am currently working on this issue, because the last time we had to track a similar issue was a complete nightmare and a lucky finding. We should avoid relying on luck for finding these nasty bugs.

Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED

(In reply to Nicolas B. Pierron [:nbp] from comment #2)

Created attachment 9044252 [details]
Bug 1441436 - Draft: Add a Cahcing decoder to catch miss instruction cache invalidation. r=

At the moment this patch is still a work-in-progress and does not contain any mechanism for handling the invalidation (in order to test if it can catch missing invalidation cases).

It successfully catch any rewritten instruction and assert if there is a mismatch between the instruction which used to be present and the current instruction.

I designed this code also as a way to potentially speed-up the decoding phase of the simulator, therefore instead of storing a yes/no boolean like done on ARM/MIPS to check the validity of the recorded instruction, I record the name of the visitor to which it should be dispatched to. Also, to avoid going through the hash-map lookup all the time, the last hash-map hit is cached.

This patch is also meant to minimize the number of lines modified in the imported vixl files. This is achieved by adding one extra line to the Simulator class.

using Decode = CachingDecoder;
Blocks: 1521158
Attachment #9044252 - Attachment is obsolete: true
Attachment #9044252 - Attachment description: Bug 1441436 - Draft: Add a Cahcing decoder to catch miss instruction cache invalidation. r= → Bug 1441436 - ARM64 Simulator: Add D&I cache coherency checks.
Attachment #9044252 - Attachment is obsolete: false
Attachment #9045009 - Attachment is obsolete: true
Attachment #9045009 - Attachment is obsolete: false
Attachment #9044252 - Attachment is obsolete: true

Nicolas, did you mean to abandon https://phabricator.services.mozilla.com/D19970?

That one seems more recent than https://phabricator.services.mozilla.com/D20378 (which is not abandoned but does not have a reviewer).

Now that Ion has landed it would be appropriate to land these checks.

(In reply to Sean Stangl [:sstangl] from comment #5)

Nicolas, did you mean to abandon https://phabricator.services.mozilla.com/D19970?

That one seems more recent than https://phabricator.services.mozilla.com/D20378 (which is not abandoned but does not have a reviewer).

No, I meant the opposite, since https://phabricator.services.mozilla.com/D20378 should now be part of https://phabricator.services.mozilla.com/D19970 .

Attachment #9044252 - Attachment is obsolete: false
Attachment #9045009 - Attachment is obsolete: true
Attachment #9045009 - Attachment is obsolete: false
Attachment #9045009 - Attachment is obsolete: true
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aadb254e2a5a
ARM64 Simulator: Add D&I cache coherency checks. r=sstangl
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.