Closed Bug 670096 Opened 9 years ago Closed 8 years ago

cairo fails to compile on mingw with D2D and DWrite enabled

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jacek, Assigned: jacek)

Details

(Whiteboard: [inbound])

Attachments

(2 files, 2 obsolete files)

Attached patch fix v1.0 (obsolete) — Splinter Review
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)
Attached patch fix v1.1 (obsolete) — Splinter Review
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.
(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.
Attached patch fix v1.2Splinter Review
Attachment #545371 - Attachment is obsolete: true
Attachment #545659 - Flags: review?(bas.schouten)
Attachment #545371 - Flags: review?(bas.schouten)
Bas, ping
Attachment #545659 - Flags: review?(bas.schouten) → review?(jones.chris.g)
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+
Thanks for the review, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a21668ea9597
Whiteboard: [inbound]
Backed out due to crashes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/116b2987471a
Whiteboard: [inbound]
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]
Attached patch RefPtr fixSplinter Review
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+
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.