787653 - Allow compiling ANGLE on mingw-w64

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jacek Caban]

Attachment: Updated ANGLE to r1267

I've fixed it upstream. The attached patch updates ANGLE to recent upstream revision. This patch together with makefile patch that I will attach later is green on try:

https://tbpl.mozilla.org/?tree=Try&rev=ac68abe037aa

Comment 1

[Jacek Caban]

Attachment: makefile part

This patch changes build config in following ways:

- Allows DirectX SDK to not be present when cross compiling
mingw-w64 (optionally) ships DirectX headers and import libs together with regular platform SDK, so nothing special is required for compilation. DirectX SDK is used in Mozilla also to ship required DLLs, but a build without those DLLs is still valid (and will work if they are present in runtime, which is usually the case on Windows AFAIK).

- Allows ANGLE source to use exceptions

- Modifies .def file for 32-bit GCC build. This is ugly, but it has to be done conditionally, so doing it in Makefile seems to be the least invasive way. It doesn't affect MSVC builds, so regular Windows builds are not affected and the whole hack is just a few lines of code.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 657555 [details] [diff] [review]
makefile part

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

::: gfx/angle/src/libGLESv2/Makefile.in
@@ +170,5 @@
> +
> +moz_libGLESv2.def: libGLESv2.def
> +	cp $< $@
> +	echo "glGetProcAddress@4=glGetProcAddress" >> $@
> +	echo "glBindTexImage@4=glBindTexImage" >> $@

Ugh. This is kind of horrible. We're trying to reduce the number of custom rules in Makefiles. Is there any other way to do this?

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Ted Mielczarek [:ted] from comment #2)
> Comment on attachment 657555 [details] [diff] [review]
> makefile part
> 
> Review of attachment 657555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/angle/src/libGLESv2/Makefile.in
> @@ +170,5 @@
> > +
> > +moz_libGLESv2.def: libGLESv2.def
> > +	cp $< $@
> > +	echo "glGetProcAddress@4=glGetProcAddress" >> $@
> > +	echo "glBindTexImage@4=glBindTexImage" >> $@
> 
> Ugh. This is kind of horrible. We're trying to reduce the number of custom
> rules in Makefiles. Is there any other way to do this?

Why not patch the .def file directly? We can have our own local ANGLE patches.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 657552 [details] [diff] [review]
Updated ANGLE to r1267

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

