Closed Bug 1501269 Opened 11 months ago Closed 11 months ago

Instruction caches aren't flushed on arm64 Windows

Categories

(Core :: JavaScript Engine: JIT, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

vixl::CPU::EnsureIAndDCacheCoherency is not implemented on arm64 Windows: the code is behind `#ifdef __aarch64__` which isn't defined (instead Windows uses `_M_ARM64`). And we can't simply change the ifdef because the Windows build wouldn't understand the inline asm anyway.

This leads to super-perplexing crashes where the CPU executes a garbage instruction from a stale cache, but by the time the debugger breaks in to complain about it, the caches have been updated and the code seems totally fine. Thanks to lth for helping to track this down.
Instead of commenting/assuming we're running on the simulator, it would have been nice if they had asserted that :)

  // If the host isn't AArch64, we must be using the simulator, so this function
  // doesn't have to do anything.
  USE(address, length);

Also there's a similar #ifdef in CPU::GetCacheType?
> Also there's a similar #ifdef in CPU::GetCacheType?

Yeah, but as far as I can tell, it's (currently) only setting up the cache line size to be used by EnsureIAndDCacheCoherency.
In my local builds I got around this by using the Windows API FlushInstructionCache in EnsureIAndDCacheCoherency if _M_ARM64 is defined. Are we allowed to land changes in our copy of the vixl code? Or would it be better to do this at the callsite at https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/js/src/jit/ExecutableAllocator.h#308 ?
See comment 3.
Flags: needinfo?(sstangl)
I'm going to be re-importing VIXL soon, due to ARMv8.3 updates. So it's likely that changes will get clobbered. In general, updating VIXL on our side is not safe, because it's not easy to recover the diff to VIXL trunk.

Usually what we've done is make "Moz" versions of the VIXL files. So for example, instead of changing VIXL simulator code, we have `js/src/jit/arm64/vixl/MozSimulator-vixl.cpp`, which is just `js/src/jit/arm64/vixl/Simulator-vixl.cpp` with a "Moz" prefix. Then instead of using the VIXL version, we use the "Moz" version.

In this case it's probably a bad idea to keep around a version of cacheFlush() that doesn't actually flush the cache on some systems. What I would suggest is to delete the current implementation and re-implement cacheFlush() in a "Moz" file, within the exact same namespace, such that if I re-imported VIXL without modification, the build would fail due to conflicting symbols.
Flags: needinfo?(sstangl)
It might be worth noting that VIXL upstream is https://git.linaro.org/arm/vixl.git, but they don't have an issue tracker as far as I can tell. Jacob Bramley has historically been receptive to this kind of thing -- it would be good to report the issue to him, at jacob.bramley@arm.com.
Priority: -- → P1
Assignee: nobody → dmajor
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5c10f26e38c
Make EnsureIAndDCacheCoherency work on aarch64-windows. r=sstangl
https://hg.mozilla.org/mozilla-central/rev/c5c10f26e38c
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.