[libc++] Introduce a new attribute keyword for Clang improves compatibility with Mingw-GCC

[kikairoya]

A new ABI annotation keyword _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 needs to be introduced and attached to ostream::sentry::sentry, ostream::sentry::~sentry and istream::sentry to improve binary compatibility on MinGW platform.

In MinGW environment, Clang handles dllexport attribute of internal class that defined in class template in different way from GCC. This incompatibility should be fixed but breaks ABI of libc++, so we need to introduce the new keyword to keep ABI in MinGW environment with old and patched Clang and to stay ABI compatible on other platforms.

This attribute is attached only for basic_ostream::sentry::sentry, basic_ostream::sentry::~sentry and basic_istream::sentry::sentry. Other entities won't be affected by patching Clang so doesn't need to be annotate.

Background

Clang (targeting MinGW a.k.a. windows-gnu, slightly different from windows-msvc) handles template instantiation:

• When exporting: extern template __declspec(dllexport) class TheTemplateClass<T>;
  allows exporting the outer template instantiation, but not its nested types.
• When importing: extern template __declspec(dllimport) class TheTemplateClass<T>;
  try to import the outer template instantiation (absence of declspec(dllimport) gives same result too by effect of implicit --enable-auto-import), but not its nested types - they will be instantiated in client object.

But MinGW-GCC handles template instantiation differently:

• When exporting: extern template __declspec(dllexport) class TheTemplateClass<T>;
  allows exporting the outer template instantiation, but not its nested types.
• When importing: extern template __declspec(dllimport) class TheTemplateClass<T>;
  causes MinGW-GCC to also try importing nested types such as TheTemplateClass::InnerClass,
  even if they were never exported. This leads to linker errors like: undefined reference to TheTemplateClass<T>::InnerClass::...

This difference causes link-time problems ( duplicated symbol or undefined reference ) or run-time problems ( illegal memory access, crash or other strange errors ) as reported in #135910 , so we are trying to align the behavior of Clang to MinGW-GCC.

But modifying Clang breaks libc++:

ld.lld: error: undefined symbol: std::__1::basic_ostream<char, std::__1::char_traits<char>>::sentry::sentry(std::__1::basic_ostream<char, std::__1::char_traits<char>>&)
>>> referenced by tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/NeonEmitter.cpp.obj:(std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:nn200100]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned int))
>>> referenced by tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/NeonEmitter.cpp.obj:((anonymous namespace)::Intrinsic::emitReverseVariable((anonymous namespace)::Variable&, (anonymous namespace)::Variable&))

ld.lld: error: undefined symbol: std::__1::basic_ostream<char, std::__1::char_traits<char>>::sentry::~sentry()
>>> referenced by tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/NeonEmitter.cpp.obj:(std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:nn200100]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned int))
>>> referenced by tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/NeonEmitter.cpp.obj:((anonymous namespace)::Intrinsic::emitReverseVariable((anonymous namespace)::Variable&, (anonymous namespace)::Variable&))

so we need to fix symbol visibility annotation in libc++ prior to patch Clang.

Effects

What attaching _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 to ostream::sentry::sentrys does:

• in MinGW environment:
  Expanded to _LIBCPP_HIDE_FROM_ABI.
  • While building a DLL:
    Virtually no-op while Clang and MinGW-GCC doesn't export them from a past.
  • Using a DLL from client-code:
    Forces instantiate in client code and prohibits trying to import from DLL.
    This is same to what former Clang does and gains compatibility with patched Clang and MinGW-GCC.
• for all other platforms including MSVC:
  Expanded to _LIBCPP_HIDE_FROM_ABI_AFTER_V1. Keeps V1 ABI stable.

Thus, this change introduces no significant side-effects except for needing to add inline together.

Other options that were not adopted

1. Modify GCC's behavior:
   @mstorsjo reported to GCC as defect over 5 years ago but there is no response yet: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89087 .
   There seems to be no chance of fixing it.

