1234494 - Skia update in 1082598 broke OpenBSD builds

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Landry Breuil (:gaston)]

/home/othersrc/mozilla-central/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:60:5: error: unknown type name 'locale_t'
    locale_t fOldLocale;
    ^
/home/othersrc/mozilla-central/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:61:5: error: unknown type name 'locale_t'
    locale_t fLocale;

we dont have locale.h nor locale_t. This same issue has been worked around in our chromium port using this patch: http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/www/chromium/patches/patch-third_party_skia_src_gpu_GrAutoLocaleSetter_h?rev=1.2&content-type=text/x-cvsweb-markup

Comment 1

[Lee Salzman [:lsalzman]]

Upstream bug report: https://bugs.chromium.org/p/skia/issues/detail?id=4736

Comment 2

[Landry Breuil (:gaston)]

Attachment: Add OpenBSD to the ifdef for locale_t

Comment 3

[Landry Breuil (:gaston)]

Fwiw, this issue also affects NetBSD:

30:41.92 In file included from /home/landry/m-c/gfx/skia/skia/src/gpu/gl/builders/GrGLProgramBuilder.cpp:10:0:
30:41.92 /home/landry/m-c/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h: In constructor 'GrAutoLocaleSetter::GrAutoLocaleSetter(const char*)':
30:41.92 /home/landry/m-c/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:34:43: error: 'uselocale' was not declared in this scope
30:41.92              fOldLocale = uselocale(fLocale);
30:41.92                                            ^
30:41.92 /home/landry/m-c/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h: In destructor 'GrAutoLocaleSetter::~GrAutoLocaleSetter()':
30:41.93 /home/landry/m-c/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:49:34: error: 'uselocale' was not declared in this scope
30:41.93               uselocale(fOldLocale);

$ uname -a
NetBSD foxfire.netbsd.de 7.0_RC3 NetBSD 7.0_RC3 (XEN3_DOMU) #4: Mon Aug 17 00:11:08 UTC 2015  [e-mail]:/home/netbsd/7/amd64/obj/sys/arch/amd64/compile/XEN3_DOMU amd64

Comment 4

[Landry Breuil (:gaston)]

Well that's not exactly the same issue, but netbsd's locale.h doesnt provide a uselocale() function.

Comment 5

[Martin Husemann]

NetBSD currently has no plans to add per-thread locale support, as that topic is highly controversial.

Comment 6

[Martin Husemann]

Attachment: Potential build-fixing hack for NetBSD

Here is a patch that should fix the build on NetBSD - this is not a good solution, but should be ok for now. Will start another discussion about the underlying issue on the NetBSD mailing lists.

Comment 7

[Landry Breuil (:gaston)]

Still something missing i'm afraid....

 0:18.07 /home/landry/m-c/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h: In constructor 'GrAutoLocaleSetter::GrAutoLocaleSetter(const char*)':
 0:18.08 /home/landry/m-c/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:32:51: error: cannot convert 'locale_t {aka _locale*}' to 'const char*
' for argument '2' to 'char* setlocale(int, const char*)'
 0:18.08          fOldLocale = setlocale(LC_NUMERIC, fLocale);
 0:18.08                                                    ^
 0:18.08 /home/landry/m-c/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h: In destructor 'GrAutoLocaleSetter::~GrAutoLocaleSetter()':
 0:18.08 /home/landry/m-c/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:50:41: error: cannot convert 'locale_t {aka _locale*}' to 'const char*
' for argument '2' to 'char* setlocale(int, const char*)'
 0:18.08          setlocale(LC_NUMERIC, fOldLocale);

Comment 8

[Martin Husemann]

Attachment: Potential build-fixing hack for NetBSD v2

Comment 9

[Landry Breuil (:gaston)]

Comment on attachment 8702254 [details] [diff] [review]
Potential build-fixing hack for NetBSD v2

Yeah, this version builds here.

Comment 10

[Lee Salzman [:lsalzman]]

There are problems associated with both of these proposed patches.

Skia is generating GLSL shaders. It is trying to temporarily set the locale to "C" to ensure it gets numbers that are correctly formatted for the GLSL compiler. Ideally we would want that to be per-thread because we don't want drawing on a Skia draw target to override the locale of the entire process everywhere else. Just temporarily setting the locale for the entire process (at an undefined time during a draw call) will cause Undefined Behavior (tm).

On the other hand, this is for only GPU accelerated Skia, which we don't enable by default except on Android or Mac. So my proposed less invasive fix for this right now would just be not to compile in GPU acceleration at all on those NetBSD or OpenBSD, since it isn't required for the Skia backend to even work. As it is, we are still using Cairo entirely for both content and canvas drawing on those platforms, so this shouldn't materially impact those platforms to not compile it in. If the GPU code that is causing this whole mess is thus never compiled in in the first place, no need to use invasive workarounds for it.

Thoughts? Okay with that idea in principle?

Comment 11

[Martin Husemann]

Fine with me. Skia needs quite a bit more portability work (e.g. GPU acceleration does not work on big endian machines at all). Someday...

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: don't build in Skia GPU code if support is disabled

This patch doesn't do anything too tricky despite the size. It merely looks for any GPU-related Skia files, and omits them from the build if GPU support is disabled at configure time.

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: disable Skia GPU support by default on certain *BSDs

In combination with the previous patch to not build in GPU support at all if it is disabled, this should make sure we work around the build problems on NetBSD and OpenBSD.

Comment 14

[Landry Breuil (:gaston)]

(In reply to Lee Salzman [:lsalzman] from comment #10)
> There are problems associated with both of these proposed patches.
> 
> Skia is generating GLSL shaders. It is trying to temporarily set the locale
> to "C" to ensure it gets numbers that are correctly formatted for the GLSL
> compiler. Ideally we would want that to be per-thread because we don't want
> drawing on a Skia draw target to override the locale of the entire process
> everywhere else. Just temporarily setting the locale for the entire process
> (at an undefined time during a draw call) will cause Undefined Behavior (tm).

Ok, got it - this is definitely horrible.

> On the other hand, this is for only GPU accelerated Skia, which we don't
> enable by default except on Android or Mac. So my proposed less invasive fix
> for this right now would just be not to compile in GPU acceleration at all
> on those NetBSD or OpenBSD, since it isn't required for the Skia backend to
> even work. As it is, we are still using Cairo entirely for both content and
> canvas drawing on those platforms, so this shouldn't materially impact those
> platforms to not compile it in. If the GPU code that is causing this whole
> mess is thus never compiled in in the first place, no need to use invasive
> workarounds for it.
> 
> Thoughts? Okay with that idea in principle?

Okay in principle, as long as this is revisited when Skia GPU is enabled on linux - with which we try to stay on par feature-wise...

Fwiw, i'm trying builds without patches on NetBSD & OpenBSD using --disable-skia-gpu which should be equivalent to your patch in att #8702370 - as for att #8702370, i had to dive in gyp files quite some times for webrtc, and i always ended up running away screaming.

Is --disable-skia-gpu a 'tested' build configuration ?

Thanks for working on this!

Comment 15

[Martin Husemann]

Random side question: does anyone understand why Skia is not using C++ locale support to avoid the issue?

Comment 16

[Landry Breuil (:gaston)]

Fwiw, i finally had to test the two patches altogether, since only --disable-skia-gpu wouldnt avoid building the offending files.

The NetBSD build is okay in skia but fails when linking test binaries, missing rpath to X lib or not linking with libXt:
../../toolkit/library/libxul.so: undefined reference to `applicationShellWidgetClass'
../../toolkit/library/libxul.so: undefined reference to `XtAppProcessEvent'
.. maybe the exact same issue as bug #1225753

And the OpenBSD build is okay too in skia (even if it still randomly fails in ICU).

Comment 17

[Lee Salzman [:lsalzman]]

(In reply to Landry Breuil (:gaston) from comment #16)
> Fwiw, i finally had to test the two patches altogether, since only
> --disable-skia-gpu wouldnt avoid building the offending files.
> 
> The NetBSD build is okay in skia but fails when linking test binaries,
> missing rpath to X lib or not linking with libXt:
> ../../toolkit/library/libxul.so: undefined reference to
> `applicationShellWidgetClass'
> ../../toolkit/library/libxul.so: undefined reference to `XtAppProcessEvent'
> .. maybe the exact same issue as bug #1225753
> 
> And the OpenBSD build is okay too in skia (even if it still randomly fails
> in ICU).

These would be best filed as a separate bug report, since they don't pertain to Skia. This way we can focus on just fixing up Skia in this bug.

Comment 18

[Landry Breuil (:gaston)]

Sure, i was just saying that the issue we had with skia itself is fixed by your diff.. (but as usual we have *other* build issues :)

Comment 19

[Lee Salzman [:lsalzman]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=881b7d65413d

Comment 20

[Lee Salzman [:lsalzman]]

Attachment: don't build in Skia GPU code if support is disabled

Refreshed to fix bit rot about GrResourceCache.cpp

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc2cbf77516
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa20905baa9

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6dc2cbf77516
https://hg.mozilla.org/mozilla-central/rev/6fa20905baa9