diff mbox series

libstdc++: Compile std::allocator instantiations as C++20

Message ID 20240411172142.587623-1-jwakely@redhat.com
State New
Headers show
Series libstdc++: Compile std::allocator instantiations as C++20 | expand

Commit Message

Jonathan Wakely April 11, 2024, 5:15 p.m. UTC 
I'm considering this late patch for gcc-14 to workaround an issue
discovered by a recent Clang change.

I'm not yet sure if Clang is right to require these symbols. It's not
really clear, because always_inline isn't part of the standard so it's
not clear how it should interact with explicit instantiations and
modules. Exporting these four extra symbols doesn't hurt, even if Clang
ends up reverting or revising its change that requires them.

Another way to fix it would be to suppress the explicit instantiation
declarations in <bits/allocator.h> for C++20, so that the compiler
always instantiates them implicitly as needed. We do similar things for
the explicit instantiations of std::string etc. so that new member
functions that aren't in the .so are implicitly instantiated as needed.

That would look like this instead:

Comments

Ville Voutilainen April 11, 2024, 5:33 p.m. UTC | #1 
On Thu, 11 Apr 2024 at 20:22, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> I'm considering this late patch for gcc-14 to workaround an issue
> discovered by a recent Clang change.
>
> I'm not yet sure if Clang is right to require these symbols. It's not
> really clear, because always_inline isn't part of the standard so it's
> not clear how it should interact with explicit instantiations and
> modules. Exporting these four extra symbols doesn't hurt, even if Clang
> ends up reverting or revising its change that requires them.
>
> Another way to fix it would be to suppress the explicit instantiation
> declarations in <bits/allocator.h> for C++20, so that the compiler
> always instantiates them implicitly as needed. We do similar things for
> the explicit instantiations of std::string etc. so that new member
> functions that aren't in the .so are implicitly instantiated as needed.
>
> That would look like this instead:
>
> --- a/libstdc++-v3/include/bits/allocator.h
> +++ b/libstdc++-v3/include/bits/allocator.h
> @@ -281,7 +281,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>    // Inhibit implicit instantiations for required instantiations,
>    // which are defined via explicit instantiations elsewhere.
> -#if _GLIBCXX_EXTERN_TEMPLATE
> +#if _GLIBCXX_EXTERN_TEMPLATE && __cplusplus <= 201703L
>    extern template class allocator<char>;
>    extern template class allocator<wchar_t>;
>  #endif
>
> But we might want to export the new functions from the library
> eventually anyway, so doing it now (before Clang 19 is released) might
> be the best option.
>
> Thoughts?

I think the symbol export is a fine solution. Both of these solutions
work, so I don't have a strong preference,
I have a minor preference for not suppressing explicit instantiations
that are otherwise already there,
but that is indeed not a strong preference.
Iain Sandoe April 11, 2024, 6:28 p.m. UTC | #2 
> On 11 Apr 2024, at 18:33, Ville Voutilainen <ville.voutilainen@gmail.com> wrote:
> 
> On Thu, 11 Apr 2024 at 20:22, Jonathan Wakely <jwakely@redhat.com> wrote:
>> 
>> I'm considering this late patch for gcc-14 to workaround an issue
>> discovered by a recent Clang change.
>> 
>> I'm not yet sure if Clang is right to require these symbols. It's not
>> really clear, because always_inline isn't part of the standard so it's
>> not clear how it should interact with explicit instantiations and
>> modules. Exporting these four extra symbols doesn't hurt, even if Clang
>> ends up reverting or revising its change that requires them.
>> 
>> Another way to fix it would be to suppress the explicit instantiation
>> declarations in <bits/allocator.h> for C++20, so that the compiler
>> always instantiates them implicitly as needed. We do similar things for
>> the explicit instantiations of std::string etc. so that new member
>> functions that aren't in the .so are implicitly instantiated as needed.
>> 
>> That would look like this instead:
>> 
>> --- a/libstdc++-v3/include/bits/allocator.h
>> +++ b/libstdc++-v3/include/bits/allocator.h
>> @@ -281,7 +281,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> 
>>   // Inhibit implicit instantiations for required instantiations,
>>   // which are defined via explicit instantiations elsewhere.
>> -#if _GLIBCXX_EXTERN_TEMPLATE
>> +#if _GLIBCXX_EXTERN_TEMPLATE && __cplusplus <= 201703L
>>   extern template class allocator<char>;
>>   extern template class allocator<wchar_t>;
>> #endif
>> 
>> But we might want to export the new functions from the library
>> eventually anyway, so doing it now (before Clang 19 is released) might
>> be the best option.

