886966 - Odin ARM issue with many-argument FFI call

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://people.mozilla.com/~vladimir/misc/webgl-perf/hello-gles.html

With Odin enabled on the latest nightly, the gears are corrupted after a few frames (only half are drawn).  If asmjs gets disabled, they render fine.

My guess is some corruption in the asmjs heap, or maybe in memory overall (because the data is stored in a VBO).

Comment 1

[Luke Wagner [:luke]]

The graphical corruption sounds like what Douglas was reporting earlier (can't find the bug maybe that was in an email thread?).

Vlad: are you able to compile a build locally and, if so, does the problem repro if you put "return true" at the head of TryEnablingIon in ion/AsmJS.cpp?

Comment 2

[Luke Wagner [:luke]]

I can repro on a plain FF Nightly build on a Samsung Galaxy SII, btw.

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Luke Wagner [:luke] from comment #1)
> Vlad: are you able to compile a build locally and, if so, does the problem
> repro if you put "return true" at the head of TryEnablingIon in
> ion/AsmJS.cpp?

I doubt it has something to do with FFI, but since it is hard to debug these problems, I'll test. I'm currently running a build to answer that question in 2h-3h.

Comment 4

[Douglas Crosher [:dougc]]

(In reply to Luke Wagner [:luke] from comment #1)
> The graphical corruption sounds like what Douglas was reporting earlier
> (can't find the bug maybe that was in an email thread?).
> 
> Vlad: are you able to compile a build locally and, if so, does the problem
> repro if you put "return true" at the head of TryEnablingIon in 
> ion/AsmJS.cpp?

I can reproduce this on a plain FF nightly on a Nexus 7.  A debug build with TryEnablingIon giving up on the fast path works without the corruption.

Comment 5

[Hannes Verschore [:h4writer]]

I can confirm this is a FFI issue. Disabling the fast path fixes the issue. Tested on a Samsung Galaxy SIII. My own build on the chromebook can't show the canvas, making it a bit harder to debug. But I'm on this.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Shell testcase

Simplified shell testcase used for testing

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Patch

When getting the values from the stack instead of from a register was faulty under arm. I checked and the interpreter path uses "ShadowStackSpace" for arm too. This fixes the issue on soft float.

The testcase still fails on hardfloat, but that is not related to the ffi fastpath. It is also present when disabling it totally. I'll create a bug report for it.

I started a try build and will test it on my brothers phone, to see if the gears are now rendered correctly.
https://tbpl.mozilla.org/?tree=Try&rev=e9b4449320f9

Comment 8

[Hannes Verschore [:h4writer]]

Confirmed. Gears aren't corrupted anymore with this patch.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 771338 [details] [diff] [review]
Patch

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

Flipping review, since luke is absent and this should get fixed as fast as possible. Also I want to uplift as soon as possible.

Comment 10

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 771338 [details] [diff] [review]
Patch

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

I feel like this patch should be unnecessary, so I'm going to look into it.  In the mean time, this should get landed.

Comment 11

[Jesse Ruderman]

I modified jsfunfuzz to generate FFI calls with more args (fuzzing rev 99522150a2f7).

Comment 12

[Douglas Crosher [:dougc]]

(In reply to Jesse Ruderman from comment #11)
> I modified jsfunfuzz to generate FFI calls with more args (fuzzing rev
> 99522150a2f7).

Could I suggest also adding support for generating FFI callee Ion functions that use a lot of local integer and floating point registers, enough to have used all the registers.  Also, for the benefit of the ARM, test that an asm.js out-of-bounds heap load of a floating point number still returns NaN to ensure that the NANReg has been restored.

Comment 13

[Hannes Verschore [:h4writer]]

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

(In reply to Jesse Ruderman from comment #11)
> I modified jsfunfuzz to generate FFI calls with more args (fuzzing rev
> 99522150a2f7).
Thanks. The tips from dougc would also help to catch bugs.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 771338 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 860838

User impact if declined: The wrong values are copied as arguments to ffi calls when providing to much arguments. So pretty serious. ARM only.

Testing completed (on m-c, etc.): m-c:2 days + tested a browser build on softfloat locally and confirmed fixed.

Risk to taking this patch (and alternatives if risky): No risk, everything is better than the current code for ARM.

String or IDL/UUID changes made by this patch: /

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c2bb3f30e2df