clang-cl tries to instantiate all of the methods on template dllexport classes #20537
Comments
|
If this is a relatively straightforward fix, I'd like to help fix it if someone can point me in the right direction. This bug hits us when building skia as a DLL, and it's preventing the last significant chunk of the tree with clang-cl. Thanks! |
|
This is a pretty wild deviation from the standard, can the source code in skia be fixed instead? |
The issue is in SkTDArray. This class has four members https://skia.googlesource.com/skia/+/master/include/core/SkTDArray.h#292 deleteAll, freeAll, unrefAll and safeUnrefAll which iterate over the contents of the array and delete it in various ways. I can't think of a way to fix this without breaking the interface contract of the class by basically removing those four functions. But a workaround would be to not dllexport this class in the first place. I've submitted a patch upstream for that: https://codereview.chromium.org/368253002/ |
Why do they even put SK_API on a template class? We should probably just ask them to remove it. I wonder why we haven't hit this in Chrome already. That SK_API usage dates back to the initial Windows DLL support in 2011: It's probably unnecessary. |
Unclear to me. As you note, looking at the history of that code, SK_API seems to have been added to that class alongside some others, so perhaps that's not intentional.
Yes, that is what my patch does.
In our builds, we link Skia in a library separate from the rest of our graphics code, so we need to define SKIA_DLL when building this code which makes SK_API map to __declspec(dllexport), so we end up exporting this class. But I think Chrome links this code into the mail DLL so there is no need to export the template, and then everything will work out fine as it should based on C++ (i.e., those methods do not even get instantiated.)
Yep, I'll get back to you once I've heard back on my patch in Skia, hopefully someone will review it soon. |
According to Richard, the Skia code is totally valid. We should fix this in Clang. |
Hmm, my patch is in the process of getting upstreamed... Is that not sufficient? |
Your skia patch is fine, because it makes no sense to have dllexport on a primary class template. I think we should also fix this in clang, because the skia code as it was written is valid C++. dllexport shouldn't break that. |
Agreed.
Do you mean something along the lines of instantiating the template members and only export them if the instantiation succeeds? This sounds good to me. |
|
Another example of this: https://codereview.chromium.org/485323002/
I think we just shouldn't instantiate class template members eagerly like we do now. I've started looking. |
|
Should be fixed in r216145. |
Extended Description
We hit this when building Skia as a DLL with clang-cl. FWIW I think the semantics implemented by clang-cl is much saner, but I guess MSVC only exports the symbols that are used in the translation unit and doesn't try to instantiate all of the other members.
$ cat test.cpp
template struct __declspec(dllexport) Foo {
void f() { T::bar(); }
};
void x() {
Foo a;
}
$ cl -c test.cpp
$ clang-cl -c test.cpp
test.cpp(2,14) : error: type 'int' cannot be used prior to '::' because it has no members
void f() { T::bar(); }
^
test.cpp(1,49) : note: in instantiation of member function 'Foo::f' requested here
template struct __declspec(dllexport) Foo {
^
1 error generated.
The text was updated successfully, but these errors were encountered: