Closed Bug 1503733 Opened 5 years ago Closed 5 years ago

Skia m71 merge breaks all tier3 platforms

Categories

(Core :: Graphics, defect, P3)

65 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: awilfox, Assigned: awilfox)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

Linux/ppc64:

 7:22.61 In file included from /var/clean/mozppc/mozilla-unified/gfx/skia/skia/src/core/SkOpts.cpp:39:0:
 7:22.61 /var/clean/mozppc/mozilla-unified/gfx/skia/skia/src/opts/SkBitmapFilter_opts.h:895:21: error: missing binary operator before token "("
 7:22.61  #if SK_HAS_ATTRIBUTE(optimize) && defined(SK_RELEASE)
 7:22.62                      ^
 7:23.96 make[4]: *** [/var/clean/mozppc/mozilla-unified/config/rules.mk:1128: SkOpts.o] Error 1


This is because in SkBitmapFilter_opts.h, there is:

#if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2
..
#elif defined(SK_ARM_HAS_NEON)
..
#else
..
    // There's a bug somewhere here with GCC autovectorization (-ftree-vectorize).  We originally
    // thought this was 32 bit only, but subsequent tests show that some 64 bit gcc compiles
    // suffer here too.
    //
    // Dropping to -O2 disables -ftree-vectorize.  GCC 4.6 needs noinline.  https://bug.skia.org/2575
#if SK_HAS_ATTRIBUTE(optimize) && defined(SK_RELEASE)
        #define SK_MAYBE_DISABLE_VECTORIZATION __attribute__((optimize("O2"), noinline))
#else
        #define SK_MAYBE_DISABLE_VECTORIZATION
#endif

    SK_MAYBE_DISABLE_VECTORIZATION
    void convolve_horizontally(const unsigned char* srcData,
                               const SkConvolutionFilter1D& filter,
                               unsigned char* outRow,
                               bool hasAlpha) {
        if (hasAlpha) {
            ConvolveHorizontally<true>(srcData, filter, outRow);
        } else {
            ConvolveHorizontally<false>(srcData, filter, outRow);
        }
    }
#undef SK_MAYBE_DISABLE_VECTORIZATION
...
#endif


SK_HAS_ATTRIBUTE was removed from gfx/skia/skia/include/core/SkPreConfig.h when Bug 1502152 (Skia m71) landed.  Since this bug only seems to affect GCC 4(?) it may be possible to simple remove that preprocessor block entirely; I also don't believe many people try building Firefox beyond -O2, so that makes it even less risky.

I'm going to try a build on Linux/ppc64 with such a change and see how that goes.
Due to Bug 1503749 I haven't been able to ensure this is correct at runtime, but it does allow the build to succeed and I don't see why it wouldn't be correct.
Attachment #9021715 - Flags: review?(lsalzman)
Blocks: 1503747
Attachment #9021715 - Flags: review?(lsalzman) → review+
Assignee: nobody → AWilcox
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
OS: Linux → Unspecified
Hardware: Desktop → Unspecified
Whiteboard: [gfx-noted]
Blocks: 1502152
See Also: 1502152
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b8b0ba18f4
Allow compilation of Skia on non-x86/arm platforms. r=lsalzman
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9b8b0ba18f4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.