If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Build failure for Linux/aarch64 after updating Skia to m59

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
ARM
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

4 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

4 months ago
mozreview-review
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
Attachment #8867637 - Flags: review?(lsalzman) → review+
Blocks: 1334963

Comment 4

4 months ago
mozreview-review
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.
Attachment #8867637 - Flags: review-

Comment 5

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

https://reviewboard.mozilla.org/r/139222/#review143382
Attachment #8867636 - Flags: review?(mh+mozilla) → review+
(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.
(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.
(Assignee)

Comment 8

4 months ago
(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.

Updated

4 months ago
Attachment #8867637 - Flags: review-

Comment 9

4 months ago
Pushed by m_kato@ga2.so-net.ne.jp:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de97b072fede
https://hg.mozilla.org/mozilla-central/rev/bc320d3634ee
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.