Closed
Bug 1453892
Opened 4 years ago
Closed 4 years ago
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’
Categories
(Core :: Graphics, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla62
People
(Reporter: cosmo0920, Assigned: lsalzman)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file)
2.10 KB,
patch
|
rhunt
:
review+
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
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.
Comment 1•4 years ago
|
||
This will be fixed by upstream's skia (https://skia-review.googlesource.com/c/skia/+/112744). Using clang is workaround.
Comment 2•4 years ago
|
||
Makoto, this type of bugs should be under Core: Graphics? Thanks
Flags: needinfo?(m_kato)
Comment 3•4 years ago
|
||
(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•4 years ago
|
||
(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•4 years ago
|
||
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•4 years ago
|
||
Does adding "-mfp16-format=ieee" to the compiler flags resolve this?
Flags: needinfo?(cosmo0920.oucc)
Reporter | ||
Comment 7•4 years ago
|
||
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•4 years ago
|
||
(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•4 years ago
|
||
(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•4 years ago
|
||
GCC 6 has this option. Not sure why GCC 7 wouldn't? At least, the documentation for GCC 7 still lists it.
Reporter | ||
Comment 11•4 years ago
|
||
(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•4 years ago
|
||
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•4 years ago
|
||
(In reply to Charles Robertson from comment #12) > I'm getting this same failure building Firefox 60 ESR on aarch64. Bug 1461041
Reporter | ||
Comment 14•4 years ago
|
||
(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.
Comment 15•4 years ago
|
||
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.
Reporter | ||
Comment 16•4 years ago
|
||
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.
Reporter | ||
Comment 17•4 years ago
|
||
(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•4 years ago
|
||
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.
Updated•4 years ago
|
Attachment #8976218 -
Flags: review?(rhunt) → review+
Comment 19•4 years ago
|
||
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
Reporter | ||
Comment 20•4 years ago
|
||
(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•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/00fcd7b81c88
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee | ||
Comment 22•4 years ago
|
||
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•4 years ago
|
status-firefox60:
--- → wontfix
status-firefox61:
--- → affected
status-firefox-esr60:
--- → affected
OS: Unspecified → All
Hardware: Unspecified → ARM64
Comment 23•4 years ago
|
||
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+
Comment 24•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b6653a9f24fc
Comment 26•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/b9217b454fd5
Comment 27•4 years ago
|
||
(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.
Description
•