2. Attaching _LIBCPP_EXPORTED_FROM_ABI like:

   --- a/libcxx/include/__ostream/basic_ostream.h
   +++ b/libcxx/include/__ostream/basic_ostream.h
   @@ -71,7 +71,7 @@ protected:
   
    public:
      // 27.7.2.4 Prefix/suffix:
   -  class sentry;
   +  class _LIBCPP_EXPORTED_FROM_ABI sentry;
   
      // 27.7.2.6 Formatted output:
      inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&)) {
   

   This is simply wrong. Breaks ABI and doesn't work with template argument other than char or wchar_t.

3. Declaring ostream::sentrys with __attribute__((exclude_from_explicit_instantiation)) if __MINGW32__ and when client-side with a new keyword like (empty if in other conditions):

   --- a/libcxx/include/__config
   +++ b/libcxx/include/__config
   @@ -365,6 +365,11 @@ typedef __char32_t char32_t;
    #      define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
    #      define _LIBCPP_OVERRIDABLE_FUNC_VIS
    #      define _LIBCPP_EXPORTED_FROM_ABI
   +#      if !__has_attribute(exclude_from_explicit_instantiation) || defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
   +#        define _LIBCPP_INNER_CLASS_IN_TEMPLATE_VIS
   +#      else
   +#        define _LIBCPP_INNER_CLASS_IN_TEMPLATE_VIS __attribute__((__exclude_from_explicit_instantiation__))
   +#      endif
    #    elif defined(_LIBCPP_BUILDING_LIBRARY)
    #      if defined(__MINGW32__)
    #        define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __declspec(dllexport)
   

   and

   --- a/libcxx/include/__ostream/basic_ostream.h
   +++ b/libcxx/include/__ostream/basic_ostream.h
   @@ -72,7 +72,7 @@ protected:
   
    public:
      // 27.7.2.4 Prefix/suffix:
   -  class sentry;
   +  class _LIBCPP_INNER_CLASS_IN_TEMPLATE_VIS sentry;
   
      // 27.7.2.6 Formatted output:
      inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&)) {
   

   This works well but leaves GCC incompatible, so this idea is not perfect.

4. Instantiate inner class explicitly

   #  if defined(__MINGW32__) || defined(__CYGWIN__)
   extern template _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_ostream<char>::sentry::sentry(basic_ostream<char>& __os);
   extern template _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_ostream<char>::sentry::~sentry();
   #  endif
   

   This works for both of Clang and GCC, but scattering # if blocks is not a good style.

5. Similar to this proposal but expand to nothing for non-MinGW platforms like this:

   #  if defined(__MINGW32__) || defined(__CYGWIN__)
   #    define _LIBCPP_HIDE_FROM_ABI_IF_MINGW inline _LIBCPP_HIDE_FROM_ABI
   #  else
   #    define _LIBCPP_HIDE_FROM_ABI_IF_MINGW
   #  endif
   

   This works fine. If keeping non-inline is preferred to keeping consistency of presence or absence of inline between platforms, this is an option.

6. Modify Clang further to inherit dllexport to inner-types and export them
   I guess this option works without any breakage of existing binaries but introduce a new incompatibility with MinGW-GCC.
   Additionally, this approach doesn't help make libc++ buildable with MinGW-GCC ( options 4, 5 and this PR do).

Conclusion

Due to incompatible behavior on MinGW platform, Clang needs to be modified. But patching Clang breaks libc++ so adjusting visibility of some symbols is required. Any keyword already exist can't be suitable so we're going to introduce a new keyword named _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[mstorsjo]

In MinGW environment, Clang handles dllexport attribute of internal class that defined in class template in different way from GCC. This incompatibility should be fixed but breaks ABI of libc++

I didn't quite see this bit answered here; in which way would that break the ABI, if we'd fix this incompatibility? If we'd make libc++.dll export more symbols than we did before, that wouldn't break anything for preexisting callers of the DLL. New binaries built would obviously require the new libc++.dll though, but that's generally the rule anyway.

But MinGW-GCC handles template instantiation differently:

• When exporting: extern template __declspec(dllexport) class TheTemplateClass<T>;
  allows exporting the outer template instantiation, but not its nested types (e.g., InnerClass).
• When importing: extern template __declspec(dllimport) class TheTemplateClass<T>;
  causes MinGW-GCC to also try importing nested types such as TheTemplateClass::InnerClass,
  even if they were never exported. This leads to linker errors like: undefined reference to TheTemplateClass<T>::InnerClass::...

This is indeed self-inconsistent, and is the issue that I reported at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89087. Now as libstdc++ doesn't use explicit dllexports, but just uses the default fallback behaviour of exporting all symbols, they're not really hit by this issue, so there's not much pressure to fix it on their side. (We could also do the same in libc++, by explicitly passing -Wl,--export-all-symbols when building libc++.dll.)

Can you elaborate on what would break if we'd try to fix this on the Clang side (making Clang do similarly to what GCC does, but self-consistently by making it export the nested class, like the dllimport behaviour seems to expect)?

[kikairoya]

I didn't quite see this bit answered here; in which way would that break the ABI, if we'd fix this incompatibility? If we'd make libc++.dll export more symbols than we did before, that wouldn't break anything for preexisting callers of the DLL. New binaries built would obviously require the new libc++.dll though, but that's generally the rule anyway.

I agree that exporting additional symbols wouldn't break existing binaries at the symbol usage level. However, I guess, from user's perspective, needing to upgrade the DLL might be treated as ABI change. Saying it "breaks ABI" may have been too strong but not completely wrong, as seeing manner of ELF world, exporting new symbols will cause change SOVERSION.

Can you elaborate on what would break if we'd try to fix this on the Clang side (making Clang do similarly to what GCC does, but self-consistently by making it export the nested class, like the dllimport behaviour seems to expect)?

I think that makes a new 'dialect' to be avoided (but I understand should not to emulate GCC's defect, too). As real harm, reverse of #135910 might occur but I don't have any test. Need a time to clarify that harms or not.

Regardless to approach of fixing Clang incompatibility with MinGW-GCC, this new keyword would be viable to able to work libc++ with MinGW-GCC.

[jeremyd2019]

In MinGW environment, Clang handles dllexport attribute of internal class that defined in class template in different way from GCC. This incompatibility should be fixed but breaks ABI of libc++, so we need to introduce the new keyword to keep ABI in MinGW environment with old and patched Clang and to stay ABI compatible on other platforms.

I believe we established that dllexport and dllimport are handled the same between Clang and GCC, and it is the more mundane extern that is handled differently.

[mstorsjo]

I agree that exporting additional symbols wouldn't break existing binaries at the symbol usage level. However, I guess, from user's perspective, needing to upgrade the DLL might be treated as ABI change. Saying it "breaks ABI" may have been too strong but not completely wrong, as seeing manner of ELF world, exporting new symbols will cause change SOVERSION.

I don't see this (in itself at least) as a reason to rule out this approach. If compiling your code with a new changed Clang version X, I would expect it to be totally reasonable to need to use libc++ >= X. (Even with this approach in this PR, changing libc++, we'd require this anyway.) And if you compile your app with libc++ == version X, then you do require having libc++.dll >= version X at runtime (regardless of whether we add any symbol or not). So the end result is the same anyway; if we change Clang in version X, you'll need to use libc++.dll >= X too.

Can you elaborate on what would break if we'd try to fix this on the Clang side (making Clang do similarly to what GCC does, but self-consistently by making it export the nested class, like the dllimport behaviour seems to expect)?

I think that makes a new 'dialect' to be avoided (but I understand should not to emulate GCC's defect, too).

Yes, that's indeed a potential problem.

However, I think the most relevant question is, if GCC would fix the issue on their side, which way would they fix it, so that it is most consistent with their model of how these things fit together? That would ideally be our guide for how we should proceed. Initially, I thought that da93dec would be it, but now we've learnt that it actually breaks things in some cases, as noted in #135910, so we need to backtrack and reevaluate. So if we, hypothetically, would have the resources/energy to do so, would we fix it in GCC by making the dllexport apply to the nested class too?

Then the practicality is mostly that the issue isn't a high priority for GCC (and doesn't really affect them much), while it is a higher priority for us.

As real harm, reverse of #135910 might occur but I don't have any test. Need a time to clarify that harms or not.

Yes, that's also possible... Theoretically, changing Clang could affect what symbols all C++ libraries export, at least libraries that use explicit dllexport. In practice, I think it's not all that common to use this construct (explicit template instantiations with nested classes), outside of the standard library. It's possible that Boost does something like that though - maybe worth testing, if we'd evaluate that approach further.

Regardless to approach of fixing Clang incompatibility with MinGW-GCC, this new keyword would be viable to able to work libc++ with MinGW-GCC.

That's a reasonable point.

Have you been able to test using libc++ with GCC in mingw environments so far, so you can reproduce that you run into this issue with the sentry nested classes, and so you can verify that this patch indeed would fix that issue, or is it hypothetical?

[jeremyd2019]

Have you been able to test using libc++ with GCC in mingw environments so far, so you can reproduce that you run into this issue with the sentry nested classes, and so you can verify that this patch indeed would fix that issue, or is it hypothetical?

I saw #135910 (comment), I don't know if the more recent patch has been tried this way though.

[jeremyd2019]

I may not be able to give this much attention until next week, but don't hold off on my account, I think the changes are good

[jeremyd2019]

Suggested change

	// with __declspec(dllexport/dllimport).
	// with __declspec(dllexport/dllimport) and extern.

[jeremyd2019]

only if there are explicit extern instantiations of that outer class template.

[jeremyd2019]

Suggested change

	// - When importing: 'extern template __declspec(dllimport) class TheTemplateClass<T>;'
	// - When importing: 'extern template class TheTemplateClass<T>;'

The __MINGW32__ paths don't seem to put explicit dllimport attributes.

[kikairoya]

I agree that exporting additional symbols wouldn't break existing binaries at the symbol usage level. However, I guess, from user's perspective, needing to upgrade the DLL might be treated as ABI change. Saying it "breaks ABI" may have been too strong but not completely wrong, as seeing manner of ELF world, exporting new symbols will cause change SOVERSION.

I don't see this (in itself at least) as a reason to rule out this approach.

Yes, I agree. I didn't examine to achieve this by more modifying Clang itself but by modifying libc++ as described in 4., to instantiate and export explicitly.

So if we, hypothetically, would have the resources/energy to do so, would we fix it in GCC by making the dllexport apply to the nested class too?

It's possible to both approach - to export or not to import. I consider it's appropriate to export inner classes as align to their outer template instantiation, but opposite is also an option like MSVC does. If GCC may do latter is considered, better not to export currently them as once they're exported, can't be removed (though, will not issue to leave them exported).

In practice, I think it's not all that common to use this construct (explicit template instantiations with nested classes), outside of the standard library.

LLVM itself has such instantiations but won't matter as __declspec(dllexport) isn't used for MinGW.

Registry<T> has class iterator

llvm-project/llvm/include/llvm/Support/Registry.h

Line 150 in 989aadf

template class LLVM_ABI_EXPORT Registry<REGISTRY_CLASS::type>; \

GenericDomTreeUpdater<...> has struct DomTreeUpdate

llvm-project/llvm/include/llvm/Analysis/DomTreeUpdater.h

Lines 120 to 122 in 989aadf

	extern template void
	GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
	PostDominatorTree>::recalculate(Function &F);

llvm-project/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h

Lines 23 to 24 in 989aadf

	template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
	class GenericDomTreeUpdater {

The cases of satisfy all of conditions (MinGW, built to DLL, use of __declspec(dllexport), instantiate explicitly a class template contains a class that has a member function or a static data member and export it) is rare but could be seen.

Have you been able to test using libc++ with GCC in mingw environments so far, so you can reproduce that you run into this issue with the sentry nested classes, and so you can verify that this patch indeed would fix that issue, or is it hypothetical?

I saw #135910 (comment), I don't know if the more recent patch has been tried this way though.

I have tested by my hand to this newest patch makes linking with g++ (and BFD ld) against clang-built libc++.dll succeed. Opposite should success too, but I haven't tested yet.

[mstorsjo]

So if we, hypothetically, would have the resources/energy to do so, would we fix it in GCC by making the dllexport apply to the nested class too?

It's possible to both approach - to export or not to import. I consider it's appropriate to export inner classes as align to their outer template instantiation, but opposite is also an option like MSVC does. If GCC may do latter is considered, better not to export currently them as once they're exported, can't be removed (though, will not issue to leave them exported).

That's true. However I think it's less probable that they would change in which cases things are instantiated (as it is mostly consistent across mingw and linux right now - the only inconsistency in GCC is where dllexport applies).

In practice, I think it's not all that common to use this construct (explicit template instantiations with nested classes), outside of the standard library.

LLVM itself has such instantiations but won't matter as __declspec(dllexport) isn't used for MinGW.

Registry<T> has class iterator

llvm-project/llvm/include/llvm/Support/Registry.h

Line 150 in 989aadf

template class LLVM_ABI_EXPORT Registry<REGISTRY_CLASS::type>; \

GenericDomTreeUpdater<...> has struct DomTreeUpdate

llvm-project/llvm/include/llvm/Analysis/DomTreeUpdater.h

Lines 120 to 122 in 989aadf

	extern template void
	GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
	PostDominatorTree>::recalculate(Function &F);

llvm-project/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h

Lines 23 to 24 in 989aadf

	template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
	class GenericDomTreeUpdater {

The cases of satisfy all of conditions (MinGW, built to DLL, use of __declspec(dllexport), instantiate explicitly a class template contains a class that has a member function or a static data member and export it) is rare but could be seen.

Oh, good to know! Also FWIW, these don't have __declspec(dllexport) right now, but there is an effort ongoing to annotate all the LLVM APIs with suitable attributes for dllexport/import (to make DLL builds feasible with MSVC). So we may need to take this into account at some point too...

[kikairoya]

Opposite should success too, but I haven't tested yet.

With msys2-ucrt64 toolchain, std::ostream::sentry in g++-built libc++.dll can be used by both of clang++ and g++ after applying this patch.

The libc++ built by

> ASM=gcc CC=gcc CXX=g++ cmake -Bbuild-runtime-mingw64 -Sruntimes -DCMAKE_BUILD_TYPE=Release "-DLLVM_ENABLE_RUNTIMES=libcxx;libcxxabi;libunwind" -DLIBCXX_ENABLE_STATIC_ABI_LIBRARY=ON -DLIBCXX_EXTRA_SITE_DEFINES="__USE_MINGW_ANSI_STDIO=1"  -DLIBCXXABI_ENABLE_SHARED=OFF  -DLIBCXXABI_USE_LLVM_UNWINDER=ON  -DLIBUNWIND_ENABLE_SHARED=OFF -DLLVM_ENABLE_LLD=ON

with requiring a patch #141328 and comment out here because lack of aligned_alloc.

llvm-project/libcxx/include/cstdlib

Line 152 in f0ff2be

using ::aligned_alloc _LIBCPP_USING_IF_EXISTS;

[jeremyd2019]

Is this not ready for review now?

[kikairoya]

Sorry, I'm currently running into some other issues while trying to investigate the side effects of exporting inner classes by modifying Clang.
If it’s not necessary to wait for that investigation, I believe this PR is ready for review (with small changes to commit msg.)

[kikairoya]

Can you elaborate on what would break if we'd try to fix this on the Clang side (making Clang do similarly to what GCC does, but self-consistently by making it export the nested class, like the dllimport behaviour seems to expect)?

I think that makes a new 'dialect' to be avoided (but I understand should not to emulate GCC's defect, too). As real harm, reverse of #135910 might occur but I don't have any test. Need a time to clarify that harms or not.

I have tested with this patch: https://gist.github.com/kikairoya/97c6daea92b94db2d5b72fb200ddd1bd
and can't find that reverse of #135910 or similar problem occurs from combinations of DLL+EXE or DLL+DLL+EXE built by Clang with this patch or GCC, on Cygwin environment.
Thus, I would say the approach can be viable without breaking existing binaries.

(as it is mostly consistent across mingw and linux right now - the only inconsistency in GCC is where dllexport applies)

My understand through this test, in Clang, handling of dllexport/dllimport quite differs from visibility control and might require a large refactoring to make its semantics consist to visibility control. In fact, my ad-hoc patch causes doubly-exporting constructors of inner class.

I would prefer to first resolve the incompatibility between Clang and GCC with this PR (and #135910 (comment) ), and then discuss how Clang’s behavior should be handled in a follow-up. Would that be acceptable to you, @mstorsjo?

[mstorsjo]

Can you elaborate on what would break if we'd try to fix this on the Clang side (making Clang do similarly to what GCC does, but self-consistently by making it export the nested class, like the dllimport behaviour seems to expect)?

I think that makes a new 'dialect' to be avoided (but I understand should not to emulate GCC's defect, too). As real harm, reverse of #135910 might occur but I don't have any test. Need a time to clarify that harms or not.

I have tested with this patch: https://gist.github.com/kikairoya/97c6daea92b94db2d5b72fb200ddd1bd and can't find that reverse of #135910 or similar problem occurs from combinations of DLL+EXE or DLL+DLL+EXE built by Clang with this patch or GCC, on Cygwin environment. Thus, I would say the approach can be viable without breaking existing binaries.

Thanks for investigating where and how this can be fixed! That's very much appreciated!

(as it is mostly consistent across mingw and linux right now - the only inconsistency in GCC is where dllexport applies)

My understand through this test, in Clang, handling of dllexport/dllimport quite differs from visibility control and might require a large refactoring to make its semantics consist to visibility control.

Do you mean that within Clang, dllexport/import and visibility are handled in different ways? Yes, that's probably true.

In fact, my ad-hoc patch causes doubly-exporting constructors of inner class.

Hmm, in which way does it cause them to be doubly exported? Does it emit two dllexport attributes somehow, or does it generate two instances of the same function?

I would prefer to first resolve the incompatibility between Clang and GCC with this PR (and #135910 (comment) ), and then discuss how Clang’s behavior should be handled in a follow-up. Would that be acceptable to you, @mstorsjo?

It's not up to me, it's more up to the libc++ maintainers (who are quite limited on time, unfortunately), and how well we can argue the case that the complexity added by these patches is acceptable and warranted.

[kikairoya]

In fact, my ad-hoc patch causes doubly-exporting constructors of inner class.

Hmm, in which way does it cause them to be doubly exported? Does it emit two dllexport attributes somehow, or does it generate two instances of the same function?

I was wrong. They are "complete object constructor" and "base object constructor" (ref: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-special-ctor-dtor ), c++filt shows same result for these. Each function body and export table entry is emitted exactly once.
Though, I found a new suspicious result - copy constructor and operator= are generated and exported automatically for outer class template but not for inner class ( of course, they are exported as expected when defined manually ).
The files used by this test are added: https://gist.github.com/kikairoya/97c6daea92b94db2d5b72fb200ddd1bd

[kikairoya]

Added 6.) to description.

[mstorsjo]

6. Modify Clang more to inherit dllexport to inner-type and export it
I guess this option works without any breakage of existing binaries but introduce a new incompatibility with MinGW-GCC.
Additionally, same as 3., MinGW-GCC will be left incompatible with libc++.

I don't see how this would leave GCC incompatible with libc++. If you build the libc++ DLL with Clang, it would dllexport the necessary inner classes. When building user code with GCC, it does not instantiate the inner classes, and it would be able to link them from the libc++ DLL, right? The only issue would be if you'd try to build the libc++ DLL with GCC, where it wouldn't have sufficient dllexports (linking with -Wl,--export-all-symbols would help though).

[kikairoya]

6. Modify Clang more to inherit dllexport to inner-type and export it
   I guess this option works without any breakage of existing binaries but introduce a new incompatibility with MinGW-GCC.
   Additionally, same as 3., MinGW-GCC will be left incompatible with libc++.

I don't see how this would leave GCC incompatible with libc++. If you build the libc++ DLL with Clang, it would dllexport the necessary inner classes. When building user code with GCC, it does not instantiate the inner classes, and it would be able to link them from the libc++ DLL, right? The only issue would be if you'd try to build the libc++ DLL with GCC, where it wouldn't have sufficient dllexports (linking with -Wl,--export-all-symbols would help though).

Yes, technically you're right. I've updated 6.) to reflect that perspective more accurately.
Though, I would consider it incompatible if -Wl,--export-all-symbols is still needed despite using __declspec(dllexport). Otherwise, we could say Clang targeting MinGW doesn't need to annotate exports at all - but that's clearly not acceptable in libc++'s model.

[mstorsjo]

6. Modify Clang more to inherit dllexport to inner-type and export it
   I guess this option works without any breakage of existing binaries but introduce a new incompatibility with MinGW-GCC.
   Additionally, same as 3., MinGW-GCC will be left incompatible with libc++.

I don't see how this would leave GCC incompatible with libc++. If you build the libc++ DLL with Clang, it would dllexport the necessary inner classes. When building user code with GCC, it does not instantiate the inner classes, and it would be able to link them from the libc++ DLL, right? The only issue would be if you'd try to build the libc++ DLL with GCC, where it wouldn't have sufficient dllexports (linking with -Wl,--export-all-symbols would help though).

Yes, technically you're right. I've updated 6.) to reflect that perspective more accurately. Though, I would consider it incompatible if -Wl,--export-all-symbols is still needed despite using __declspec(dllexport). Otherwise, we could say Clang targeting MinGW doesn't need to annotate exports at all - but that's clearly not acceptable in libc++'s model.

Yes, if building the libc++ DLL with Clang with dllexport annotations, then we clearly don't want to use -Wl,--export-all-symbols. But if we'd build it with GCC, if that even is feasible, we would need that option as a workaround for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89087.

[kikairoya]

I think I’ve covered everything needed before requesting a review, but I might have missed something. Please let me know if anything still seems off.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Tomohiro Kashiwada (kikairoya)

Changes

A new ABI annotation keyword _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 needs to be introduced and attached to ostream::sentry::sentry, ostream::sentry::~sentry and istream::sentry to improve binary compatibility on MinGW platform.

In MinGW environment, Clang handles dllexport attribute of internal class that defined in class template in different way from GCC. This incompatibility should be fixed but breaks ABI of libc++, so we need to introduce the new keyword to keep ABI in MinGW environment with old and patched Clang and to stay ABI compatible on other platforms.

This attribute is attached only for basic_ostream::sentry::sentry, basic_ostream::sentry::~sentry and basic_istream::sentry::sentry. Other entities won't be affected by patching Clang so doesn't need to be annotate.

Background

Clang (targeting MinGW a.k.a. windows-gnu, slightly different from windows-msvc) handles template instantiation:

• When exporting: extern template __declspec(dllexport) class TheTemplateClass<T>;
  allows exporting the outer template instantiation, but not its nested types.
• When importing: extern template __declspec(dllimport) class TheTemplateClass<T>;
  try to import the outer template instantiation (absence of declspec(dllimport) gives same result too by effect of implicit --enable-auto-import), but not its nested types - they will be instantiated in client object.

But MinGW-GCC handles template instantiation differently:

• When exporting: extern template __declspec(dllexport) class TheTemplateClass<T>;
  allows exporting the outer template instantiation, but not its nested types.
• When importing: extern template __declspec(dllimport) class TheTemplateClass<T>;
  causes MinGW-GCC to also try importing nested types such as TheTemplateClass<T>::InnerClass,
  even if they were never exported. This leads to linker errors like: undefined reference to TheTemplateClass<T>::InnerClass::...

This difference causes link-time problems ( duplicated symbol or undefined reference ) or run-time problems ( illegal memory access, crash or other strange errors ) as reported in #135910 , so we are trying to align the behavior of Clang to MinGW-GCC.

But modifying Clang breaks libc++:

ld.lld: error: undefined symbol: std::__1::basic_ostream<char, std::__1::char_traits<char>>::sentry::sentry(std::__1::basic_ostream<char, std::__1::char_traits<char>>&)
>>> referenced by tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/NeonEmitter.cpp.obj:(std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:nn200100]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned int))
>>> referenced by tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/NeonEmitter.cpp.obj:((anonymous namespace)::Intrinsic::emitReverseVariable((anonymous namespace)::Variable&, (anonymous namespace)::Variable&))

ld.lld: error: undefined symbol: std::__1::basic_ostream<char, std::__1::char_traits<char>>::sentry::~sentry()
>>> referenced by tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/NeonEmitter.cpp.obj:(std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:nn200100]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned int))
>>> referenced by tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/NeonEmitter.cpp.obj:((anonymous namespace)::Intrinsic::emitReverseVariable((anonymous namespace)::Variable&, (anonymous namespace)::Variable&))