Is this patch exactly the diff from r1242 to r1267? If yes, we would take it without review, but we also need to rebase all the local patches (gfx/angle/*.patch).

Comment 5

[Jacek Caban]

(In reply to Benoit Jacob [:bjacob] from comment #3)
> (In reply to Ted Mielczarek [:ted] from comment #2)
> > Comment on attachment 657555 [details] [diff] [review]
> > makefile part
> > 
> > Review of attachment 657555 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/angle/src/libGLESv2/Makefile.in
> > @@ +170,5 @@
> > > +
> > > +moz_libGLESv2.def: libGLESv2.def
> > > +	cp $< $@
> > > +	echo "glGetProcAddress@4=glGetProcAddress" >> $@
> > > +	echo "glBindTexImage@4=glBindTexImage" >> $@
> > 
> > Ugh. This is kind of horrible. We're trying to reduce the number of custom
> > rules in Makefiles. Is there any other way to do this?
> 
> Why not patch the .def file directly? We can have our own local ANGLE
> patches.

I wanted it to be conditional. It doesn't make sense for win64 and for MSVC it's not needed. Makefile.in seemed like a place where hackish things are done occasionally in the rest of codebase and I wrote the patch before I saw the discussion about clean up :) Changing the def file to be preprocessed like .def file for gkmedias.dll would be an option, but I wanted it to be less invasive. I will revisit the patch and try to find a better solution.

Comment 6

[Jacek Caban]

(In reply to Benoit Jacob [:bjacob] from comment #4)

> Is this patch exactly the diff from r1242 to r1267? If yes, we would take it
> without review, but we also need to rebase all the local patches
> (gfx/angle/*.patch).

Yes, it's just a diff applied to the tree (plus updated README.mozilla and added the new file to Makefile.in). I've verified that local patches still apply.

Comment 7

[Jacek Caban]

Pushed ANGLE update part to m-i:

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

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

OK thanks. Next time please say explicitly 'no review' rather than 'r=nonexistentperson' in the commit msg; but that's fine.

Regarding the .def file: is there any real downside to applying the tweak unconditionally?

If you have to be conditional, you could have two .def files and have the makefile select one with a #ifdef. That's still quite clean as it could easily be translated into any other build-system-representation format.

Comment 9

[Ed Morley [:emorley]]

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

Comment 10

[Jacek Caban]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> OK thanks. Next time please say explicitly 'no review' rather than
> 'r=nonexistentperson' in the commit msg; but that's fine.

OK, sorry about that.

> Regarding the .def file: is there any real downside to applying the tweak
> unconditionally?

It causes both forms to be exported on MSVC, where it's not needed. On win64, stdcall doesn't exist and no name decorations are used, so it will look strange there. But... it's just a minor detail with no real runtime impact, so if you're fine with it, I will attach a patch.

Comment 11

[Jacek Caban]

Attachment: makefile part v2

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 659641 [details] [diff] [review]
makefile part v2

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

Need explanation:

::: gfx/angle/Makefile.in
@@ +123,5 @@
>  # libEGL depends on (links against!) libGLESv2!
>  DIRS = src/libGLESv2 src/libEGL
>  
>  libs::
> +ifdef MOZ_D3DX9_CAB

Can you explain why this ifdef? It seems orthogonal to the choice of compiler, MSVC vs MinGw. What does depend on the compiler is building the ANGLE renderer, as given by ifdef MOZ_ANGLE_RENDERER, but we are inside such a ifdef block here.

Comment 13

[Jacek Caban]

(In reply to Benoit Jacob [:bjacob] from comment #12)
> Comment on attachment 659641 [details] [diff] [review]
> makefile part v2
> 
> Review of attachment 659641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Need explanation:
> 
> ::: gfx/angle/Makefile.in
> @@ +123,5 @@
> >  # libEGL depends on (links against!) libGLESv2!
> >  DIRS = src/libGLESv2 src/libEGL
> >  
> >  libs::
> > +ifdef MOZ_D3DX9_CAB
> 
> Can you explain why this ifdef? It seems orthogonal to the choice of
> compiler, MSVC vs MinGw. What does depend on the compiler is building the
> ANGLE renderer, as given by ifdef MOZ_ANGLE_RENDERER, but we are inside such
> a ifdef block here.

Yes, they are compiler-independent, but mingw allows also cross compiling from other OSes (Linux in my case), where it's impossible to install MS DirectX SDK (except in Wine). That's why I changed configure.in to not try to locate them (it would fail soon, because it uses Windows-only reg.exe) and attempt to copy DLLs in Makefile only if they were found by configure. Note that when compiling with mingw on Windows, those will be required just like for MSVC. I think that allowing cross builds without including MS redistributable DLLs is the right solution - after all the builds are still valid and working as long as you provide them in runtime.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 659641 [details] [diff] [review]
makefile part v2

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

OK, got it about MOZ_ANGLE_RENDERER. You approach seems fine.

::: gfx/angle/src/libEGL/Makefile.in
@@ +67,5 @@
>  RCFILE = $(srcdir)/libEGL.rc
>  
>  include $(topsrcdir)/config/rules.mk
>  
> +OS_LIBS += $(call EXPAND_LIBNAME,dwmapi)

I don't understand enough about Makefile to be comfortable here: will this correctly preserve the /delayload below, at least in the NOT GNU_CC case?

::: gfx/angle/src/libGLESv2/Makefile.in
@@ +169,5 @@
> +
> +TextureSSE2.$(OBJ_SUFFIX): CXXFLAGS+=-msse2
> +
> +OS_CXXFLAGS := $(filter-out -fno-exceptions,$(OS_CXXFLAGS)) -fexceptions
> +OS_LIBS += -ld3d9 -ld3dx9 -ld3dcompiler_43

Don't hardcode the number 43 here. Instead, use the MOZ_D3DX9_VERSION variable that is defined by configure.in. It has a AC_SUBST so it should be accessible from the makefile.

Also note that you need it for -ld3dx9 as well, not just for -ld3dcompiler.

r- for that.

::: gfx/angle/src/libGLESv2/libGLESv2.def
@@ +183,5 @@
> +
> +    ; GCC has problems with linking to undecored stdcall functions,
> +    ; so we explicitly add aliases for APIs used by EGL
> +    glGetProcAddress@4=glGetProcAddress
> +    glBindTexImage@4=glBindTexImage

That seems fine to do unconditionally, since that affects only libGLESv2.dll which is only used when WebGL is used, and so typically doesn't affect startup time.

Comment 15

[Jacek Caban]

(In reply to Benoit Jacob [:bjacob] from comment #14)
> Comment on attachment 659641 [details] [diff] [review]
> makefile part v2
> 
> Review of attachment 659641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, got it about MOZ_ANGLE_RENDERER. You approach seems fine.
> 
> ::: gfx/angle/src/libEGL/Makefile.in
> @@ +67,5 @@
> >  RCFILE = $(srcdir)/libEGL.rc
> >  
> >  include $(topsrcdir)/config/rules.mk
> >  
> > +OS_LIBS += $(call EXPAND_LIBNAME,dwmapi)
> 
> I don't understand enough about Makefile to be comfortable here: will this
> correctly preserve the /delayload below, at least in the NOT GNU_CC case?

Yes, it will work this way. That's how delayload is done in toolkit/library and I wanted to share between MSVC and GCC as much as possible here.

> ::: gfx/angle/src/libGLESv2/Makefile.in
> @@ +169,5 @@
> > +
> > +TextureSSE2.$(OBJ_SUFFIX): CXXFLAGS+=-msse2
> > +
> > +OS_CXXFLAGS := $(filter-out -fno-exceptions,$(OS_CXXFLAGS)) -fexceptions
> > +OS_LIBS += -ld3d9 -ld3dx9 -ld3dcompiler_43
> 
> Don't hardcode the number 43 here. Instead, use the MOZ_D3DX9_VERSION
> variable that is defined by configure.in. It has a AC_SUBST so it should be
> accessible from the makefile.
> 
> Also note that you need it for -ld3dx9 as well, not just for -ld3dcompiler.
> 
> r- for that.

Oh, that was my temporary solution. I should have cleaned that up before submitting the patch, sorry about that. I will attach another version that uses version prefix if DirectX SDK is found to ensure that we compile for the same version as shipped redistributable and version-independent names otherwise (which would be the case for cross compilation).

Comment 16

[Jacek Caban]

Attachment: makefile part v2.1

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 660359 [details] [diff] [review]
makefile part v2.1

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

Just a question then: does the OS_LIBS stuff also work with MSVC? If yes, why have this inside of #if GNU ? It would be nice to avoid having 2 separate paths in these makefiles, if that's possible.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 660359 [details] [diff] [review]
makefile part v2.1

I suppose that you still want a review from Ted here. I know ANGLE specifics but am a Makefile n00b.

Comment 19

[Jacek Caban]

Thanks for the review!

(In reply to Benoit Jacob [:bjacob] from comment #17)
> Just a question then: does the OS_LIBS stuff also work with MSVC? If yes,
> why have this inside of #if GNU ? It would be nice to avoid having 2
> separate paths in these makefiles, if that's possible.

Yeah, it would be nicer to have just one path. Unfortunately AFAIK MSVC doesn't have anything like -L/lib/path option, so we need to provide the whole path to its .lib files.

Comment 20

[Jacek Caban]

Thanks for reviews, landed on m-i:

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

Comment 21

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/6eed25d2f2c9