Skia update in 1082598 broke OpenBSD builds

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: gaston, Assigned: lsalzman)

Tracking

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 4 obsolete attachments)

/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
Blocks: 1082598
Blocks: 1233966
No longer blocks: 1233966
Attachment #8701946 - Flags: review?(lsalzman)
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  spz@franklin.NetBSD.org:/home/netbsd/7/amd64/obj/sys/arch/amd64/compile/XEN3_DOMU amd64
Well that's not exactly the same issue, but netbsd's locale.h doesnt provide a uselocale() function.
NetBSD currently has no plans to add per-thread locale support, as that topic is highly controversial.
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.
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);
Attachment #8702253 - Attachment is obsolete: true
Comment on attachment 8702254 [details] [diff] [review]
Potential build-fixing hack for NetBSD v2

Yeah, this version builds here.
Attachment #8702254 - Flags: feedback+
Whiteboard: [gfx-noted]
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?
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...
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.
Assignee: nobody → lsalzman
Attachment #8701946 - Attachment is obsolete: true
Attachment #8702254 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8701946 - Flags: review?(lsalzman)
Attachment #8702362 - Flags: review?(jmuizelaar)
Attachment #8702362 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8702370 - Flags: review?(ted)
(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!
Random side question: does anyone understand why Skia is not using C++ locale support to avoid the issue?
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).
(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.
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 :)
Attachment #8702370 - Flags: review?(ted) → review?(mh+mozilla)
Attachment #8702370 - Flags: review?(mh+mozilla) → review+
Refreshed to fix bit rot about GrResourceCache.cpp
Attachment #8702362 - Attachment is obsolete: true
Attachment #8702921 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6dc2cbf77516
https://hg.mozilla.org/mozilla-central/rev/6fa20905baa9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: 1447962
You need to log in before you can comment on or make changes to this bug.