901208 - Skia fails to build on debian armel since bug 777614

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mike Hommey [:glandium]]

Skia r5539 brought some armv6+ instructions without counterpart for older arm. There's also a #if SK_ARM_ARCH == 6 that should really be a <= 6.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Fix Skia for ARM v4t

Note the #ifdef for pld is technically wrong, because it's supported on ARMv5TE, but i didn't want to go through hoops to do this right, considering apart from me, probably nobody cares, and I'm targetting ARMv4T anyways.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Fix Skia for ARM v4t

With a typo fix.

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 790567 [details] [diff] [review]
Fix Skia for ARM v4t

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

r=me, but see one question below.

::: gfx/skia/patches/0026-Bug-901208-Fix-ARM-v4t.patch
@@ +9,5 @@
> +                   "blo     2f                           \n\t"
> +                   "and     r4, r3, #0x0000f8            \n\t"
> +                   "and     r5, r3, #0x00fc00            \n\t"
> +                   "and     r6, r3, #0xf80000            \n\t"
> ++#if SK_ARM_ARCH >= 6

This is safe, so I have no objection, but isn't this what SK_ARM_HAS_EDSP is for?

Comment 4

[Mike Hommey [:glandium]]

(In reply to Timothy B. Terriberry (:derf) from comment #3)
> This is safe, so I have no objection, but isn't this what SK_ARM_HAS_EDSP is
> for?

Indeed, thanks.

Comment 5

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb8a5f5724e

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0bb8a5f5724e

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 790567 [details] [diff] [review]
Fix Skia for ARM v4t

[Approval Request Comment]
I'd like this to land for the next ESR to avoid carrying the patch around for a year.
Bug caused by last skia upstream import
User impact if declined: Failure to build on the debian armel port
Testing completed (on m-c, etc.): Tested on Firefox 23 and 24 on debian.
Risk to taking this patch (and alternatives if risky): NPoTB
String or IDL/UUID changes made by this patch: None

Comment 8

[bhavana bajaj [:bajaj]]

Comment on attachment 790567 [details] [diff] [review]
Fix Skia for ARM v4t

Please make sure that any Builds that may be impacted by this patch are generated once the patch lands on Beta.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4ed497d794b
https://hg.mozilla.org/releases/mozilla-beta/rev/ad1f0ae291d7

Comment 10

[u279076]

Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.

Comment 11

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

This has been upstreamed: https://codereview.chromium.org/337853003/

Comment 12

[Mike Hommey [:glandium]]

Attachment: Fix Skia for ARM v4t, refreshed against current trunk

This is the same patch as what landed in Fx26, but was left out in bug 910754.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8466837 [details] [diff] [review]
Fix Skia for ARM v4t, refreshed against current trunk

Carrying over r+

Comment 14

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a641e3d9dd

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/747541380bbb
https://hg.mozilla.org/releases/mozilla-beta/rev/aa124023125a
https://hg.mozilla.org/releases/mozilla-esr31/rev/216554269ce2

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1a641e3d9dd