cairo fails to compile on mingw with D2D and DWrite enabled

RESOLVED FIXED in mozilla13

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jacek Caban, Assigned: Jacek Caban)

Tracking

Trunk
mozilla13
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 544757 [details] [diff] [review]
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).
Attachment #544757 - Flags: review?(bas.schouten)
(Assignee)

Comment 1

7 years ago
Created attachment 545371 [details] [diff] [review]
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.
Attachment #544757 - Attachment is obsolete: true
Attachment #545371 - Flags: review?(bas.schouten)
Attachment #544757 - Flags: review?(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.
(Assignee)

Comment 3

7 years ago
(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.
(Assignee)

Comment 4

7 years ago
Created attachment 545659 [details] [diff] [review]
fix v1.2
Attachment #545371 - Attachment is obsolete: true
Attachment #545659 - Flags: review?(bas.schouten)
Attachment #545371 - Flags: review?(bas.schouten)
(Assignee)

Comment 5

7 years ago
Bas, ping
(Assignee)

Updated

7 years ago
Attachment #545659 - Flags: review?(bas.schouten) → review?(jones.chris.g)
(Assignee)

Comment 6

7 years ago
Chris, can you please review it. That's currently the only bug preventing clean m-c from compiling on mingw left.
Comment on attachment 545659 [details] [diff] [review]
fix v1.2

Bas, I don't like the pattern that's developing here ... ;)
Attachment #545659 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 8

7 years ago
Thanks for the review, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a21668ea9597
Whiteboard: [inbound]
(Assignee)

Comment 9

7 years ago
Backed out due to crashes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/116b2987471a
Whiteboard: [inbound]
(Assignee)

Comment 10

7 years ago
Pushed non-problematic parts to m-i:

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

I still need to investigate strange RefPtr problems.
Whiteboard: [inbound],[don't close after merge]
Whiteboard: [inbound],[don't close after merge]
(Assignee)

Comment 12

7 years ago
Created attachment 595430 [details] [diff] [review]
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
Attachment #595430 - Flags: review?(jones.chris.g)
Comment on attachment 595430 [details] [diff] [review]
RefPtr fix

This is Bas's turf.
Attachment #595430 - Flags: review?(jones.chris.g) → review?(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.
Attachment #595430 - Flags: review?(bas.schouten) → review+
(Assignee)

Comment 15

7 years ago
Thanks, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d279c6473c9
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/1d279c6473c9
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.