Odin ARM issue with many-argument FFI call

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vlad, Assigned: h4writer)

Tracking

(Blocks 1 bug)

unspecified
mozilla25
ARM
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed)

Details

(Whiteboard: [games:p1])

Attachments

(2 attachments)

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).
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 repro on a plain FF Nightly build on a Samsung Galaxy SII, btw.
Assignee

Comment 3

6 years ago
(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.
(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.
Assignee

Comment 5

6 years ago
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.
Assignee: general → hv1989
Assignee

Comment 6

6 years ago
Simplified shell testcase used for testing
Assignee

Comment 7

6 years ago
Posted patch PatchSplinter Review
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
Attachment #771338 - Flags: review?(luke)
Assignee

Comment 8

6 years ago
Confirmed. Gears aren't corrupted anymore with this patch.
Assignee

Comment 9

6 years ago
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.
Attachment #771338 - Flags: review?(luke) → review?(mrosenberg)
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.
Attachment #771338 - Flags: review?(mrosenberg) → review+

Comment 11

6 years ago
I modified jsfunfuzz to generate FFI calls with more args (fuzzing rev 99522150a2f7).
(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.
Assignee

Comment 13

6 years ago
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.
Assignee

Updated

6 years ago
Blocks: 860838
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/a25f41748ec2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

6 years ago
Summary: Odin ARM issue with simple WebGL testcase → Odin ARM issue with many-argument FFI call
Assignee

Comment 15

6 years ago
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: /
Attachment #771338 - Flags: approval-mozilla-aurora?
Attachment #771338 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.