670096 - cairo fails to compile on mingw with D2D and DWrite enabled

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jacek Caban]

Attachment: fix v1.0

There are a few problems:

- d2d_clip is used for both variable and type name. It's not allowed in GCC. I've renamed tpe name to d2d_clip_t.

- Use of __in/__out macros that are not supported on GCC due to conflict with GCC-provided headers

- Some variables are declared between goto statement and its label. Some declaration moving was required.

- GCC couldn't cope with assignment, where assignee is child class of stored type. Some explicit casts to base classes are needed (|newSurf->surface = static_cast<ID3D10Resource*>(texture);| for example).

Comment 1

[Jacek Caban]

Attachment: fix v1.1

The attached patch is the same as previous one, except it removes duplicated cairo_device_t typedef in cairo-win32.h that is already present in cairo.h. It caused troubles on GCC 4.5.

Comment 2

[Bas Schouten (:bas.schouten)]

(In reply to comment #0)
> Created attachment 544757 [details] [diff] [review] [review]
> fix v1.0
> - GCC couldn't cope with assignment, where assignee is child class of stored
> type. Some explicit casts to base classes are needed (|newSurf->surface =
> static_cast<ID3D10Resource*>(texture);| for example).


What's responsible for GCC on MinGW being so broken? It makes me a little bit concerned that we'd need to insert static_casts for such a big bug (maybe it fails to find the right template functions). Could we maybe just make all of this not be compiled when a MinGW build is used? Just make D2D/DWrite unavailable in those builds.

Comment 3

[Jacek Caban]

(In reply to comment #2)
> (In reply to comment #0)
> > Created attachment 544757 [details] [diff] [review] [review] [review]
> > fix v1.0
> > - GCC couldn't cope with assignment, where assignee is child class of stored
> > type. Some explicit casts to base classes are needed (|newSurf->surface =
> > static_cast<ID3D10Resource*>(texture);| for example).
> 
> 
> What's responsible for GCC on MinGW being so broken? It makes me a little
> bit concerned that we'd need to insert static_casts for such a big bug
> (maybe it fails to find the right template functions).

GCC in regards to handling such things is no different than GCC on any other platform. I've investigated the problem deeper and here is a short code explaining the problem:

struct class1 {};
struct class2 : public class1 {};

template<class T>
class RefPtr
{
public:
    RefPtr() {}

    RefPtr<T> &operator =(const RefPtr<T> aPtr) {}
    RefPtr<T> &operator =(T* aPtr) {}


    operator T*() {}

    template <typename U>
    operator RefPtr<U>() {}
};

static void test() {
    RefPtr<class2> a;
    RefPtr<class1> b;

    b = a;
}

$ g++ refptr.cpp -c 
refptr.cpp: In function 'void test()':
refptr.cpp:24:9: error: ambiguous overload for 'operator=' in 'b = a'
refptr.cpp:10:16: note: candidates are: RefPtr<T>& RefPtr<T>::operator=(RefPtr<T>) [with T = class1]
refptr.cpp:11:16: note:                 RefPtr<T>& RefPtr<T>::operator=(T*) [with T = class1]

(this is on Linux GCC, but mingw version gives exactly the same error). Note that in |b = a;| assignment, a conversion is required, but compiler can't decide how which option it should choose:

1)  convert RefPtr<class2> -> class2* (using RefPtr's cast operator)
    call assignment operator (variant with class1* argument)

2)  convert RefPtr<class2> -> RefPtr<class1> (using RefPtr's cast operator)
    call assignment operator (variant with RefPtr<class1> variant)

IMO the behavior of GCC is right.

The right fix is changing RefPtr's implementation and templating assignment operator with RefPtr argument like mftb/RefPtr.h does. This way it is best match and second conversion happens in its body. I will attach a patch.

> Could we maybe just
> make all of this not be compiled when a MinGW build is used? Just make
> D2D/DWrite unavailable in those builds.

That's what I did until recently (by adding --with-windows-version=... to mozconfig), but it looks like mingw builds are only such config, so it was broken every now and then. In long run we probably will drop completely support for such config, so it doesn't sound like the right way. Also, with my fix for RefPtr, all required changes to support mingw look like a clean up, no drawbacks so far.

Comment 4

[Jacek Caban]

Attachment: fix v1.2

Comment 5

[Jacek Caban]

Bas, ping

Comment 6

[Jacek Caban]

Chris, can you please review it. That's currently the only bug preventing clean m-c from compiling on mingw left.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 545659 [details] [diff] [review]
fix v1.2

Bas, I don't like the pattern that's developing here ... ;)

Comment 8

[Jacek Caban]

Thanks for the review, pushed to m-i:

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

Comment 9

[Jacek Caban]

Backed out due to crashes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/116b2987471a

Comment 10

[Jacek Caban]

Pushed non-problematic parts to m-i:

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

I still need to investigate strange RefPtr problems.

Comment 11

[Matt Brubeck (:mbrubeck)]

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

Comment 12

[Jacek Caban]

Attachment: RefPtr fix

I've replaced custom RefPtr implementation with well-tested and working with GCC version (see comment #3) from mfbt/RefPtr.h. I've copied its implementation and changed it to not depend on other Mozilla-specific stuff so it's suitable for cairo. I've kept cairo version's extension in form of overloaded operator&, because byRef solution doesn't work well with D2D COM interfaces.

This passes on try:
https://tbpl.mozilla.org/?tree=Try&rev=74fdd52fb079

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 595430 [details] [diff] [review]
RefPtr fix

This is Bas's turf.

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 595430 [details] [diff] [review]
RefPtr fix

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

If this passes all tests and try, it doesn't make me very happy, but I can live with it :), especially since we're not going to do much on the cairo backend anymore, and it probably makes your life better.

Comment 15

[Jacek Caban]

Thanks, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d279c6473c9

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1d279c6473c9