This is an archive of the discontinued LLVM Phabricator instance.

[MinGW] Don't let template instantiation declarations cover nested classes
ClosedPublic

Authored by mstorsjo on Apr 26 2019, 12:47 AM.

Details

Summary

An explicit template instantiation declaration used to let callers assume both outer and nested classes instantiations were defined in a different translation unit.

If the instantiation is marked dllexport, only the outer class is exported, but the caller will try to reference the instantiation of both outer and inner classes.

This makes MinGW mode match both MSVC and Windows Itanium, by having instantations only cover the outer class, and locally emitting definitions of the nested classes. Windows Itanium was changed to use this behavious in SVN r300804.

This deviates from what GCC does, but should be safe (and only inflate the object file size a bit, but MSVC and Windows Itanium modes do the same), and fixes cases where inner classes aren't dllexported.

This fixes missing references in combination with dllexported/imported template intantiations.

GCC suffers from the same issue, reported at [1], but the issue is still unresolved there. The issue can probably be solved either by making dllexport cover all nested classes as well, or this way (matching MSVC).

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89087

Diff Detail

Repository
rC Clang

Event Timeline

mstorsjo created this revision.Apr 26 2019, 12:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2019, 12:47 AM
hans accepted this revision.Apr 26 2019, 2:00 AM

Seems okay to me.

This revision is now accepted and ready to land.Apr 26 2019, 2:00 AM
This revision was automatically updated to reflect the committed changes.
rnk added inline comments.Apr 29 2019, 2:00 PM
lib/Sema/SemaTemplateInstantiate.cpp
2687–2689

I think this can be simplified to "if Windows" at this point. But, I'm confused why we need this change to the general template instantiation machinery... Anyway, I'll send the simplification as a code review.

mstorsjo marked an inline comment as done.Apr 29 2019, 2:09 PM
mstorsjo added inline comments.
lib/Sema/SemaTemplateInstantiate.cpp
2687–2689

Yes, I believe so. Not sure about why this is needed though.

In case GCC actually does make a move on this matter, and chooses a different path for fixing it, we might want to follow that so in that case we might need to reintroduce some condition like this. But until that hypothetical case, let's simplify it.