Fix Skia compilation on mingw after update to m55 branch

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gk, Assigned: lsalzman)

Tracking

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

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.
What version of g++ is this?
Flags: needinfo?(gk)
5.1.0. I am happy to test a mingw-w64 based on any other GCC 5.x version if that could help.
Flags: needinfo?(gk)
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?
Flags: needinfo?(nfroyd)
Actually, Jacek might know what is going on as he fixed a lot of mingw related bugs in the past.
Flags: needinfo?(jacek)
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.
Flags: needinfo?(nfroyd)
If you the following define inside gfx/skia/skia/include/config/SkUserConfig.h:

#define GR_GL_FUNCTION_TYPE

Does the problem go away?
Flags: needinfo?(gk)
Whiteboard: [gfx-noted]
(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!
Flags: needinfo?(gk)
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.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8815849 - Flags: review?(mchang)
Attachment #8815849 - Flags: review?(mchang) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a53a6b7aa1
don't use __stdcall with SkiaGLGlue. r=mchang
https://hg.mozilla.org/mozilla-central/rev/25a53a6b7aa1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Flags: needinfo?(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
Flags: needinfo?(lsalzman)
Flags: needinfo?(jacek)
Attachment #8815849 - Flags: approval-mozilla-aurora?
Comment on attachment 8815849 [details] [diff] [review]
don't use __stdcall with SkiaGLGlue

fix mingw build issue in aurora52
Attachment #8815849 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.