1364840 - Build failure for Linux/aarch64 after updating Skia to m59

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Makoto Kato [:m_kato]]

2 issues
- m59 requires SkOpts_crc32.cpp for aarch64 build, so we have to update python script to generate moz.build file
- Linux/arm and Linux/aarch64 might use sys/auxv.h to detect CPU feature.  So we have to add it to system-headers

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1364840 - Part 1. Add sys/auxv.h to system-headers.

gfx/skia/skia/src/core/SkCpu.cpp might use sys/auxv.h to detect CPU feature.  So we should add it to system-headers

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

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1364840 - Part 2. Generate aarch64 build rule to add CRC32 feature on aarch64.

SkOpts.cpp will call Init_crc32() on aarch64 build.  So we should add SkOpts_crc32.cpp to aarch64 sources.

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

Comment 3

[Lee Salzman [:lsalzman]]

Comment on attachment 8867637 [details]
Bug 1364840 - Part 2. Generate aarch64 build rule to add CRC32 feature on aarch64.

https://reviewboard.mozilla.org/r/139224/#review142560

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8867637 [details]
Bug 1364840 - Part 2. Generate aarch64 build rule to add CRC32 feature on aarch64.

https://reviewboard.mozilla.org/r/139224/#review143380

::: gfx/skia/moz.build:742
(Diff revision 1)
>      SOURCES['skia/src/opts/SkOpts_avx.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=51']
>      SOURCES['skia/src/opts/SkOpts_hsw.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=52']
>  elif CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC']:
>      CXXFLAGS += CONFIG['NEON_FLAGS']
> +elif CONFIG['CPU_ARCH'] == 'aarch64' and CONFIG['GNU_CC']:
> +    SOURCES['skia/src/opts/SkOpts_crc32.cpp'].flags += ['-march=armv8-a+crc']

Not all armv8's support the crc extensions.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8867636 [details]
Bug 1364840 - Part 1. Add sys/auxv.h to system-headers.

https://reviewboard.mozilla.org/r/139222/#review143382

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8867637 [details]
> Bug 1364840 - Part 2. Generate aarch64 build rule to add CRC32 feature on
> aarch64.
> 
> https://reviewboard.mozilla.org/r/139224/#review143380
> 
> ::: gfx/skia/moz.build:742
> (Diff revision 1)
> >      SOURCES['skia/src/opts/SkOpts_avx.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=51']
> >      SOURCES['skia/src/opts/SkOpts_hsw.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=52']
> >  elif CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC']:
> >      CXXFLAGS += CONFIG['NEON_FLAGS']
> > +elif CONFIG['CPU_ARCH'] == 'aarch64' and CONFIG['GNU_CC']:
> > +    SOURCES['skia/src/opts/SkOpts_crc32.cpp'].flags += ['-march=armv8-a+crc']
> 
> Not all armv8's support the crc extensions.

This should only let us compile SkOpts_crc32. Skia uses a runtime check to determine whether to actually use SkOpts_crc32 or not.

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Comment on attachment 8867637 [details]
> > Bug 1364840 - Part 2. Generate aarch64 build rule to add CRC32 feature on
> > aarch64.
> > 
> > https://reviewboard.mozilla.org/r/139224/#review143380
> > 
> > ::: gfx/skia/moz.build:742
> > (Diff revision 1)
> > >      SOURCES['skia/src/opts/SkOpts_avx.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=51']
> > >      SOURCES['skia/src/opts/SkOpts_hsw.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=52']
> > >  elif CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC']:
> > >      CXXFLAGS += CONFIG['NEON_FLAGS']
> > > +elif CONFIG['CPU_ARCH'] == 'aarch64' and CONFIG['GNU_CC']:
> > > +    SOURCES['skia/src/opts/SkOpts_crc32.cpp'].flags += ['-march=armv8-a+crc']
> > 
> > Not all armv8's support the crc extensions.
> 
> This should only let us compile SkOpts_crc32. Skia uses a runtime check to
> determine whether to actually use SkOpts_crc32 or not.

Correct, SkOpts_crc32 is compiled with the feature, but entry into that code is guarded by a runtime check for the CRC32 feature. That's basically how all the SkOpts_foo files work.

Comment 8

[Makoto Kato [:m_kato]]

(In reply to Lee Salzman [:lsalzman] from comment #7)
> Correct, SkOpts_crc32 is compiled with the feature, but entry into that code
> is guarded by a runtime check for the CRC32 feature. That's basically how
> all the SkOpts_foo files work.

Yes, SkCpu.cpp added runtime check to detect new CRC32 feature.  So part 1 fix requires on m59.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de97b072fede
Part 1. Add sys/auxv.h to system-headers. r=glandium
https://hg.mozilla.org/integration/autoland/rev/bc320d3634ee
Part 2. Generate aarch64 build rule to add CRC32 feature on aarch64. r=lsalzman

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de97b072fede
https://hg.mozilla.org/mozilla-central/rev/bc320d3634ee