skia build error on aarch64, with 61.0a: SkJumper_stages.cpp:670:12: error: ‘vcvt_f32_f16’ SkJumper_stages.cpp:690:12: error: ‘vcvt_f16_f32’

RESOLVED FIXED in Firefox -esr60

Status

()

defect
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: cosmo0920, Assigned: lsalzman)

Tracking

Trunk
mozilla62
ARM64
All
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr60 fixed, firefox60 wontfix, firefox61 fixed, firefox62 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

I'm building Firefox 61.0a on an aarch64 machine (Jetson TX2) with the following configurations.

.mozconfig:
---
ac_add_options --disable-webrtc
ac_add_options --disable-av1
export CC=gcc-7
export CXX=g++-7
---

ARM neon extension is enabled.
---
$ gcc-7 -dM -E - < /dev/null  | grep -i neon
#define __ARM_NEON 1
---

Firefox source tree version:
---
$ hg tip
changeset:   413059:da809ecceaf3
tag:         tip
parent:      413044:46615d425bcb
parent:      413058:91cfe8e76dd1
user:        Gurzau Raul <rgurzau@mozilla.com>
date:        Fri Apr 13 02:08:51 2018 +0300
summary:     Merge inbound to mozilla-central.  a=merge
---


Actual results:

I'm getting compile error.

---
 2:15.68     Blocking waiting for file lock on build directory
 2:17.29 /home/nvidia/mozilla-central/gfx/skia/skia/src/jumper/SkJumper_stages.cpp: In function ‘F from_half(U16)’:
 2:17.29 /home/nvidia/mozilla-central/gfx/skia/skia/src/jumper/SkJumper_stages.cpp:670:12: error: ‘vcvt_f32_f16’ was not declared in this scope
 2:17.29      return vcvt_f32_f16(h);
 2:17.29             ^~~~~~~~~~~~
 2:17.33 /home/nvidia/mozilla-central/gfx/skia/skia/src/jumper/SkJumper_stages.cpp: In function ‘U16 to_half(F)’:
 2:17.33 /home/nvidia/mozilla-central/gfx/skia/skia/src/jumper/SkJumper_stages.cpp:690:12: error: ‘vcvt_f16_f32’ was not declared in this scope
 2:17.33      return vcvt_f16_f32(f);
 2:17.33             ^~~~~~~~~~~~
 2:18.91 /home/nvidia/mozilla-central/config/rules.mk:1024: recipe for target 'SkJumper_stages.o' failed
 2:18.91 make[4]: *** [SkJumper_stages.o] Error 1
 2:18.91 /home/nvidia/mozilla-central/config/recurse.mk:73: recipe for target 'gfx/skia/target' failed
 2:18.91 make[3]: *** [gfx/skia/target] Error 2
 2:18.91 make[3]: *** Waiting for unfinished jobs....
---


Expected results:

Build succeeded with above .mozconfig.
This will be fixed by upstream's skia (https://skia-review.googlesource.com/c/skia/+/112744).  Using clang is workaround.
Makoto, this type of bugs should be under Core: Graphics? Thanks
Flags: needinfo?(m_kato)
(In reply to ovidiu boca[:Ovidiu] from comment #2)
> Makoto, this type of bugs should be under Core: Graphics? Thanks

Yes
Component: Untriaged → Graphics
Flags: needinfo?(m_kato)
Product: Firefox → Core
Reporter

Comment 4

Last year
(In reply to Makoto Kato [:m_kato] from comment #1)
> This will be fixed by upstream's skia
> (https://skia-review.googlesource.com/c/skia/+/112744).  Using clang is
> workaround.

I've confirmed that using clang 3.9 can build m-c on rev 413059.
But I want to build with gcc 7.x for aarch64 machine.
Because of Yocto Rocko (2.4) has gcc 7.x, but does not clang by default.
This would require us to upgrade to milestone 68. We recently completed the update to 66 in bug 1444506.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee

Comment 6

Last year
Does adding "-mfp16-format=ieee" to the compiler flags resolve this?
Flags: needinfo?(cosmo0920.oucc)
Reporter

Comment 7

Last year
Adding -mfp16-format=ieee into CFLAGS and CXXFLAGS on mozconfig cannot resolve this issue.

---
ac_add_options --disable-webrtc
ac_add_options --disable-av1
export CC=gcc-7
export CXX=g++-7
mk_add_options CFLAGS="-mfp16-format=ieee $CFLAGS"
mk_add_options CXXFLAGS="-mfp16-format=ieee $CXXFLAGS"
---

Where should I add -mfp16-format=ieee?
Flags: needinfo?(cosmo0920.oucc) → needinfo?(lsalzman)
Assignee

Comment 8

Last year
(In reply to Hiroshi Hatake [:cosmo0920] from comment #7)
> Adding -mfp16-format=ieee into CFLAGS and CXXFLAGS on mozconfig cannot
> resolve this issue.
> 
> ---
> ac_add_options --disable-webrtc
> ac_add_options --disable-av1
> export CC=gcc-7
> export CXX=g++-7
> mk_add_options CFLAGS="-mfp16-format=ieee $CFLAGS"
> mk_add_options CXXFLAGS="-mfp16-format=ieee $CXXFLAGS"
> ---
> 
> Where should I add -mfp16-format=ieee?

Theoretically it should be CFLAGS or CXXFLAGS. But you could try something like CXX="g++-7 -mfp16-format=ieee" I guess.
Flags: needinfo?(lsalzman)
Reporter

Comment 9

Last year
(In reply to Lee Salzman [:lsalzman] from comment #8)
> (In reply to Hiroshi Hatake [:cosmo0920] from comment #7)
> > Adding -mfp16-format=ieee into CFLAGS and CXXFLAGS on mozconfig cannot
> > resolve this issue.
> > 
> > ---
> > ac_add_options --disable-webrtc
> > ac_add_options --disable-av1
> > export CC=gcc-7
> > export CXX=g++-7
> > mk_add_options CFLAGS="-mfp16-format=ieee $CFLAGS"
> > mk_add_options CXXFLAGS="-mfp16-format=ieee $CXXFLAGS"
> > ---
> > 
> > Where should I add -mfp16-format=ieee?
> 
> Theoretically it should be CFLAGS or CXXFLAGS. But you could try something
> like CXX="g++-7 -mfp16-format=ieee" I guess.

CXX="g++-7 -mfp-format=ieee" does not work, though....

---
 0:03.97 DEBUG: Executing: `/usr/bin/gcc-7 -mfp16-format=ieee -E /tmp/conftest.iUzVep.c`
 0:03.97 DEBUG: The command returned non-zero exit status 1.
 0:03.97 DEBUG: Its error output was:
 0:03.97 DEBUG: | gcc-7: error: unrecognized command line option ‘-mfp16-format=ieee’
 0:03.97 ERROR: Command `/usr/bin/gcc-7 -mfp16-format=ieee -E /tmp/conftest.iUzVep.c` failed with exit status 1.
---
Assignee

Comment 10

Last year
GCC 6 has this option. Not sure why GCC 7 wouldn't? At least, the documentation for GCC 7 still lists it.
(In reply to Lee Salzman [:lsalzman] from comment #10)
> GCC 6 has this option. Not sure why GCC 7 wouldn't? At least, the
> documentation for GCC 7 still lists it.

I'm not sure what gcc-7 says 'unrecognized option' on Tegra X2.

But gcc-7 on Tegra X2 seems to support fp16 by default:
---
$ gcc-7 -dM -E - < /dev/null  | grep -i FP16
#define __ARM_FP16_FORMAT_IEEE 1
#define __ARM_FP16_ARGS 1
---
I'm getting this same failure building Firefox 60 ESR on aarch64. I am also using gcc-7. Any suggestions? Hiroshi: did you get yours to compile?
(In reply to Charles Robertson from comment #12)
> I'm getting this same failure building Firefox 60 ESR on aarch64.

Bug 1461041
(In reply to Charles Robertson from comment #12)
> I'm getting this same failure building Firefox 60 ESR on aarch64. I am also
> using gcc-7. Any suggestions? Hiroshi: did you get yours to compile?

No, I couldn't get to compile.
I still got failure on rev 418039.

```
changeset:   418039:87cc17943894
tag:         tip
parent:      418021:782b1ae19860
parent:      418038:c209a79a7043
user:        Ciure Andrei <aciure@mozilla.com>
date:        Mon May 14 00:55:43 2018 +0300
summary:     Merge inbound to mozilla-central. a=merge
```

Also same errors are generated on comment #0.
I came across this patch in researching this build issue:

https://github.com/OSSystems/meta-browser/blob/master/recipes-browser/chromium/files/aarch64-skia-build-fix.patch

I thought I would try it and behold, Firefox 60 ESR did build successfully for Aarch64. However, when I ran this newly built Firefox on my Aarch64 machine it crashed. 

See Bug 1461041

I cannot confirm if the crash is due to this patch, but it seems unlikely.
(In reply to Hiroshi Hatake [:cosmo0920] from comment #16)
> I've also found Skia patch for aarch64 build on Ubuntu package.
> 
> https://launchpad.net/ubuntu/+source/firefox/60.0+build2-0ubuntu1
> 
> *
> https://launchpad.net/ubuntu/+archive/primary/+sourcefiles/firefox/60.
> 0+build2-0ubuntu1/firefox_60.0+build2-0ubuntu1.debian.tar.xz
> 
> They use another macro to fix this.

I've confirmed to work firefox on aarch64 with the above Ubuntu patch.
Assignee

Comment 18

Last year
It seems like the safer choice here is to only use the vcvt_f32_f16 when clang and ARM NEON support are detected, like in the Ubuntu patch. So let's just go with that.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8976218 - Flags: review?(rhunt)
Attachment #8976218 - Flags: review?(rhunt) → review+

Comment 19

Last year
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00fcd7b81c88
only use SkJumper's arm64 half-float optimizations with clang. r=rhunt
(In reply to Lee Salzman [:lsalzman] from comment #18)
> Created attachment 8976218 [details] [diff] [review]
> only use SkJumper's arm64 half-float optimizations with clang
> 
> It seems like the safer choice here is to only use the vcvt_f32_f16 when
> clang and ARM NEON support are detected, like in the Ubuntu patch. So let's
> just go with that.

Lee, can we uplift your patch into 60esr?
aarch64 isn't tier 1, but some Linux distributions support aarch64.

Comment 21

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/00fcd7b81c88
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee

Comment 22

Last year
Comment on attachment 8976218 [details] [diff] [review]
only use SkJumper's arm64 half-float optimizations with clang

Approval Request Comment
[Feature/Bug causing the regression]: bug 1444506
[User impact if declined]: Firefox will not build on arm64 due to a compile error in Skia.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No risk at all.
[Why is the change risky/not risky?]: Just fixes a compile error on a tier-3 platform in a codepath that we don't even actually run at all.
[String changes made/needed]: none
Attachment #8976218 - Flags: approval-mozilla-esr60?
Attachment #8976218 - Flags: approval-mozilla-beta?
Assignee

Updated

Last year
OS: Unspecified → All
Hardware: Unspecified → ARM64
Comment on attachment 8976218 [details] [diff] [review]
only use SkJumper's arm64 half-float optimizations with clang

Fix for build bustage on Tier 3 platforms. Approved for 61.0b7 and ESR 60.1.
Attachment #8976218 - Flags: approval-mozilla-esr60?
Attachment #8976218 - Flags: approval-mozilla-esr60+
Attachment #8976218 - Flags: approval-mozilla-beta?
Attachment #8976218 - Flags: approval-mozilla-beta+
Duplicate of this bug: 1462868
(In reply to Lee Salzman [:lsalzman] from comment #22)
> [Is this code covered by automated tests?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no 

Marking as qe-verify- due to the automated coverage for this issue and based on Lee's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.