so we need to fix symbol visibility annotation in libc++ prior to patch Clang.

Effects

What attaching _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 to ostream::sentry::sentrys does:

• in MinGW environment:
  Expanded to _LIBCPP_HIDE_FROM_ABI.
  • While building a DLL:
    Virtually no-op while Clang and MinGW-GCC doesn't export them from a past.
  • Using a DLL from client-code:
    Forces instantiate in client code and prohibits trying to import from DLL.
    This is same to what former Clang does and gains compatibility with patched Clang and MinGW-GCC.
• for all other platforms including MSVC:
  Expanded to _LIBCPP_HIDE_FROM_ABI_AFTER_V1. Keeps V1 ABI stable.

Thus, this change introduces no significant side-effects except for needing to add inline together.

Other options that were not adopted

1. Modify GCC's behavior:
   @mstorsjo reported to GCC as defect over 5 years ago but there is no response yet: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89087 .
   There seems to be no chance of fixing it.

2. Attaching _LIBCPP_EXPORTED_FROM_ABI like:

   --- a/libcxx/include/__ostream/basic_ostream.h
   +++ b/libcxx/include/__ostream/basic_ostream.h
   @@ -71,7 +71,7 @@ protected:
   
    public:
      // 27.7.2.4 Prefix/suffix:
   -  class sentry;
   +  class _LIBCPP_EXPORTED_FROM_ABI sentry;
   
      // 27.7.2.6 Formatted output:
      inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&)) {
   

   This is simply wrong. Breaks ABI and doesn't work with template argument other than char or wchar_t.

3. Declaring ostream::sentrys with __attribute__((exclude_from_explicit_instantiation)) if __MINGW32__ and when client-side with a new keyword like (empty if in other conditions):

   --- a/libcxx/include/__config
   +++ b/libcxx/include/__config
   @@ -365,6 +365,11 @@ typedef __char32_t char32_t;
    #      define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
    #      define _LIBCPP_OVERRIDABLE_FUNC_VIS
    #      define _LIBCPP_EXPORTED_FROM_ABI
   +#      if !__has_attribute(exclude_from_explicit_instantiation) || defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
   +#        define _LIBCPP_INNER_CLASS_IN_TEMPLATE_VIS
   +#      else
   +#        define _LIBCPP_INNER_CLASS_IN_TEMPLATE_VIS __attribute__((__exclude_from_explicit_instantiation__))
   +#      endif
    #    elif defined(_LIBCPP_BUILDING_LIBRARY)
    #      if defined(__MINGW32__)
    #        define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __declspec(dllexport)
   

   and

   --- a/libcxx/include/__ostream/basic_ostream.h
   +++ b/libcxx/include/__ostream/basic_ostream.h
   @@ -72,7 +72,7 @@ protected:
   
    public:
      // 27.7.2.4 Prefix/suffix:
   -  class sentry;
   +  class _LIBCPP_INNER_CLASS_IN_TEMPLATE_VIS sentry;
   
      // 27.7.2.6 Formatted output:
      inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&)) {
   

   This works well but leaves GCC incompatible, so this idea is not perfect.

4. Instantiate inner class explicitly

   #  if defined(__MINGW32__) || defined(__CYGWIN__)
   extern template _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_ostream<char>::sentry::sentry(basic_ostream<char>& __os);
   extern template _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_ostream<char>::sentry::~sentry();
   #  endif
   

   This works for both of Clang and GCC, but scattering # if blocks is not a good style.

5. Similar to this proposal but expand to nothing for non-MinGW platforms like this:

   #  if defined(__MINGW32__) || defined(__CYGWIN__)
   #    define _LIBCPP_HIDE_FROM_ABI_IF_MINGW inline _LIBCPP_HIDE_FROM_ABI
   #  else
   #    define _LIBCPP_HIDE_FROM_ABI_IF_MINGW
   #  endif
   

   This works fine. If keeping non-inline is preferred to keeping consistency of presence or absence of inline between platforms, this is an option.

6. Modify Clang further to inherit dllexport to inner-types and export them
   I guess this option works without any breakage of existing binaries but introduce a new incompatibility with MinGW-GCC.
   Additionally, this approach doesn't help make libc++ buildable with MinGW-GCC ( options 4, 5 and this PR do).

Conclusion

Due to incompatible behavior on MinGW platform, Clang needs to be modified. But patching Clang breaks libc++ so adjusting visibility of some symbols is required. Any keyword already exist can't be suitable so we're going to introduce a new keyword named _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1.

---

Full diff: https://github.com/llvm/llvm-project/pull/141040.diff

3 Files Affected:

• (modified) libcxx/include/__config (+44)
• (modified) libcxx/include/__ostream/basic_ostream.h (+2-2)
• (modified) libcxx/include/istream (+2-1)

diff --git a/libcxx/include/__config b/libcxx/include/__config
index af8a297fdf3fd..b7e9872032dba 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -505,6 +505,50 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
 #  endif
 
+// _LIBCPP_HIDE_FROM_ABI is required for member functions defined within an inner class of a class template
+// (e.g., std::basic_ostream<...>::sentry::sentry(...)), due to inconsistent behavior in MinGW-GCC (and
+// Cygwin as well, in all relevant cases) regarding explicit instantiation declaration and symbol visibility
+// when combined with __declspec(dllexport).
+//
+// Previous versions of Clang did not exhibit this issue, but upcoming versions are expected to align with
+// GCC's behavior for compatibility. This is particularly important because some of libstdc++ packages
+// compiled with --with-default-libstdcxx-abi=gcc4-compatible are incompatible with Clang, resulting in linking
+// errors or runtime crushes.
+//
+// A few such member functions already exist (here are ostream::sentry::sentry, ostream::~sentry and
+// istream::sentry::sentry) were not previously marked with _LIBCPP_HIDE_FROM_ABI but should be to avoid symbol
+// visibility issues. However, adding the macro unconditionally would break the ABI on other platforms.
+//
+// Therefore, a dedicated macro _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 is introduced. This macro expands to
+// _LIBCPP_HIDE_FROM_ABI only when targeting MinGW, and to _LIBCPP_HIDE_FROM_ABI_AFTER_V1 on all other platforms.
+//
+// Going forward, whenever a new (static or non-static) member function or static data member is added to an
+// inner class within a class template that has explicit instantiation declaration, it must be annotated with
+// _LIBCPP_HIDE_FROM_ABI to ensure proper symbol visibility when targeting MinGW. Otherwise, the resulting DLL
+// will be unusable due to missing symbols.
+//
+// The underlying issue arises from how MinGW-GCC handles explicit instantiation declaration of a class template:
+//
+//   - When exporting: 'extern template __declspec(dllexport) class TheTemplateClass<T>;'
+//     allows exporting the outer template instantiation, but not its nested types (e.g., InnerClass).
+//     note: this is just a declaration, needs a definition as `template class TheTemplateClass<T>;' at somewere.
+//
+//   - When importing: 'extern template class TheTemplateClass<T>;'
+//     causes MinGW-GCC to also try importing nested types such as TheTemplateClass<T>::InnerClass,
+//     even if they were never exported. This leads to linker errors like:
+//     'undefined reference to TheTemplateClass<T>::InnerClass::...'
+//
+// This differs from Clang's historical behavior, which did not import nested classes implicitly.
+// However, to ensure ABI compatibility with the MinGW-GCC toolchain (commonly used in the MinGW ecosystem),
+// Clang will adopt this behavior as well.
+//
+// Note: As of this writing, Clang does not yet implement this behavior, since doing so would break libc++.
+#  if defined(__MINGW32__) || defined(__CYGWIN__)
+#    define _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
+#  else
+#    define _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 _LIBCPP_HIDE_FROM_ABI_AFTER_V1
+#  endif
+
 // Clang modules take a significant compile time hit when pushing and popping diagnostics.
 // Since all the headers are marked as system headers unless _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER is defined, we can
 // simply disable this pushing and popping when _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER isn't defined.
diff --git a/libcxx/include/__ostream/basic_ostream.h b/libcxx/include/__ostream/basic_ostream.h
index f7473a36d8ccc..9ae905f689b6a 100644
--- a/libcxx/include/__ostream/basic_ostream.h
+++ b/libcxx/include/__ostream/basic_ostream.h
@@ -186,8 +186,8 @@ class basic_ostream<_CharT, _Traits>::sentry {
   basic_ostream<_CharT, _Traits>& __os_;
 
 public:
-  explicit sentry(basic_ostream<_CharT, _Traits>& __os);
-  ~sentry();
+  explicit inline _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 sentry(basic_ostream<_CharT, _Traits>& __os);
+  inline _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1 ~sentry();
   sentry(const sentry&)            = delete;
   sentry& operator=(const sentry&) = delete;
 
diff --git a/libcxx/include/istream b/libcxx/include/istream
index 02546902494e3..2e6e8778680b2 100644
--- a/libcxx/include/istream
+++ b/libcxx/include/istream
@@ -309,7 +309,8 @@ class basic_istream<_CharT, _Traits>::sentry {
   bool __ok_;
 
 public:
-  explicit sentry(basic_istream<_CharT, _Traits>& __is, bool __noskipws = false);
+  explicit inline _LIBCPP_HIDE_FROM_ABI_MINGW_OR_AFTER_V1
+  sentry(basic_istream<_CharT, _Traits>& __is, bool __noskipws = false);
   //    ~sentry() = default;
 
   _LIBCPP_HIDE_FROM_ABI explicit operator bool() const { return __ok_; }