849207 - Use __declspec(dllexport) for exporting skia C++ symbols.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jacek Caban]

Attachment: fix

It's never a good idea to export C++ by .def file, because it requires hardcoding mangles names (it already broke win64 after landing). GCC (g++) mangles names differently, so this also breaks mingw builds. Using __declspec(dllexport/dllimport) seems like a nicer solution in this case. The attached patch implements that. I decided to use SKIA_IMPLEMENTATION/GR_IMPLEMENTATION for that purpose, since that's what skia uses in normal (as in not m-c import) builds. Also I didn't include a patch file for gfx/skia/patches, I'd prefer to add that after review.

Comment 1

[George Wright (:gw280) (needinfo me!)]

Does this actually work? When I tried doing pretty much the exact same thing, I got failures on try...

Comment 2

[Jacek Caban]

My first attempt failed on try, here is new one, with a minor change (it still runs):

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

Comment 3

[George Wright (:gw280) (needinfo me!)]

As an aside, I'm not sure I'm happy with putting changes into Skia that aren't upstreamable. What would be the downside of just building with SKIA_DLL everywhere, and SKIA_IMPLEMENTATION when building gfx/skia, like in bug 787225?

Comment 4

[George Wright (:gw280) (needinfo me!)]

(ie - just export all public APIs as upstream intended)

Comment 5

[Jacek Caban]

I think that should work. The downside would be exporting much more symbols from gkmedias.dll than we need. I'm not sure how much we should care (I assumed we do, that's why I did the patch the way it is). If you prefer the other solution, I will change the patch.

Comment 6

[George Wright (:gw280) (needinfo me!)]

My policy for the Skia tree is to try to minimise the number of patches applied against upstream that aren't upstreamable. I would prefer exporting all symbols in that respect, but my mind can be swayed if there's a significant impact in performance.

Comment 7

[Joe Drew (not getting mail)]

I'm OK with this.

Comment 8

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 722769 [details] [diff] [review]
fix

Let's go with exposing all public API functions, as per bug 787225.

Comment 9

[George Wright (:gw280) (needinfo me!)]

Got a test patch working its way through try right now.

https://tbpl.mozilla.org/?showall=1&tree=Try&rev=14e5574bea78

Comment 10

[Jacek Caban]

Thanks for the patch. It failed, but it just needs a few more changes. Here is my attempt to address them:

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

It changes two thing.

- It adds SKIA_IMPLEMENTATION and GR_IMPLEMENTATION to gfx/2d. It ends up in the same dll as skia and as such it should use the same declarations (AFAIR mingw is more picky about that than MSVC).

- It adds SK_API to SkString so that the class is exported. It seems strange that upstream doesn't do that. I will try to see if there is a reason later. If there is not, that part should be upstreamable.

Comment 11

[Jacek Caban]

Attachment: fix v2

Previous try commit succeeded. I took another look at SkString and it came out that the only use case is a helper in GLContextSkia.cpp (as in no API call strictly requires SkString). This is easy to replace by plain C string operations, avoiding the need to patch upstream skia code.

Comment 12

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 723471 [details] [diff] [review]
fix v2

SkString is actually SK_API declared upstream now, so the changes to GLContextSkia are unnecessary. However, I'm also fine with this change.

Comment 13

[George Wright (:gw280) (needinfo me!)]

I am a little concerned about the orange on https://tbpl.mozilla.org/?tree=Try&rev=eff0c11af06f&showall=1

Comment 14

[Jacek Caban]

Thanks for the review. This orange is also already present on m-c, so it's unrelated.

https://hg.mozilla.org/integration/mozilla-inbound/rev/612506f83f28

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/612506f83f28