1320644 - Fix Skia compilation on mingw after update to m55 branch

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Georg Koppen]

After bug 1299435 landed I get the following compiler error with mingw-w64:

/home/firefox/win52/toolchain/mingw-w64/bin/i686-w64-mingw32-g++ -std=gnu++11 -mwindows -o SkiaGLGlue.o -c -I/home/firefox/win52/mozilla-central/obj-mingw/dist/stl_wrappers  -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/home/firefox/win52/mozilla-central/gfx/gl -I/home/firefox/win52/mozilla-central/obj-mingw/gfx/gl -I/home/firefox/win52/mozilla-central/obj-mingw/ipc/ipdl/_ipdlheaders -I/home/firefox/win52/mozilla-central/ipc/chromium/src -I/home/firefox/win52/mozilla-central/ipc/glue -I/home/firefox/win52/mozilla-central/gfx/skia -I/home/firefox/win52/mozilla-central/gfx/skia/skia/include/config -I/home/firefox/win52/mozilla-central/gfx/skia/skia/include/core -I/home/firefox/win52/mozilla-central/gfx/skia/skia/include/gpu -I/home/firefox/win52/mozilla-central/gfx/skia/skia/include/utils -I/home/firefox/win52/mozilla-central/obj-mingw/dist/include  -I/home/firefox/win52/mozilla-central/obj-mingw/dist/include/nspr -I/home/firefox/win52/mozilla-central/obj-mingw/dist/include/nss         -DMOZILLA_CLIENT -include /home/firefox/win52/mozilla-central/obj-mingw/mozilla-config.h -MD -MP -MF .deps/SkiaGLGlue.o.pp  -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++14-compat -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-format -fno-exceptions -fno-strict-aliasing -mms-bitfields -mstackrealign -fno-keep-inline-dllexport -fno-rtti -fno-exceptions -fno-math-errno -pipe  -g -O -fomit-frame-pointer  -I/home/firefox/win52/mozilla-central/obj-mingw/dist/include/cairo -Wno-error=shadow  /home/firefox/win52/mozilla-central/gfx/gl/SkiaGLGlue.cpp
In file included from /home/firefox/win52/mozilla-central/gfx/skia/skia/include/gpu/gl/GrGLInterface.h:12:0,
                 from /home/firefox/win52/mozilla-central/gfx/gl/SkiaGLGlue.cpp:7:
/home/firefox/win52/mozilla-central/gfx/skia/skia/include/gpu/gl/GrGLExtensions.h:45:64: error: could not convert ‘nullptr’ from ‘std::nullptr_t’ to ‘GrGLFunction<const char* (__attribute__((__stdcall__)) *)(void*, int)> {aka std::function<const char*(void*, int)>}’
               GrGLFunction<GrEGLQueryStringProc> queryString = nullptr,
                                                                ^
In file included from /home/firefox/win52/mozilla-central/gfx/gl/SkiaGLGlue.cpp:7:0:
/home/firefox/win52/mozilla-central/gfx/skia/skia/include/gpu/gl/GrGLInterface.h:109:45: error: field ‘fActiveTexture’ has incomplete type ‘GrGLFunction<void (__attribute__((__stdcall__)) *)(unsigned int)> {aka std::function<void(unsigned int)>}’
         GrGLFunction<GrGLActiveTextureProc> fActiveTexture;
                                             ^

etc.

Comment 1

[Lee Salzman [:lsalzman]]

What version of g++ is this?

Comment 2

[Georg Koppen]

5.1.0. I am happy to test a mingw-w64 based on any other GCC 5.x version if that could help.

Comment 3

[Lee Salzman [:lsalzman]]

Hmm, somewhat perplexed on this one. std::function should have a constructor that accepts nullptr, and this works on VC, and GCC setups on other platforms, taken individually. Not entirely sure why it is barfing with mingw, without being able to test a mingw build setup myself... Nathan, thoughts?

Comment 4

[Georg Koppen]

Actually, Jacek might know what is going on as he fixed a lot of mingw related bugs in the past.

Comment 5

[Nathan Froyd [:froydnj]]

I don't have any ideas offhand, though I'm curious what the incomplete type errors are about: g++ knows enough to tell you that the type is a typedef for another type!  Maybe GCC is doing something wrong--or libstdc++ isn't correctly handling __stdcall__ function types somewhere--which would explain why this only shows up on mingw64, since most other platforms generally only have a single ABI for function pointers.

Comment 6

[Lee Salzman [:lsalzman]]

If you the following define inside gfx/skia/skia/include/config/SkUserConfig.h:

#define GR_GL_FUNCTION_TYPE

Does the problem go away?

Comment 7

[Georg Koppen]

(In reply to Lee Salzman [:lsalzman] from comment #6)
> If you the following define inside
> gfx/skia/skia/include/config/SkUserConfig.h:
> 
> #define GR_GL_FUNCTION_TYPE
> 
> Does the problem go away?

It does indeed, thanks!

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: don't use __stdcall with SkiaGLGlue

In Skia versions previous to m55, GR_GL_FUNCTION_TYPE was not defined. With the m55 branch, they defined this as __stdcall for interop with OpenGL function pointers from libGL. However, since the actual function we are supplying in SkiaGLGlue are lambda wrappers around trampolines in GLContext, we neither need nor want __stdcall where this would have actually differed from the default, since the lambdas would have used the default rather than __stdcall.

And, of course, this seems to work around MinGW's issue with std::function<__stdcall ..> and nullptr.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a53a6b7aa1
don't use __stdcall with SkiaGLGlue. r=mchang

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/25a53a6b7aa1

Comment 11

[Georg Koppen]

Thanks Lee! Would you be amenable to ask for uplifting of this small patch to Aurora? Firefox 52 will be the next ESR and it would be good if we would have a patch less to backport (and other folks might want to build with mingw-w64 as well). If so, could you file the resepctive request? Thanks.

Comment 12

[Lee Salzman [:lsalzman]]

Comment on attachment 8815849 [details] [diff] [review]
don't use __stdcall with SkiaGLGlue

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1299435
[User impact if declined]: Compilation not working on tier 3 platform (MinGW). Would like to have this fixed for 52 ESR.
[Is this code covered by automated tests?]: No, MinGW is tier 3.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: aurora
[Is the change risky?]: No
[Why is the change risky/not risky?]: This patch increases the correctness of code by disabling a buggy change Skia upstream made.
[String changes made/needed]: None

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8815849 [details] [diff] [review]
don't use __stdcall with SkiaGLGlue

fix mingw build issue in aurora52

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d59e82481c86