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

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jacek, Assigned: jacek)

Tracking

unspecified
mozilla22
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.34 KB, patch
gw280
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Created attachment 722769 [details] [diff] [review]
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.
Attachment #722769 - Flags: review?(gwright)
Does this actually work? When I tried doing pretty much the exact same thing, I got failures on try...
(Assignee)

Comment 2

6 years ago
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
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?
(ie - just export all public APIs as upstream intended)
(Assignee)

Comment 5

6 years ago
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.
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.
I'm OK with this.
Comment on attachment 722769 [details] [diff] [review]
fix

Let's go with exposing all public API functions, as per bug 787225.
Attachment #722769 - Flags: review?(gwright) → review-
Got a test patch working its way through try right now.

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

Comment 10

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

Comment 11

6 years ago
Created attachment 723471 [details] [diff] [review]
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.
Attachment #722769 - Attachment is obsolete: true
Attachment #723471 - Flags: review?(gwright)
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.
Attachment #723471 - Flags: review?(gwright) → review+
(Assignee)

Comment 14

6 years ago
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
https://hg.mozilla.org/mozilla-central/rev/612506f83f28
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.