hans added a comment. In https://reviews.llvm.org/D26657#597859, @smeenai wrote:
> In https://reviews.llvm.org/D26657#597523, @hans wrote: > > > In https://reviews.llvm.org/D26657#596897, @smeenai wrote: > > > > > In https://reviews.llvm.org/D26657#596759, @hans wrote: > > > > > > > > On MSVC, if an implicit instantiation already exists and an explicit > > > > > instantiation definition with a DLL attribute is created, the DLL > > > > > attribute still takes effect. Make clang match this behavior. > > > > > > > > This is scary territory, and behaviour I think might be hard for us to > > > > match. > > > > > > > > What if we already codegenned the implicit instantiation? > > > > > > > > > Hmm. Under what specific cases would that happen? > > > > > > Actually maybe I was being overly cautius. I think > > https://reviews.llvm.org/rL225570 might just make this work. > > > > > I can see odd behavior differences occurring for `dllimport`. For > > > example, for the C++ source in https://reviews.llvm.org/P7936, if I > > > compile with clang (even with this patch applied), the `t.f()` call in > > > `g` gets assembled to a call to `?f@?$s@H@@QAEHXZ`. If I hoist the > > > dllimport explicit instantiation definition above `g`, `t.f()` instead > > > assembles to `__imp_?f@?$s@H@@QAEHXZ`. > > > > Yeah, dllimport is interesting here. If you turn on `-O1`, it will call the > > `__imp` one. Fun times :-) > > > Huh. Weird stuff. > > > > > > >> With cl 19.00.24210, `t.f()` always assembles to `__imp_?f@?$s@H@@QAEHXZ`, > >> regardless of the placement of the explicit instantiation definition. > > > > Presumably they parse and analyze the whole translation unit before > > emitting any code, which makes changing things around later easier for them > > than for us. > > > >> I'm having a harder time imagining things going awry for `dllexport`. I > >> can limit this patch to only `dllexport`, if that's going to be safer. > > > > Yeah, I think https://reviews.llvm.org/rL225570 makes this work actually. > > So is the conclusion that this is probably safe as is, or should I limit it > to `dllexport` specifically? I think we should limit it do `dllexport` if we can't fix the `dllimport` case. >>> To give some context, libc++ uses extern templates extensively, and I ran >>> into an issue with dllexport explicit locale template instantiations >>> <https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/src/locale.cpp;287072$6034-6094?color=1>. >>> Some of those instantiations are also implicitly instantiated via sizeof >>> calls >>> <https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/src/locale.cpp;287072$67,77,87?color=1>. >>> Therefore, all instances which make is called on >>> <https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/src/locale.cpp;287072$177-204?color=1> >>> end up having their `dllexport` attribute ignored. >>> >>> We could work around this in libc++ by hoisting the affected instantiations >>> to near the top of the file, but it seemed preferable to make clang match >>> MSVC's behavior instead, at least for `dllexport`. I don't have any >>> particular interest in making `dllimport` semantics match MSVC for this >>> specific case. >> >> Dont' we need `dllimport` to work analogously though? When using libc++, >> don't these template members need to be dllimported? If not, why are they >> being exported in the first place? > > Ah. We put `dllimport` on the explicit template instantiation declarations in > the header > <https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/include/locale> > (all the instances of `_LIBCPP_EXTERN_TEMPLATE2` in that file). Those get > paired with the `dllexport` on the corresponding instantiation definitions. > If you're compiling the library, the declarations have no DLL attribute, and > the definitions have `dllexport`. If you're referencing the headers from > outside the library, the declarations get the `dllimport`. https://reviews.llvm.org/D26657 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
