1500490 - Crash in D2DBitmap::GetSizeInternal

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, defect, P5)

Description

[Tom Ritter [:tjr]]

Attachment: Sample Crash 1.txt

I've seen the build crash three times (out of maybe 10 runs; other crashes were Bug 1500477) in D2DBitmap::GetSizeInternal, called from here:

https://searchfox.org/mozilla-central/rev/9cb3e241502a2d47e2d5057ca771324a446b6695/gfx/2d/DrawTargetD2D1.cpp#442

I've determined that image->QueryInterface succeeds, and bitmap does point to something.  

There was the old GCC bug relating to GetSize that we fixed with DWIDL_EXPLICIT_AGGREGATE_RETURNS - but that shouldn't affect clang...

Build I used (x64 debug): https://treeherder.mozilla.org/#/jobs?repo=try&revision=613caaa9dd22d522290de6e19d49d4110da7a08c

Comment 1

[(Away)]

It looks like the compiler was expecting the result of `GetSize()`, a struct containing two floats, to come back in rax, with `width` in the lower 32 bits and `height` in the upper 32 bits.

But the implementation of GetSize() expects to be passed a hidden second parameter pointing to the desired location of the return-struct.

Comment 2

[(Away)]

...which means it's DWIDL_EXPLICIT_AGGREGATE_RETURNS after all?

Comment 3

[Jacek Caban]

Sorry for misleading information on this.

Martin, do you know anything about this? Is it intentional on clang side? I think it's considered a bug in GCC: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52792

If both GCC and clang suffer from this problem, maybe we should enable DWIDL_EXPLICIT_AGGREGATE_RETURNS by default in mingw-w64 headers?

Comment 4

[Martin Storsjö]

Sorry, no idea about this matter, I haven't touched those parts of clang to know where this behaviour is defined. As clang does support the MSVC ABI just fine in general, it sounds like it is linked to some other gcc-compatible/mingw feature when running in mingw mode - perhaps part of the itanium C++ ABI in general?

Does enabling DWIDL_EXPLICIT_AGGREGATE_RETURNS mean that callers of these methods, in mingw mode, need to adjust the calling code? At least that would make it clear to the caller that something has to be adjusted, instead of having to debug runtime crashes.

Comment 5

[Jacek Caban]

(In reply to Martin Storsjö from comment #4)
> Sorry, no idea about this matter, I haven't touched those parts of clang to
> know where this behaviour is defined. As clang does support the MSVC ABI
> just fine in general, it sounds like it is linked to some other
> gcc-compatible/mingw feature when running in mingw mode - perhaps part of
> the itanium C++ ABI in general?

Maybe. Now that I think of it, maybe it's needed for libstdc++ compatibility...

> Does enabling DWIDL_EXPLICIT_AGGREGATE_RETURNS mean that callers of these
> methods, in mingw mode, need to adjust the calling code? At least that would
> make it clear to the caller that something has to be adjusted, instead of
> having to debug runtime crashes.


It should be transparent to callers. Callers should work just fine without modification. Code that implements those methods (by inheriting troublesome interfaces) needs to be adjusted and attempting to build without modification will fail. As far as d2d1.h is considered, it's not common that someone would implement problematic interfaces. Eg. Firefox only does calls to interfaces implemented by system dlls.

Comment 6

[Tom Ritter [:tjr]]

I tested with https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a430539f8bed766f1bcd00a13b629fb89f72d1&selectedJob=207356727

Adding DWIDL_EXPLICIT_AGGREGATE_RETURNS has fixed the issue; I've had several successful start-ups without issue!

Comment 7

[Tom Ritter [:tjr]]

Attachment: Bug 1500490 - Add DWIDL_EXPLICIT_AGGREGATE_RETURNS to the mingw-clang CXXFLAGS

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 9019809 [details] [diff] [review]
Bug 1500490 - Add DWIDL_EXPLICIT_AGGREGATE_RETURNS to the mingw-clang CXXFLAGS

dmajor apparently knows something about what's going on here.

Comment 9

[Jacek Caban]

I just pushed a fix to mingw-w64:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/a91a7e405752bbdbe39101f7aaeb1e1d932199db/

With that fix, no changes should be needed on mozilla side, just mingw-w64 update.

Comment 10

[(Away)]

Comment on attachment 9019809 [details] [diff] [review]
Bug 1500490 - Add DWIDL_EXPLICIT_AGGREGATE_RETURNS to the mingw-clang CXXFLAGS

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

I'm assuming we'll update mingw-w64 instead of using this patch?