1453892 - 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’

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Hiroshi Hatake [:cosmo0920]]

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 <[e-mail]>
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.

Comment 1

[Makoto Kato [:m_kato]]

This will be fixed by upstream's skia (https://skia-review.googlesource.com/c/skia/+/112744).  Using clang is workaround.

Comment 2

[ovidiu boca[:Ovidiu]]

Makoto, this type of bugs should be under Core: Graphics? Thanks

Comment 3

[Makoto Kato [:m_kato]]

(In reply to ovidiu boca[:Ovidiu] from comment #2)
> Makoto, this type of bugs should be under Core: Graphics? Thanks

Yes

Comment 4

[Hiroshi Hatake [:cosmo0920]]

(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.

Comment 5

[Andrew Osmond [:aosmond]]

This would require us to upgrade to milestone 68. We recently completed the update to 66 in bug 1444506.

Comment 6

[Lee Salzman [:lsalzman]]

Does adding "-mfp16-format=ieee" to the compiler flags resolve this?

Comment 7

[Hiroshi Hatake [:cosmo0920]]

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?

Comment 8

[Lee Salzman [:lsalzman]]

(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.

Comment 9

[Hiroshi Hatake [:cosmo0920]]

(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.
---

Comment 10

[Lee Salzman [:lsalzman]]

GCC 6 has this option. Not sure why GCC 7 wouldn't? At least, the documentation for GCC 7 still lists it.

Comment 11

[Hiroshi Hatake [:cosmo0920]]

(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
---

Comment 12

[Charles Robertson]

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?

Comment 13

[Gingerbread Man]

(In reply to Charles Robertson from comment #12)
> I'm getting this same failure building Firefox 60 ESR on aarch64.

Bug 1461041

Comment 14

[Hiroshi Hatake [:cosmo0920]]

(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 <[e-mail]>
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.

Comment 15

[Charles Robertson]

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.

Comment 16

[Hiroshi Hatake [:cosmo0920]]

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.

Comment 17

[Hiroshi Hatake [:cosmo0920]]

(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.

Comment 18

[Lee Salzman [:lsalzman]]

Attachment: 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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00fcd7b81c88
only use SkJumper's arm64 half-float optimizations with clang. r=rhunt

Comment 20

[Hiroshi Hatake [:cosmo0920]]

(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

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/00fcd7b81c88

Comment 22

[Lee Salzman [:lsalzman]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b6653a9f24fc

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b9217b454fd5

Comment 27

[Cornel Ionce [:cornel_ionce], Desktop Release QA]

(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.