I think clang-19 will branch after gcc-14 (so the urgency is more on the GCC
side - or making a decision to back out of the clang change.

>> 
>> Thoughts?
> 
> I think the symbol export is a fine solution. Both of these solutions
> work, so I don't have a strong preference,
> I have a minor preference for not suppressing explicit instantiations
> that are otherwise already there,
> but that is indeed not a strong preference.

In the case of interaction between modules and items not identified in
the std, the only solution is to get agreement between the ‘vendors’.

If we emit the symbols, then that gets baked into the libstdc++ ABI, right?

Is there an alternate solution that we can propose? (there is a modules
implementer’s group that meets bi-weekly, including next Tuesday - 
this includes representation from GCC, clang, MSVC and usually some
of the larger players like google… it could be a topic of discussion if
there is some tidier proposal we could make.

Iain
diff mbox series

Patch

--- a/libstdc++-v3/include/bits/allocator.h
+++ b/libstdc++-v3/include/bits/allocator.h
@@ -281,7 +281,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   // Inhibit implicit instantiations for required instantiations,
   // which are defined via explicit instantiations elsewhere.
-#if _GLIBCXX_EXTERN_TEMPLATE
+#if _GLIBCXX_EXTERN_TEMPLATE && __cplusplus <= 201703L
   extern template class allocator<char>;
   extern template class allocator<wchar_t>;
 #endif

But we might want to export the new functions from the library
eventually anyway, so doing it now (before Clang 19 is released) might
be the best option.

Thoughts?


-- >8 --

This ensures that the new std::allocator::allocate and
std::allocator::deallocate functions are included in the explicit
instantiation definitions. They're only defined for C++20 and later
(because they are needed to support constexpr dynamic allocation) so
were not being instantiated when those files were compiled as C++98.

This isn't needed for GCC because those functions are marked
'always_inline' and so no extern symbols are needed. But a recent Clang
change has meant that always_inline functions do not get exported from
modules, and so the extern symbols (which were not previously being
instantiated or exported) are needed by Clang. See the Clang issue
https://github.com/llvm/llvm-project/issues/86893 for details.

The allocator-inst.cc file is changed to include <bits/allocator.h>
instead of all of <memory>, because otherwise it indirectly includes
<bits/basic_string.h> and the implicit instantiations of std::string and
std::wstring also instantiate std::allocator<char> and
std::allocator<wchar_t>, which for some reason suppresses the explicit
instantiation definitions of the default constructors in this file.
Just including <bits/allocator.h> avoids that problem.

libstdc++-v3/ChangeLog:

	* config/abi/pre/gnu.ver: Export new symbols.
	* src/c++20/Makefile.am: Add allocator-inst.o.
	* src/c++20/Makefile.in: Regenerate.
	* src/c++98/allocator-inst.cc: Move to...
	* src/c++20/allocator-inst.cc: ...here.
	* src/c++98/Makefile.am: Remove allocator-inst.o.
	* src/c++98/Makefile.in: Regenerate.
---
 libstdc++-v3/config/abi/pre/gnu.ver                 | 4 ++++
 libstdc++-v3/src/c++20/Makefile.am                  | 1 +
 libstdc++-v3/src/c++20/Makefile.in                  | 4 +++-
 libstdc++-v3/src/{c++98 => c++20}/allocator-inst.cc | 2 +-
 libstdc++-v3/src/c++98/Makefile.am                  | 1 -
 libstdc++-v3/src/c++98/Makefile.in                  | 6 ++----
 6 files changed, 11 insertions(+), 7 deletions(-)
 rename libstdc++-v3/src/{c++98 => c++20}/allocator-inst.cc (97%)

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 31449b5b87b..ab001048a67 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2526,6 +2526,10 @@  GLIBCXX_3.4.32 {
 GLIBCXX_3.4.33 {
     # std::basic_file<char>::native_handle()
     _ZNKSt12__basic_fileIcE13native_handleEv;
+
+    # std::allocator<[cw]>::allocate and ::deallocate for C++20
+    _ZNSaI[cw]E8allocateE[jmy];
+    _ZNSaI[cw]E10deallocateEP[cw][jmy];
 } GLIBCXX_3.4.32;
 
 # Symbols in the support library (libsupc++) have their own tag.
diff --git a/libstdc++-v3/src/c++20/Makefile.am b/libstdc++-v3/src/c++20/Makefile.am
index a24505e5141..f4a06e6de78 100644
--- a/libstdc++-v3/src/c++20/Makefile.am
+++ b/libstdc++-v3/src/c++20/Makefile.am
@@ -30,6 +30,7 @@  headers =
 if ENABLE_EXTERN_TEMPLATE
 # XTEMPLATE_FLAGS = -fno-implicit-templates
 inst_sources = \
+	allocator-inst.cc \
 	sstream-inst.cc
 else
 # XTEMPLATE_FLAGS =
diff --git a/libstdc++-v3/src/c++20/Makefile.in b/libstdc++-v3/src/c++20/Makefile.in
index 3ec8c5ce804..ec4c5eec026 100644
--- a/libstdc++-v3/src/c++20/Makefile.in
+++ b/libstdc++-v3/src/c++20/Makefile.in
@@ -122,7 +122,8 @@  CONFIG_CLEAN_VPATH_FILES =
 LTLIBRARIES = $(noinst_LTLIBRARIES)
 libc__20convenience_la_LIBADD =
 am__objects_1 = tzdb.lo
-@ENABLE_EXTERN_TEMPLATE_TRUE@am__objects_2 = sstream-inst.lo
+@ENABLE_EXTERN_TEMPLATE_TRUE@am__objects_2 = allocator-inst.lo \
+@ENABLE_EXTERN_TEMPLATE_TRUE@	sstream-inst.lo
 @GLIBCXX_HOSTED_TRUE@am_libc__20convenience_la_OBJECTS =  \
 @GLIBCXX_HOSTED_TRUE@	$(am__objects_1) $(am__objects_2)
 libc__20convenience_la_OBJECTS = $(am_libc__20convenience_la_OBJECTS)
@@ -430,6 +431,7 @@  headers =
 
 # XTEMPLATE_FLAGS = -fno-implicit-templates
 @ENABLE_EXTERN_TEMPLATE_TRUE@inst_sources = \
+@ENABLE_EXTERN_TEMPLATE_TRUE@	allocator-inst.cc \
 @ENABLE_EXTERN_TEMPLATE_TRUE@	sstream-inst.cc
 
 sources = tzdb.cc
diff --git a/libstdc++-v3/src/c++98/allocator-inst.cc b/libstdc++-v3/src/c++20/allocator-inst.cc
similarity index 97%
rename from libstdc++-v3/src/c++98/allocator-inst.cc
rename to libstdc++-v3/src/c++20/allocator-inst.cc
index 1d85eabf2bd..622df42bd84 100644
--- a/libstdc++-v3/src/c++98/allocator-inst.cc
+++ b/libstdc++-v3/src/c++20/allocator-inst.cc
@@ -26,7 +26,7 @@ 
 // ISO C++ 14882:
 //
 
-#include <memory>
+#include <bits/allocator.h>
 
 namespace std _GLIBCXX_VISIBILITY(default)
 {
diff --git a/libstdc++-v3/src/c++98/Makefile.am b/libstdc++-v3/src/c++98/Makefile.am
index d12fdf0f121..69616bbaa11 100644
--- a/libstdc++-v3/src/c++98/Makefile.am
+++ b/libstdc++-v3/src/c++98/Makefile.am
@@ -96,7 +96,6 @@  endif
 if ENABLE_EXTERN_TEMPLATE
 # XTEMPLATE_FLAGS = -fno-implicit-templates
 inst_sources = \
-	allocator-inst.cc \
 	concept-inst.cc \
 	ext-inst.cc \
 	misc-inst.cc
diff --git a/libstdc++-v3/src/c++98/Makefile.in b/libstdc++-v3/src/c++98/Makefile.in
index 95e909b1049..87d14e04fc5 100644
--- a/libstdc++-v3/src/c++98/Makefile.in
+++ b/libstdc++-v3/src/c++98/Makefile.in
@@ -130,9 +130,8 @@  am__objects_3 = $(am__objects_2) codecvt_members.lo collate_members.lo \
 	messages_members.lo monetary_members.lo numeric_members.lo \
 	time_members.lo
 am__objects_4 = c++locale.lo
-@ENABLE_EXTERN_TEMPLATE_TRUE@am__objects_5 = allocator-inst.lo \
-@ENABLE_EXTERN_TEMPLATE_TRUE@	concept-inst.lo ext-inst.lo \
-@ENABLE_EXTERN_TEMPLATE_TRUE@	misc-inst.lo
+@ENABLE_EXTERN_TEMPLATE_TRUE@am__objects_5 = concept-inst.lo \
+@ENABLE_EXTERN_TEMPLATE_TRUE@	ext-inst.lo misc-inst.lo
 am__objects_6 = parallel_settings.lo
 am__objects_7 = bitmap_allocator.lo pool_allocator.lo mt_allocator.lo \
 	codecvt.lo complex_io.lo globals_io.lo hash_tr1.lo \
@@ -478,7 +477,6 @@  host_sources_extra = \
 
 # XTEMPLATE_FLAGS = -fno-implicit-templates
 @ENABLE_EXTERN_TEMPLATE_TRUE@inst_sources = \
-@ENABLE_EXTERN_TEMPLATE_TRUE@	allocator-inst.cc \
 @ENABLE_EXTERN_TEMPLATE_TRUE@	concept-inst.cc \
 @ENABLE_EXTERN_TEMPLATE_TRUE@	ext-inst.cc \
 @ENABLE_EXTERN_TEMPLATE_TRUE@	misc-inst.cc