893552 - OdinMonkey: (ARM) dynamic code is not preserving float registers d8 to d15 as required by the ABI.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Douglas Crosher [:dougc]]

The ARM ABI requires that float registers d8 to d15 be preserved across function calls, but this does not appear to be done.  The Ion trampolines do not appear to save and restore these registers, and the trampolines into Odin code do not either, yet the dynamic code assumes these registers can be destructively modified?

Comment 1

[Marty Rosenberg [:mjrosenb]]

I think I know why I had been assuming that the trampoline code saved and restored the vfp registers.  The bailout code does that explicitly (which is why I remember writing the code to begin with).  The reason that the double registers were not saved/restored around the enterjit calls was because all of the functions that called enterjit didn't have any double code, so there was no reason to actually save their values.  An optimization could have changed that, but there were never any warning signs from the fuzzers.  Should probably still be fixed.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: preserve-floats

Here's a patch to do this.

The Odin code makes use of FloatRegisters::NonVolatileMask so just changing this constant should fix that case.

For Ion/Baseline I updated the code in Trampoline-arm.cpp.

Comment 3

[Douglas Crosher [:dougc]]

It's getting very confusing because the Ion and AsmJS calling conventions differ from the ABI conventions, yet they are using the same defined sets.

Might changing the VolatileMask here change the Ion and AsmJS calling conventions?  If so then perhaps this needs more discussion?

Comment 4

[Douglas Crosher [:dougc]]

Perhaps another option to consider is using all 32 FP registers when available but not touching d8-d15 and thus avoiding the need to save and restore them.  This would give more registers than are currently used and not slow down the entry and exits.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 781607 [details] [diff] [review]
preserve-floats

Review of attachment 781607 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.  Do we actually have a test case where this causes problems?  This will make all of Asm.js -> IM, Interp -> IM and BC -> IM calls slower, but I'm pretty sure we're already prepared for these transitions not being the fastest in the world.  If this gives too much of a headache, it may be possible to push these to the caller side for ASM.js, and go back to hoping we don't need it on the other calls.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Douglas Crosher [:dougc] from comment #3)

As far as I can see, these masks are used in the trampoline code used for entry/exit and the registers saved around use of callWithABI() only, so I think we're ok.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Marty Rosenberg [:mjrosenb] from comment #5)

> Do we actually have a test case where this causes problems? 

I tried to put together a test case for this, but I couldn't get GCC to generate any code that used more than d6 and d7 at the same time, so I was unable to set up a situation where the problem would be apparent.

> This will make all of Asm.js -> IM, Interp -> IM and BC -> IM calls slower

Yes but also it should also speed up anything called with callWithABI() as it will not save the non volatile float registers any more.

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80e1116e4599

Comment 9

[Jon Coppeard (:jonco)]

Backed out again for causing Android failures on opt builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8af597df1d0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

clobbering didn't help, fwiw

Comment 11

[Jon Coppeard (:jonco)]

I missed the fact that on return the stack pointer is adjusted to skip over the r0 value on the stack (which is not restored).  This needs to happen after the FP registers are loaded and not before.  I'm surprised this didn't cause more failures.

Fixed and pushed again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2dc96c6efa5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2dc96c6efa5