1142056 - Skia does not build on arm64 (aarch64)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

Skia does not build on arm64 (aarch64) although upstream Skia may have this support already.

Comment 1

[Marcin Juszkiewicz]

Attachment: Fix used for firefox 37/38b1

As Firefox failed to build in Fedora I took a look at upstream and made that fix based on their commits.

But proper solution would be update of Skia to latest version.

Comment 2

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8587220 [details] [diff] [review]
Fix used for firefox 37/38b1

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

r+ conditional upon confirmation of an upstream change, please ensure the upstream change is referenced in the commit message.

Comment 3

[Makoto Kato [:m_kato]]

This change is included in the following changeset.
https://skia.googlesource.com/skia/+/b79ff56de23fef680ae7187040f2d6a9516b553d

Comment 4

[Lee Salzman [:lsalzman]]

Now that the Skia update has landed, can you confirm if this has been resolved?

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Part 1. Add aarch64 configuration to skia's moz.build

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8700920 [details] [diff] [review]
Part 1. Add aarch64 configuration to skia's moz.build

When building for aarch64, some files are missing.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

I can confirm that when the fix hits stable release - Fedora does not build nightly on aarch64 and I don't have such HW handy. I'll watch this and post here.

Comment 8

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8700920 [details] [diff] [review]
Part 1. Add aarch64 configuration to skia's moz.build

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

This looks fine to me, providing it builds.

Comment 9

[Makoto Kato [:m_kato]]

Ah, I forget landing this.  So after rebasing this, I will handle this again.

Comment 10

[Makoto Kato [:m_kato]]

Attachment: Bug 1142056 - Part 1. Add aarch64 configuration on Skia's moz.build.

Sorry for reviewing again.  I rebase the previous fix to trunk.

Review commit: https://reviewboard.mozilla.org/r/73524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73524/

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Bug 1142056 - Part 2. Enable skia on aarch64 as default.

I tested this using Ubuntu 16.04/aarch64 on PINE64.

Review commit: https://reviewboard.mozilla.org/r/73526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73526/

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8783857 [details]
Bug 1142056 - Part 2. Enable skia on aarch64 as default.

https://reviewboard.mozilla.org/r/73526/#review71928

Comment 13

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8783856 [details]
Bug 1142056 - Part 1. Add aarch64 configuration on Skia's moz.build.

https://reviewboard.mozilla.org/r/73524/#review71476

::: gfx/skia/generate_mozbuild.py:435
(Diff revision 1)
>    write_sources(f, sources['intel'], 4)
>  
> -  f.write("elif CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC']:\n")
> +  f.write("elif CONFIG['CPU_ARCH'] in ('arm', 'aarch64') and CONFIG['GNU_CC']:\n")
>    write_sources(f, sources['arm'], 4)
>  
> -  f.write("    if CONFIG['BUILD_ARM_NEON']:\n")
> +  f.write("    if CONFIG['CPU_ARCH'] == 'aarch64':\n")

Why isn't BUILD_ARM_NEON defined on aarch64? This should probably be enabled here: http://searchfox.org/mozilla-central/source/build/autoconf/arch.m4#204

::: gfx/skia/generate_mozbuild.py:437
(Diff revision 1)
> -  f.write("elif CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC']:\n")
> +  f.write("elif CONFIG['CPU_ARCH'] in ('arm', 'aarch64') and CONFIG['GNU_CC']:\n")
>    write_sources(f, sources['arm'], 4)
>  
> -  f.write("    if CONFIG['BUILD_ARM_NEON']:\n")
> +  f.write("    if CONFIG['CPU_ARCH'] == 'aarch64':\n")
> +  write_sources(f, sources['neon'], 8)
> +  f.write("    elif CONFIG['BUILD_ARM_NEON']:\n")

It seems like we should ensure that BUILD_ARM_NEON is defined on aarch64?

It looks like we only check for NEON on "arm"; I suspect we need to add aarch64 to this:
http://searchfox.org/mozilla-central/source/build/autoconf/arch.m4#

glandium, what do you think?

::: gfx/skia/moz.build:531
(Diff revision 1)
>          'skia/src/opts/SkBlitMask_opts_arm.cpp',
>      ]
>      SOURCES += [
>          'skia/src/opts/SkBlitRow_opts_arm.cpp',
>      ]
> -    if CONFIG['BUILD_ARM_NEON']:
> +    if CONFIG['CPU_ARCH'] == 'aarch64':

Same issue here re. BUILD_ARM_NEON.

Comment 14

[George Wright (:gw280) (needinfo me!)]

Oops, I reviewed this a while back but forgot to hit publish. Anyway, ignore the duplicate review comment :)

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8783856 [details]
Bug 1142056 - Part 1. Add aarch64 configuration on Skia's moz.build.

https://reviewboard.mozilla.org/r/73524/#review71476

> Why isn't BUILD_ARM_NEON defined on aarch64? This should probably be enabled here: http://searchfox.org/mozilla-central/source/build/autoconf/arch.m4#204

We don't set BUILD_ARM_NEON on aarch64 now.  Some NEON code with BUILD_ARM_NEON use inline assembler, but assembler syntax of arm32 and arm64 isn't compatible.

Comment 16

[Makoto Kato [:m_kato]]

Although I ask glandium to add BUILD_ARM_NEON to aarch64, this is his comment.

From  bug 1303952 comment #10.

> Which leaves us with the NEON assembly case, and I think that should be handled separately than by setting BUILD_ARM_NEON
> and/or HAVE_ARM_NEON.

:gw280, could you reconsider review for Part 1 patch since there is a reason that we don't add BUILD_ARM_NEON to aarch64?

Comment 17

[Makoto Kato [:m_kato]]

part 2 is unnecessary by another changes (bug 1137305)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bab8ab8cd03
Add aarch64 configuration on Skia's moz.build. r=gw280

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5bab8ab8cd03