* [PATCH] Fix gnu-versioned-namespace build @ 2019-12-11 7:29 François Dumont 2019-12-11 11:16 ` Jonathan Wakely 2019-12-11 16:48 ` Jonathan Wakely 0 siblings, 2 replies; 17+ messages in thread From: François Dumont @ 2019-12-11 7:29 UTC (permalink / raw) To: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 561 bytes --] I plan to commit this tomorrow. Note that rather than just adding the missing _GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous namespace usage outside std namespace. Let me know if it was intentional.    * src/c++11/random.cc: Add _GLIBCXX_BEGIN_NAMESPACE_VERSION and    _GLIBCXX_END_NAMESPACE_VERSION. Move anonymous namespace outside std    namespace. Tested under Linux x86_64 normal/debug/versioned namespace modes. There are still tests failing in versioned namespace, more patches to come. François [-- Attachment #2: versioned_namespace_build.patch --] [-- Type: text/x-patch, Size: 1163 bytes --] diff --git a/libstdc++-v3/src/c++11/random.cc b/libstdc++-v3/src/c++11/random.cc index 10fbe1dc4c4..d4ebc9556ab 100644 --- a/libstdc++-v3/src/c++11/random.cc +++ b/libstdc++-v3/src/c++11/random.cc @@ -73,8 +73,6 @@ # define USE_MT19937 1 #endif -namespace std _GLIBCXX_VISIBILITY(default) -{ namespace { #if USE_RDRAND @@ -124,6 +122,10 @@ namespace std _GLIBCXX_VISIBILITY(default) #endif } +namespace std _GLIBCXX_VISIBILITY(default) +{ +_GLIBCXX_BEGIN_NAMESPACE_VERSION + void random_device::_M_init(const std::string& token) { @@ -286,7 +288,7 @@ namespace std _GLIBCXX_VISIBILITY(default) _M_mt.seed(seed); #else // Convert old default token "mt19937" or numeric seed tokens to "default". - if (token == "mt19937" || isdigit((unsigned char)token[0])) + if (token == "mt19937" || std::isdigit((unsigned char)token[0])) _M_init("default"); else _M_init(token); @@ -407,5 +409,7 @@ namespace std _GLIBCXX_VISIBILITY(default) 0x9d2c5680UL, 15, 0xefc60000UL, 18, 1812433253UL>; #endif // USE_MT19937 -} + +_GLIBCXX_END_NAMESPACE_VERSION +} // namespace std #endif // _GLIBCXX_USE_C99_STDINT_TR1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2019-12-11 7:29 [PATCH] Fix gnu-versioned-namespace build François Dumont @ 2019-12-11 11:16 ` Jonathan Wakely 2019-12-11 11:22 ` Jonathan Wakely 2019-12-11 20:23 ` [PATCH] Fix gnu-versioned-namespace build François Dumont 2019-12-11 16:48 ` Jonathan Wakely 1 sibling, 2 replies; 17+ messages in thread From: Jonathan Wakely @ 2019-12-11 11:16 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 11/12/19 08:29 +0100, François Dumont wrote: >I plan to commit this tomorrow. > >Note that rather than just adding the missing >_GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous namespace >usage outside std namespace. Let me know if it was intentional. It was intentional, why move it? Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces are inline, so std::random_device already refers to std::__8::random_device when the original declaration was in the versioned namespace. The only fix needed here seems to be qualifying std::isdigit (and strictly-speaking we should also include <cctype> to declare that). > * src/c++11/random.cc: Add _GLIBCXX_BEGIN_NAMESPACE_VERSION and > _GLIBCXX_END_NAMESPACE_VERSION. Move anonymous namespace outside std > namespace. > >Tested under Linux x86_64 normal/debug/versioned namespace modes. > >There are still tests failing in versioned namespace, more patches to come. > >François > >diff --git a/libstdc++-v3/src/c++11/random.cc b/libstdc++-v3/src/c++11/random.cc >index 10fbe1dc4c4..d4ebc9556ab 100644 >--- a/libstdc++-v3/src/c++11/random.cc >+++ b/libstdc++-v3/src/c++11/random.cc >@@ -73,8 +73,6 @@ > # define USE_MT19937 1 > #endif > >-namespace std _GLIBCXX_VISIBILITY(default) >-{ > namespace > { > #if USE_RDRAND >@@ -124,6 +122,10 @@ namespace std _GLIBCXX_VISIBILITY(default) > #endif > } > >+namespace std _GLIBCXX_VISIBILITY(default) >+{ >+_GLIBCXX_BEGIN_NAMESPACE_VERSION >+ > void > random_device::_M_init(const std::string& token) > { >@@ -286,7 +288,7 @@ namespace std _GLIBCXX_VISIBILITY(default) > _M_mt.seed(seed); > #else > // Convert old default token "mt19937" or numeric seed tokens to "default". >- if (token == "mt19937" || isdigit((unsigned char)token[0])) >+ if (token == "mt19937" || std::isdigit((unsigned char)token[0])) > _M_init("default"); > else > _M_init(token); >@@ -407,5 +409,7 @@ namespace std _GLIBCXX_VISIBILITY(default) > 0x9d2c5680UL, 15, > 0xefc60000UL, 18, 1812433253UL>; > #endif // USE_MT19937 >-} >+ >+_GLIBCXX_END_NAMESPACE_VERSION >+} // namespace std > #endif // _GLIBCXX_USE_C99_STDINT_TR1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2019-12-11 11:16 ` Jonathan Wakely @ 2019-12-11 11:22 ` Jonathan Wakely 2019-12-11 20:26 ` [PATCH] Fix gnu-versioned-namespace tr1 declaration François Dumont 2019-12-11 20:23 ` [PATCH] Fix gnu-versioned-namespace build François Dumont 1 sibling, 1 reply; 17+ messages in thread From: Jonathan Wakely @ 2019-12-11 11:22 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 11/12/19 11:16 +0000, Jonathan Wakely wrote: >On 11/12/19 08:29 +0100, François Dumont wrote: >>I plan to commit this tomorrow. >> >>Note that rather than just adding the missing >>_GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous >>namespace usage outside std namespace. Let me know if it was >>intentional. > >It was intentional, why move it? > >Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces >are inline, so std::random_device already refers to >std::__8::random_device when the original declaration was in the >versioned namespace. > >The only fix needed here seems to be qualifying std::isdigit (and >strictly-speaking we should also include <cctype> to declare that). I was curious why that qualification is needed. Th problem is that <ctype.h> is being indirectly included by some other header, and so is <locale>, so the declarations visible are ::isdigit(int) and std::__8::isdigit<CharT>(CharT, const locale&). Even after including <cctype> we still can't call it unqualified, because <cctype> doesn't use the versioned namespace: namespace std { using ::isalnum; using ::isalpha; using ::iscntrl; using ::isdigit; In any case, I think the correct fix is to #include <cctype> and add the std:: qualification. There should be no need to change any namespace nesting. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Fix gnu-versioned-namespace tr1 declaration 2019-12-11 11:22 ` Jonathan Wakely @ 2019-12-11 20:26 ` François Dumont 2019-12-11 21:04 ` Jonathan Wakely 0 siblings, 1 reply; 17+ messages in thread From: François Dumont @ 2019-12-11 20:26 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 6167 bytes --] On 12/11/19 12:22 PM, Jonathan Wakely wrote: > On 11/12/19 11:16 +0000, Jonathan Wakely wrote: >> On 11/12/19 08:29 +0100, François Dumont wrote: >>> I plan to commit this tomorrow. >>> >>> Note that rather than just adding the missing >>> _GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous >>> namespace usage outside std namespace. Let me know if it was >>> intentional. >> >> It was intentional, why move it? >> >> Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces >> are inline, so std::random_device already refers to >> std::__8::random_device when the original declaration was in the >> versioned namespace. >> >> The only fix needed here seems to be qualifying std::isdigit (and >> strictly-speaking we should also include <cctype> to declare that). > > I was curious why that qualification is needed. Th problem is that > <ctype.h> is being indirectly included by some other header, and so is > <locale>, so the declarations visible are ::isdigit(int) and > std::__8::isdigit<CharT>(CharT, const locale&). Even after including > <cctype> we still can't call it unqualified, because <cctype> doesn't > use the versioned namespace: Yes, this is the other patch I wanted to propose. Make sure that tr1 namespace is always defined consistently with the version namespace. For instance 17_intro/headers/c++2011/parallel_mode.cc is failing at the moment with: /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/tr1/gamma.tcc:292: error: reference to 'tr1' is ambiguous In file included from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/tr1/random:45,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/random_number.h:36,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/partition.h:38,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/quicksort.h:36,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/sort.h:48,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/algo.h:45,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/algorithm:37,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/algorithm:80,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/stdc++.h:65,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/extc++.h:32,                 from /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/17_intro/headers/c++2011/parallel_mode.cc:24: /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/tr1/type_traits:40: note: candidates are: 'namespace std::__8::tr1 { }' In file included from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/types.h:37,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/parallel.h:38,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/base.h:40,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/parallel/algobase.h:40,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:2071,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/char_traits.h:39,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/ios:40,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/istream:38,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/sstream:38,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/complex:45,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/ccomplex:39,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/stdc++.h:54,                 from /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/extc++.h:32,                 from /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/17_intro/headers/c++2011/parallel_mode.cc:24: /home/fdt/dev/gcc/build_versioned_ns/x86_64-pc-linux-gnu/libstdc++-v3/include/tr1/cstdint:61: note:                'namespace std::tr1 { }' Tested under Linux x86_64 normal and versioned namespace. Ok to commit ? François [-- Attachment #2: versioned_tr1_namespace.patch --] [-- Type: text/x-patch, Size: 3732 bytes --] diff --git a/libstdc++-v3/include/tr1/cctype b/libstdc++-v3/include/tr1/cctype index ce994066188..b35cd04f0db 100644 --- a/libstdc++-v3/include/tr1/cctype +++ b/libstdc++-v3/include/tr1/cctype @@ -38,10 +38,14 @@ namespace std _GLIBCXX_VISIBILITY(default) { +_GLIBCXX_BEGIN_NAMESPACE_VERSION + namespace tr1 { using ::isblank; } + +_GLIBCXX_END_NAMESPACE_VERSION } #endif diff --git a/libstdc++-v3/include/tr1/cfenv b/libstdc++-v3/include/tr1/cfenv index a058888978f..97de7542f5b 100644 --- a/libstdc++-v3/include/tr1/cfenv +++ b/libstdc++-v3/include/tr1/cfenv @@ -53,6 +53,8 @@ namespace std _GLIBCXX_VISIBILITY(default) { +_GLIBCXX_BEGIN_NAMESPACE_VERSION + namespace tr1 { // types @@ -74,6 +76,8 @@ namespace tr1 using ::fesetenv; using ::feupdateenv; } + +_GLIBCXX_END_NAMESPACE_VERSION } #endif // _GLIBCXX_USE_C99_FENV_TR1 diff --git a/libstdc++-v3/include/tr1/cinttypes b/libstdc++-v3/include/tr1/cinttypes index e665e188289..1c08166efdc 100644 --- a/libstdc++-v3/include/tr1/cinttypes +++ b/libstdc++-v3/include/tr1/cinttypes @@ -50,6 +50,8 @@ namespace std _GLIBCXX_VISIBILITY(default) { +_GLIBCXX_BEGIN_NAMESPACE_VERSION + namespace tr1 { // types @@ -77,6 +79,8 @@ namespace tr1 using ::wcstoumax; #endif } + +_GLIBCXX_END_NAMESPACE_VERSION } #endif // _GLIBCXX_USE_C99_INTTYPES_TR1 diff --git a/libstdc++-v3/include/tr1/cstdint b/libstdc++-v3/include/tr1/cstdint index 0597d19fb1b..3211a9690f8 100644 --- a/libstdc++-v3/include/tr1/cstdint +++ b/libstdc++-v3/include/tr1/cstdint @@ -58,6 +58,7 @@ namespace std _GLIBCXX_VISIBILITY(default) { +_GLIBCXX_BEGIN_NAMESPACE_VERSION namespace tr1 { using ::int8_t; @@ -96,6 +97,7 @@ namespace tr1 using ::uintmax_t; using ::uintptr_t; } +_GLIBCXX_END_NAMESPACE_VERSION } #endif // _GLIBCXX_USE_C99_STDINT_TR1 diff --git a/libstdc++-v3/include/tr1/cstdio b/libstdc++-v3/include/tr1/cstdio index 7d72e58b5d4..f8a9182bcaa 100644 --- a/libstdc++-v3/include/tr1/cstdio +++ b/libstdc++-v3/include/tr1/cstdio @@ -37,6 +37,8 @@ namespace std _GLIBCXX_VISIBILITY(default) { +_GLIBCXX_BEGIN_NAMESPACE_VERSION + namespace tr1 { using std::snprintf; @@ -46,6 +48,8 @@ namespace tr1 using std::vscanf; using std::vsscanf; } + +_GLIBCXX_END_NAMESPACE_VERSION } #endif diff --git a/libstdc++-v3/include/tr1/cstdlib b/libstdc++-v3/include/tr1/cstdlib index a8259575ba2..0271e884436 100644 --- a/libstdc++-v3/include/tr1/cstdlib +++ b/libstdc++-v3/include/tr1/cstdlib @@ -39,6 +39,8 @@ namespace std _GLIBCXX_VISIBILITY(default) { +_GLIBCXX_BEGIN_NAMESPACE_VERSION + namespace tr1 { #if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC @@ -63,6 +65,8 @@ namespace tr1 using std::div; #endif } + +_GLIBCXX_END_NAMESPACE_VERSION } #endif // _GLIBCXX_USE_C99_STDLIB diff --git a/libstdc++-v3/include/tr1/cwchar b/libstdc++-v3/include/tr1/cwchar index 0d45ca79933..d2517f665f8 100644 --- a/libstdc++-v3/include/tr1/cwchar +++ b/libstdc++-v3/include/tr1/cwchar @@ -37,6 +37,8 @@ namespace std _GLIBCXX_VISIBILITY(default) { +_GLIBCXX_BEGIN_NAMESPACE_VERSION + namespace tr1 { #if _GLIBCXX_HAVE_WCSTOF @@ -58,6 +60,8 @@ namespace tr1 using std::wcstoull; #endif } + +_GLIBCXX_END_NAMESPACE_VERSION } #endif // _GLIBCXX_USE_WCHAR_T diff --git a/libstdc++-v3/include/tr1/cwctype b/libstdc++-v3/include/tr1/cwctype index b5f2c2f07fc..5d343a11c6b 100644 --- a/libstdc++-v3/include/tr1/cwctype +++ b/libstdc++-v3/include/tr1/cwctype @@ -37,12 +37,16 @@ namespace std _GLIBCXX_VISIBILITY(default) { +_GLIBCXX_BEGIN_NAMESPACE_VERSION + namespace tr1 { #if _GLIBCXX_HAVE_ISWBLANK using std::iswblank; #endif } + +_GLIBCXX_END_NAMESPACE_VERSION } #endif // _GLIBCXX_USE_WCHAR_T ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace tr1 declaration 2019-12-11 20:26 ` [PATCH] Fix gnu-versioned-namespace tr1 declaration François Dumont @ 2019-12-11 21:04 ` Jonathan Wakely 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Wakely @ 2019-12-11 21:04 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 11/12/19 21:25 +0100, François Dumont wrote: >On 12/11/19 12:22 PM, Jonathan Wakely wrote: >>On 11/12/19 11:16 +0000, Jonathan Wakely wrote: >>>On 11/12/19 08:29 +0100, François Dumont wrote: >>>>I plan to commit this tomorrow. >>>> >>>>Note that rather than just adding the missing >>>>_GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous >>>>namespace usage outside std namespace. Let me know if it was >>>>intentional. >>> >>>It was intentional, why move it? >>> >>>Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces >>>are inline, so std::random_device already refers to >>>std::__8::random_device when the original declaration was in the >>>versioned namespace. >>> >>>The only fix needed here seems to be qualifying std::isdigit (and >>>strictly-speaking we should also include <cctype> to declare that). >> >>I was curious why that qualification is needed. Th problem is that >><ctype.h> is being indirectly included by some other header, and so is >><locale>, so the declarations visible are ::isdigit(int) and >>std::__8::isdigit<CharT>(CharT, const locale&). Even after including >><cctype> we still can't call it unqualified, because <cctype> doesn't >>use the versioned namespace: > >Yes, this is the other patch I wanted to propose. Make sure that tr1 >namespace is always defined consistently with the version namespace. > >For instance 17_intro/headers/c++2011/parallel_mode.cc is failing at >the moment with: In the Venn diagram for TR1 and Parallel Mode the intersection is labelled "nobody cares" :-) The patch is OK for trunk though. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2019-12-11 11:16 ` Jonathan Wakely 2019-12-11 11:22 ` Jonathan Wakely @ 2019-12-11 20:23 ` François Dumont 2019-12-11 20:44 ` Jonathan Wakely 1 sibling, 1 reply; 17+ messages in thread From: François Dumont @ 2019-12-11 20:23 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 3056 bytes --] On 12/11/19 12:16 PM, Jonathan Wakely wrote: > On 11/12/19 08:29 +0100, François Dumont wrote: >> I plan to commit this tomorrow. >> >> Note that rather than just adding the missing >> _GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous >> namespace usage outside std namespace. Let me know if it was >> intentional. > > It was intentional, why move it? I just don't get the intention so I proposed to move it. But there are indeed other usages of this pattern in other src files. Note that in src/c++11/debug.cc I am using anonymous namespace at global scope, is that wrong ? > > Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces > are inline, so std::random_device already refers to > std::__8::random_device when the original declaration was in the > versioned namespace. Ok. I must confess I wasn't clear about this but looking at other src files, at least in src/c++11, was showing that it is done almost always this way, I guess we could cleanup those files. > > The only fix needed here seems to be qualifying std::isdigit (and > strictly-speaking we should also include <cctype> to declare that). > Like in attached patch ? I am including it unconditionnaly with other C wrapping headers like <cstdio>, is that fine ? At least it builds fine. > >>    * src/c++11/random.cc: Add _GLIBCXX_BEGIN_NAMESPACE_VERSION and >>    _GLIBCXX_END_NAMESPACE_VERSION. Move anonymous namespace outside std >>    namespace. >> >> Tested under Linux x86_64 normal/debug/versioned namespace modes. >> >> There are still tests failing in versioned namespace, more patches to >> come. >> >> François >> > >> diff --git a/libstdc++-v3/src/c++11/random.cc >> b/libstdc++-v3/src/c++11/random.cc >> index 10fbe1dc4c4..d4ebc9556ab 100644 >> --- a/libstdc++-v3/src/c++11/random.cc >> +++ b/libstdc++-v3/src/c++11/random.cc >> @@ -73,8 +73,6 @@ >> # define USE_MT19937 1 >> #endif >> >> -namespace std _GLIBCXX_VISIBILITY(default) >> -{ >> namespace >> { >> #if USE_RDRAND >> @@ -124,6 +122,10 @@ namespace std _GLIBCXX_VISIBILITY(default) >> #endif >> } >> >> +namespace std _GLIBCXX_VISIBILITY(default) >> +{ >> +_GLIBCXX_BEGIN_NAMESPACE_VERSION >> + >>  void >>  random_device::_M_init(const std::string& token) >>  { >> @@ -286,7 +288,7 @@ namespace std _GLIBCXX_VISIBILITY(default) >>    _M_mt.seed(seed); >> #else >>    // Convert old default token "mt19937" or numeric seed tokens to >> "default". >> -   if (token == "mt19937" || isdigit((unsigned char)token[0])) >> +   if (token == "mt19937" || std::isdigit((unsigned char)token[0])) >>      _M_init("default"); >>    else >>      _M_init(token); >> @@ -407,5 +409,7 @@ namespace std _GLIBCXX_VISIBILITY(default) >>    0x9d2c5680UL, 15, >>    0xefc60000UL, 18, 1812433253UL>; >> #endif // USE_MT19937 >> -} >> + >> +_GLIBCXX_END_NAMESPACE_VERSION >> +} // namespace std >> #endif // _GLIBCXX_USE_C99_STDINT_TR1 > > . [-- Attachment #2: versioned_namespace_build.patch --] [-- Type: text/x-patch, Size: 1164 bytes --] diff --git a/libstdc++-v3/src/c++11/random.cc b/libstdc++-v3/src/c++11/random.cc index 10fbe1dc4c4..d4ebc9556ab 100644 --- a/libstdc++-v3/src/c++11/random.cc +++ b/libstdc++-v3/src/c++11/random.cc @@ -73,8 +73,6 @@ # define USE_MT19937 1 #endif -namespace std _GLIBCXX_VISIBILITY(default) -{ namespace { #if USE_RDRAND @@ -124,6 +122,10 @@ namespace std _GLIBCXX_VISIBILITY(default) #endif } +namespace std _GLIBCXX_VISIBILITY(default) +{ +_GLIBCXX_BEGIN_NAMESPACE_VERSION + void random_device::_M_init(const std::string& token) { @@ -286,7 +288,7 @@ namespace std _GLIBCXX_VISIBILITY(default) _M_mt.seed(seed); #else // Convert old default token "mt19937" or numeric seed tokens to "default". - if (token == "mt19937" || isdigit((unsigned char)token[0])) + if (token == "mt19937" || std::isdigit((unsigned char)token[0])) _M_init("default"); else _M_init(token); @@ -407,5 +409,7 @@ namespace std _GLIBCXX_VISIBILITY(default) 0x9d2c5680UL, 15, 0xefc60000UL, 18, 1812433253UL>; #endif // USE_MT19937 -} + +_GLIBCXX_END_NAMESPACE_VERSION +} // namespace std #endif // _GLIBCXX_USE_C99_STDINT_TR1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2019-12-11 20:23 ` [PATCH] Fix gnu-versioned-namespace build François Dumont @ 2019-12-11 20:44 ` Jonathan Wakely 2019-12-11 21:28 ` François Dumont 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Wakely @ 2019-12-11 20:44 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 11/12/19 21:23 +0100, François Dumont wrote: >On 12/11/19 12:16 PM, Jonathan Wakely wrote: >>On 11/12/19 08:29 +0100, François Dumont wrote: >>>I plan to commit this tomorrow. >>> >>>Note that rather than just adding the missing >>>_GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous >>>namespace usage outside std namespace. Let me know if it was >>>intentional. >> >>It was intentional, why move it? > >I just don't get the intention so I proposed to move it. But there are >indeed other usages of this pattern in other src files. > >Note that in src/c++11/debug.cc I am using anonymous namespace at >global scope, is that wrong ? It's certainly more fragile, so arguably it's wrong, yes. Consider: // some libc function in a system header we don't control: extern "C" void __foo(); // libstdc++ code in a .cc file: namespace { void foo() { } } namespace std { void bar() { foo(); } } This fails to compile, because the name foo is ambiguous in the global scope. We don't control the libc headers, so we don't know all the names they might declare at global scope. If you don't put the unnamed namespace at global scope, the problem simply doesn't exist: namespace std { namespace { void foo() { } } void bar() { foo(); } } Now it doesn't matter what names libc puts in the global scope, because we're never looking for foo in the global scope. It's obviously better to add our declarations to our own namespace that we control, not to the global namespace (and an unnamed namespace at global scope effectively adds the names to the global namespace). >> >>Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces >>are inline, so std::random_device already refers to >>std::__8::random_device when the original declaration was in the >>versioned namespace. > >Ok. I must confess I wasn't clear about this but looking at other src >files, at least in src/c++11, was showing that it is done almost >always this way, I guess we could cleanup those files. > >> >>The only fix needed here seems to be qualifying std::isdigit (and >>strictly-speaking we should also include <cctype> to declare that). >> >Like in attached patch ? Did you attach the wrong patch? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2019-12-11 20:44 ` Jonathan Wakely @ 2019-12-11 21:28 ` François Dumont 2019-12-11 21:33 ` Jonathan Wakely 0 siblings, 1 reply; 17+ messages in thread From: François Dumont @ 2019-12-11 21:28 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 2455 bytes --] On 12/11/19 9:44 PM, Jonathan Wakely wrote: > On 11/12/19 21:23 +0100, François Dumont wrote: >> On 12/11/19 12:16 PM, Jonathan Wakely wrote: >>> On 11/12/19 08:29 +0100, François Dumont wrote: >>>> I plan to commit this tomorrow. >>>> >>>> Note that rather than just adding the missing >>>> _GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous >>>> namespace usage outside std namespace. Let me know if it was >>>> intentional. >>> >>> It was intentional, why move it? >> >> I just don't get the intention so I proposed to move it. But there >> are indeed other usages of this pattern in other src files. >> >> Note that in src/c++11/debug.cc I am using anonymous namespace at >> global scope, is that wrong ? > > It's certainly more fragile, so arguably it's wrong, yes. > > Consider: > > // some libc function in a system header we don't control: > extern "C" void __foo(); > > // libstdc++ code in a .cc file: > namespace > { >  void foo() { } > } > namespace std > { >  void bar() { foo(); } > } > > This fails to compile, because the name foo is ambiguous in the global > scope. We don't control the libc headers, so we don't know all the > names they might declare at global scope. > > If you don't put the unnamed namespace at global scope, the problem > simply doesn't exist: > > namespace std > { >  namespace >  { >   void foo() { } >  } > >  void bar() { foo(); } > } > > Now it doesn't matter what names libc puts in the global scope, > because we're never looking for foo in the global scope. > > It's obviously better to add our declarations to our own namespace > that we control, not to the global namespace (and an unnamed namespace > at global scope effectively adds the names to the global namespace). > >>> >>> Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces >>> are inline, so std::random_device already refers to >>> std::__8::random_device when the original declaration was in the >>> versioned namespace. >> >> Ok. I must confess I wasn't clear about this but looking at other src >> files, at least in src/c++11, was showing that it is done almost >> always this way, I guess we could cleanup those files. >> >>> >>> The only fix needed here seems to be qualifying std::isdigit (and >>> strictly-speaking we should also include <cctype> to declare that). >>> >> Like in attached patch ? > > Did you attach the wrong patch? > > Indeed, here is the correct one. [-- Attachment #2: versioned_namespace_build.patch --] [-- Type: text/x-patch, Size: 757 bytes --] diff --git a/libstdc++-v3/src/c++11/random.cc b/libstdc++-v3/src/c++11/random.cc index 10fbe1dc4c4..04edc582b69 100644 --- a/libstdc++-v3/src/c++11/random.cc +++ b/libstdc++-v3/src/c++11/random.cc @@ -41,6 +41,7 @@ #include <cerrno> #include <cstdio> +#include <cctype> // For std::isdigit. #if defined _GLIBCXX_HAVE_UNISTD_H && defined _GLIBCXX_HAVE_FCNTL_H # include <unistd.h> @@ -286,7 +287,7 @@ namespace std _GLIBCXX_VISIBILITY(default) _M_mt.seed(seed); #else // Convert old default token "mt19937" or numeric seed tokens to "default". - if (token == "mt19937" || isdigit((unsigned char)token[0])) + if (token == "mt19937" || std::isdigit((unsigned char)token[0])) _M_init("default"); else _M_init(token); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2019-12-11 21:28 ` François Dumont @ 2019-12-11 21:33 ` Jonathan Wakely 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Wakely @ 2019-12-11 21:33 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 11/12/19 22:28 +0100, François Dumont wrote: >On 12/11/19 9:44 PM, Jonathan Wakely wrote: >>On 11/12/19 21:23 +0100, François Dumont wrote: >>>On 12/11/19 12:16 PM, Jonathan Wakely wrote: >>>>On 11/12/19 08:29 +0100, François Dumont wrote: >>>>>I plan to commit this tomorrow. >>>>> >>>>>Note that rather than just adding the missing >>>>>_GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous >>>>>namespace usage outside std namespace. Let me know if it was >>>>>intentional. >>>> >>>>It was intentional, why move it? >>> >>>I just don't get the intention so I proposed to move it. But there >>>are indeed other usages of this pattern in other src files. >>> >>>Note that in src/c++11/debug.cc I am using anonymous namespace at >>>global scope, is that wrong ? >> >>It's certainly more fragile, so arguably it's wrong, yes. >> >>Consider: >> >>// some libc function in a system header we don't control: >>extern "C" void __foo(); >> >>// libstdc++ code in a .cc file: >>namespace >>{ >> void foo() { } >>} >>namespace std >>{ >> void bar() { foo(); } >>} >> >>This fails to compile, because the name foo is ambiguous in the global >>scope. We don't control the libc headers, so we don't know all the >>names they might declare at global scope. >> >>If you don't put the unnamed namespace at global scope, the problem >>simply doesn't exist: >> >>namespace std >>{ >> namespace >> { >> void foo() { } >> } >> >> void bar() { foo(); } >>} >> >>Now it doesn't matter what names libc puts in the global scope, >>because we're never looking for foo in the global scope. >> >>It's obviously better to add our declarations to our own namespace >>that we control, not to the global namespace (and an unnamed namespace >>at global scope effectively adds the names to the global namespace). >> >>>> >>>>Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces >>>>are inline, so std::random_device already refers to >>>>std::__8::random_device when the original declaration was in the >>>>versioned namespace. >>> >>>Ok. I must confess I wasn't clear about this but looking at other >>>src files, at least in src/c++11, was showing that it is done >>>almost always this way, I guess we could cleanup those files. >>> >>>> >>>>The only fix needed here seems to be qualifying std::isdigit (and >>>>strictly-speaking we should also include <cctype> to declare that). >>>> >>>Like in attached patch ? >> >>Did you attach the wrong patch? >> >> >Indeed, here is the correct one. OK for trunk (including <cctype> unconditionally is fine). Thanks. > >diff --git a/libstdc++-v3/src/c++11/random.cc b/libstdc++-v3/src/c++11/random.cc >index 10fbe1dc4c4..04edc582b69 100644 >--- a/libstdc++-v3/src/c++11/random.cc >+++ b/libstdc++-v3/src/c++11/random.cc >@@ -41,6 +41,7 @@ > > #include <cerrno> > #include <cstdio> >+#include <cctype> // For std::isdigit. > > #if defined _GLIBCXX_HAVE_UNISTD_H && defined _GLIBCXX_HAVE_FCNTL_H > # include <unistd.h> >@@ -286,7 +287,7 @@ namespace std _GLIBCXX_VISIBILITY(default) > _M_mt.seed(seed); > #else > // Convert old default token "mt19937" or numeric seed tokens to "default". >- if (token == "mt19937" || isdigit((unsigned char)token[0])) >+ if (token == "mt19937" || std::isdigit((unsigned char)token[0])) > _M_init("default"); > else > _M_init(token); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2019-12-11 7:29 [PATCH] Fix gnu-versioned-namespace build François Dumont 2019-12-11 11:16 ` Jonathan Wakely @ 2019-12-11 16:48 ` Jonathan Wakely 1 sibling, 0 replies; 17+ messages in thread From: Jonathan Wakely @ 2019-12-11 16:48 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 11/12/19 08:29 +0100, François Dumont wrote: >I plan to commit this tomorrow. > >Note that rather than just adding the missing >_GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous namespace >usage outside std namespace. Let me know if it was intentional. > > * src/c++11/random.cc: Add _GLIBCXX_BEGIN_NAMESPACE_VERSION and > _GLIBCXX_END_NAMESPACE_VERSION. Move anonymous namespace outside std > namespace. > >Tested under Linux x86_64 normal/debug/versioned namespace modes. > >There are still tests failing in versioned namespace, more patches to come. One of them fails like this: /home/jwakely/src/gcc/buildso8/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/locale_classes.tcc:134: undefined reference to `_ZNSt3__87codecvtIDsDu11__mbstate_tE2idE' For some reason some of the char8_t instantiations are not exported from the libstdc++.so.8.0.0 shared library: 00000000000aaad0 t _ZNKSt3__87codecvtIDiDu11__mbstate_tE10do_unshiftERS1_PDuS4_RS4_ 00000000000aaa80 t _ZNKSt3__87codecvtIDiDu11__mbstate_tE11do_encodingEv 00000000000aaaa0 t _ZNKSt3__87codecvtIDiDu11__mbstate_tE13do_max_lengthEv 00000000000aaa90 t _ZNKSt3__87codecvtIDiDu11__mbstate_tE16do_always_noconvEv 00000000000ab870 t _ZNKSt3__87codecvtIDiDu11__mbstate_tE5do_inERS1_PKDuS5_RS5_PDiS7_RS7_ 00000000000abc50 t _ZNKSt3__87codecvtIDiDu11__mbstate_tE6do_outERS1_PKDiS5_RS5_PDuS7_RS7_ 00000000000ab810 t _ZNKSt3__87codecvtIDiDu11__mbstate_tE9do_lengthERS1_PKDuS5_m 00000000000aaac0 t _ZNKSt3__87codecvtIDsDu11__mbstate_tE10do_unshiftERS1_PDuS4_RS4_ 00000000000aaa80 t _ZNKSt3__87codecvtIDsDu11__mbstate_tE11do_encodingEv 00000000000aaaa0 t _ZNKSt3__87codecvtIDsDu11__mbstate_tE13do_max_lengthEv 00000000000aaa90 t _ZNKSt3__87codecvtIDsDu11__mbstate_tE16do_always_noconvEv 00000000000abd40 t _ZNKSt3__87codecvtIDsDu11__mbstate_tE5do_inERS1_PKDuS5_RS5_PDsS7_RS7_ 00000000000ab990 t _ZNKSt3__87codecvtIDsDu11__mbstate_tE6do_outERS1_PKDsS5_RS5_PDuS7_RS7_ 00000000000ab900 t _ZNKSt3__87codecvtIDsDu11__mbstate_tE9do_lengthERS1_PKDuS5_m 0000000000157a00 b _ZNSt3__87codecvtIDiDu11__mbstate_tE2idE 00000000000ab270 t _ZNSt3__87codecvtIDiDu11__mbstate_tED0Ev 00000000000ab130 t _ZNSt3__87codecvtIDiDu11__mbstate_tED1Ev 00000000000ab130 t _ZNSt3__87codecvtIDiDu11__mbstate_tED2Ev 0000000000157a08 b _ZNSt3__87codecvtIDsDu11__mbstate_tE2idE 00000000000ab250 t _ZNSt3__87codecvtIDsDu11__mbstate_tED0Ev 00000000000ab110 t _ZNSt3__87codecvtIDsDu11__mbstate_tED1Ev 00000000000ab110 t _ZNSt3__87codecvtIDsDu11__mbstate_tED2Ev We should be able to add them manually to the config/abi/pre/gnu-versioned-namespace.ver script, but that shouldn't be necessary. I think the problem is that the binutils demangler doesn't know how to demangle char8_t symbols, so this fails to match them: GLIBCXX_8.0 { global: # Names inside the 'extern' block are demangled names. extern "C++" { std::*; std::__8::*; std::random_device::* }; I'm using binutils-2.31.1-36.fc30.x86_64 but it looks like binutils-2.32-30.fc31.x86_64 also can't demangle those symbols. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Fix gnu-versioned-namespace build @ 2020-10-30 12:59 François Dumont 2020-10-30 13:23 ` Jonathan Wakely 0 siblings, 1 reply; 17+ messages in thread From: François Dumont @ 2020-10-30 12:59 UTC (permalink / raw) To: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 685 bytes --] The gnu-versioned-namespace build is broken. The fix in charconv/floating_from_chars.cc is quite trivial. I am not so sure about the fix in sstream-inst.cc. libstdc++: Fix gnu-version-namespace buid libstdc++-v3/ChangeLog * include/std/charconv (from_chars): Define only if _GLIBCXX_USE_CXX11_ABI. * src/c++17/floating_from_chars.cc (from_chars): Likewise. * src/c++20/sstream-inst.cc: Limit instantiations if _GLIBCXX_USE_CXX11_ABI. I build the lib with this patch. I am now running tests. Ok to commit if tests are successful ? François [-- Attachment #2: version-ns.patch --] [-- Type: text/x-patch, Size: 1632 bytes --] diff --git a/libstdc++-v3/include/std/charconv b/libstdc++-v3/include/std/charconv index dd1ebdf8322..90142659a0c 100644 --- a/libstdc++-v3/include/std/charconv +++ b/libstdc++-v3/include/std/charconv @@ -688,7 +688,7 @@ namespace __detail operator^=(chars_format& __lhs, chars_format __rhs) noexcept { return __lhs = __lhs ^ __rhs; } -#if _GLIBCXX_HAVE_USELOCALE +#if _GLIBCXX_HAVE_USELOCALE && _GLIBCXX_USE_CXX11_ABI from_chars_result from_chars(const char* __first, const char* __last, float& __value, chars_format __fmt = chars_format::general) noexcept; diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc b/libstdc++-v3/src/c++17/floating_from_chars.cc index d52c0a937b9..36685c2d6f4 100644 --- a/libstdc++-v3/src/c++17/floating_from_chars.cc +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc @@ -41,7 +41,7 @@ # include <xlocale.h> #endif -#if _GLIBCXX_HAVE_USELOCALE +#if _GLIBCXX_HAVE_USELOCALE && _GLIBCXX_USE_CXX11_ABI namespace std _GLIBCXX_VISIBILITY(default) { _GLIBCXX_BEGIN_NAMESPACE_VERSION diff --git a/libstdc++-v3/src/c++20/sstream-inst.cc b/libstdc++-v3/src/c++20/sstream-inst.cc index e04560d28c8..8c6840115c5 100644 --- a/libstdc++-v3/src/c++20/sstream-inst.cc +++ b/libstdc++-v3/src/c++20/sstream-inst.cc @@ -29,6 +29,7 @@ // Instantiations in this file are only for the new SSO std::string ABI #include <sstream> +#if _GLIBCXX_USE_CXX11_ABI namespace std _GLIBCXX_VISIBILITY(default) { _GLIBCXX_BEGIN_NAMESPACE_VERSION @@ -106,3 +107,5 @@ basic_stringstream<wchar_t>::view() const noexcept; _GLIBCXX_END_NAMESPACE_VERSION } + +#endif //_GLIBCXX_USE_CXX11_ABI ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2020-10-30 12:59 François Dumont @ 2020-10-30 13:23 ` Jonathan Wakely 2020-10-30 13:37 ` Jonathan Wakely 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Wakely @ 2020-10-30 13:23 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 30/10/20 13:59 +0100, François Dumont via Libstdc++ wrote: >The gnu-versioned-namespace build is broken. > >The fix in charconv/floating_from_chars.cc is quite trivial. I am not >so sure about the fix in sstream-inst.cc. The change for src/c++20/sstream-inst.cc is OK to commit. It would probably be better to not build that file at all if the cxx11 ABI is not supported at all, but then the src/c++20 directory would be empty and I'm not sure if that would work. So just making the file empty is fine. The change for from_chars is not OK. With your change the <charconv> header doesn't declare those functions if included by a file using the old ABI. That's wrong, they should be declared unconditionally. I see two ways to fix it. Either make the declarations in the header depend on ! _GLIBCXX_INLINE_VERSION (so they're disabled for gnu-versioned namespace) or fix the code in floating_from_chars to not use a pmr::memory_resource for allocation in the versioned namespace build. Please commit the sstream-inst.cc part only, thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2020-10-30 13:23 ` Jonathan Wakely @ 2020-10-30 13:37 ` Jonathan Wakely 2020-10-30 14:11 ` Jonathan Wakely 2020-10-30 17:51 ` François Dumont 0 siblings, 2 replies; 17+ messages in thread From: Jonathan Wakely @ 2020-10-30 13:37 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 1382 bytes --] On 30/10/20 13:23 +0000, Jonathan Wakely wrote: >On 30/10/20 13:59 +0100, François Dumont via Libstdc++ wrote: >>The gnu-versioned-namespace build is broken. >> >>The fix in charconv/floating_from_chars.cc is quite trivial. I am >>not so sure about the fix in sstream-inst.cc. > >The change for src/c++20/sstream-inst.cc is OK to commit. It would >probably be better to not build that file at all if the cxx11 ABI is >not supported at all, but then the src/c++20 directory would be empty >and I'm not sure if that would work. So just making the file empty is >fine. > >The change for from_chars is not OK. With your change the <charconv> >header doesn't declare those functions if included by a file using the >old ABI. That's wrong, they should be declared unconditionally. > >I see two ways to fix it. Either make the declarations in the header >depend on ! _GLIBCXX_INLINE_VERSION (so they're disabled for >gnu-versioned namespace) or fix the code in floating_from_chars to not >use a pmr::memory_resource for allocation in the versioned namespace >build. Here's a patch for the second way. A third way to fix it would be to make basic_string work with C++ allocators, so that pmr::string is usable for the gnu-versioned namespace. And the fourth would be to switch the versioned namespace to use the new ABI unconditionally, instead of using the old ABI unconditionally. [-- Attachment #2: patch.txt --] [-- Type: text/x-patch, Size: 3092 bytes --] diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc b/libstdc++-v3/src/c++17/floating_from_chars.cc index d52c0a937b9f..c279809cf35d 100644 --- a/libstdc++-v3/src/c++17/floating_from_chars.cc +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc @@ -27,6 +27,9 @@ // 23.2.9 Primitive numeric input conversion [utility.from.chars] // +// Prefer to use std::pmr::string if possible, which requires the cxx11 ABI. +#define _GLIBCXX_USE_CXX11_ABI 1 + #include <charconv> #include <string> #include <memory_resource> @@ -87,6 +90,12 @@ namespace void* m_ptr = nullptr; }; +#if _GLIBCXX_USE_CXX11_ABI + using buffered_string = std::pmr::string; +#else + using buffered_string = std::string; +#endif + inline bool valid_fmt(chars_format fmt) { return fmt != chars_format{} @@ -130,7 +139,7 @@ namespace // Returns a nullptr if a valid pattern is not present. const char* pattern(const char* const first, const char* last, - chars_format& fmt, pmr::string& buf) + chars_format& fmt, buffered_string& buf) { // fmt has the value of one of the enumerators of chars_format. __glibcxx_assert(valid_fmt(fmt)); @@ -359,6 +368,22 @@ namespace return result; } +#if ! _GLIBCXX_USE_CXX11_ABI + inline bool + reserve_string(std::string& s) noexcept + { + __try + { + s.reserve(buffer_resource::guaranteed_capacity()); + } + __catch (const std::bad_alloc&) + { + return false; + } + return true; + } +#endif + } // namespace // FIXME: This should be reimplemented so it doesn't use strtod and newlocale. @@ -369,10 +394,16 @@ from_chars_result from_chars(const char* first, const char* last, float& value, chars_format fmt) noexcept { + errc ec = errc::invalid_argument; +#if _GLIBCXX_USE_CXX11_ABI buffer_resource mr; pmr::string buf(&mr); +#else + string buf; + if (!reserve_string(buf)) + return make_result(first, 0, {}, ec); +#endif size_t len = 0; - errc ec = errc::invalid_argument; __try { if (const char* pat = pattern(first, last, fmt, buf)) [[likely]] @@ -389,10 +420,16 @@ from_chars_result from_chars(const char* first, const char* last, double& value, chars_format fmt) noexcept { + errc ec = errc::invalid_argument; +#if _GLIBCXX_USE_CXX11_ABI buffer_resource mr; pmr::string buf(&mr); +#else + string buf; + if (!reserve_string(buf)) + return make_result(first, 0, {}, ec); +#endif size_t len = 0; - errc ec = errc::invalid_argument; __try { if (const char* pat = pattern(first, last, fmt, buf)) [[likely]] @@ -409,10 +446,16 @@ from_chars_result from_chars(const char* first, const char* last, long double& value, chars_format fmt) noexcept { + errc ec = errc::invalid_argument; +#if _GLIBCXX_USE_CXX11_ABI buffer_resource mr; pmr::string buf(&mr); +#else + string buf; + if (!reserve_string(buf)) + return make_result(first, 0, {}, ec); +#endif size_t len = 0; - errc ec = errc::invalid_argument; __try { if (const char* pat = pattern(first, last, fmt, buf)) [[likely]] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2020-10-30 13:37 ` Jonathan Wakely @ 2020-10-30 14:11 ` Jonathan Wakely 2020-10-30 17:51 ` François Dumont 1 sibling, 0 replies; 17+ messages in thread From: Jonathan Wakely @ 2020-10-30 14:11 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 30/10/20 13:38 +0000, Jonathan Wakely wrote: >On 30/10/20 13:23 +0000, Jonathan Wakely wrote: >>On 30/10/20 13:59 +0100, François Dumont via Libstdc++ wrote: >>>The gnu-versioned-namespace build is broken. >>> >>>The fix in charconv/floating_from_chars.cc is quite trivial. I am >>>not so sure about the fix in sstream-inst.cc. >> >>The change for src/c++20/sstream-inst.cc is OK to commit. It would >>probably be better to not build that file at all if the cxx11 ABI is >>not supported at all, but then the src/c++20 directory would be empty >>and I'm not sure if that would work. So just making the file empty is >>fine. >> >>The change for from_chars is not OK. With your change the <charconv> >>header doesn't declare those functions if included by a file using the >>old ABI. That's wrong, they should be declared unconditionally. >> >>I see two ways to fix it. Either make the declarations in the header >>depend on ! _GLIBCXX_INLINE_VERSION (so they're disabled for >>gnu-versioned namespace) or fix the code in floating_from_chars to not >>use a pmr::memory_resource for allocation in the versioned namespace >>build. > >Here's a patch for the second way. > >A third way to fix it would be to make basic_string work with C++ Oops, I meant "make the old basic_string work with C++11 allocators" of course. >allocators, so that pmr::string is usable for the gnu-versioned >namespace. > >And the fourth would be to switch the versioned namespace to use the >new ABI unconditionally, instead of using the old ABI unconditionally. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2020-10-30 13:37 ` Jonathan Wakely 2020-10-30 14:11 ` Jonathan Wakely @ 2020-10-30 17:51 ` François Dumont 2020-10-30 18:48 ` Jonathan Wakely 1 sibling, 1 reply; 17+ messages in thread From: François Dumont @ 2020-10-30 17:51 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches On 30/10/20 2:37 pm, Jonathan Wakely wrote: > On 30/10/20 13:23 +0000, Jonathan Wakely wrote: >> On 30/10/20 13:59 +0100, François Dumont via Libstdc++ wrote: >>> The gnu-versioned-namespace build is broken. >>> >>> The fix in charconv/floating_from_chars.cc is quite trivial. I am >>> not so sure about the fix in sstream-inst.cc. >> >> The change for src/c++20/sstream-inst.cc is OK to commit. It would >> probably be better to not build that file at all if the cxx11 ABI is >> not supported at all, but then the src/c++20 directory would be empty >> and I'm not sure if that would work. So just making the file empty is >> fine. >> >> The change for from_chars is not OK. With your change the <charconv> >> header doesn't declare those functions if included by a file using the >> old ABI. That's wrong, they should be declared unconditionally. >> >> I see two ways to fix it. Either make the declarations in the header >> depend on ! _GLIBCXX_INLINE_VERSION (so they're disabled for >> gnu-versioned namespace) or fix the code in floating_from_chars to not >> use a pmr::memory_resource for allocation in the versioned namespace >> build. > > Here's a patch for the second way. > > A third way to fix it would be to make basic_string work with C++ > allocators, so that pmr::string is usable for the gnu-versioned > namespace. > > And the fourth would be to switch the versioned namespace to use the > new ABI unconditionally, instead of using the old ABI unconditionally. > > Can I commit this one once tested then ? I'll try to put the fourth way in place however. Thanks, François ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2020-10-30 17:51 ` François Dumont @ 2020-10-30 18:48 ` Jonathan Wakely 2020-10-31 17:16 ` François Dumont 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Wakely @ 2020-10-30 18:48 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 30/10/20 18:51 +0100, François Dumont wrote: >On 30/10/20 2:37 pm, Jonathan Wakely wrote: >>On 30/10/20 13:23 +0000, Jonathan Wakely wrote: >>>On 30/10/20 13:59 +0100, François Dumont via Libstdc++ wrote: >>>>The gnu-versioned-namespace build is broken. >>>> >>>>The fix in charconv/floating_from_chars.cc is quite trivial. I >>>>am not so sure about the fix in sstream-inst.cc. >>> >>>The change for src/c++20/sstream-inst.cc is OK to commit. It would >>>probably be better to not build that file at all if the cxx11 ABI is >>>not supported at all, but then the src/c++20 directory would be empty >>>and I'm not sure if that would work. So just making the file empty is >>>fine. >>> >>>The change for from_chars is not OK. With your change the <charconv> >>>header doesn't declare those functions if included by a file using the >>>old ABI. That's wrong, they should be declared unconditionally. >>> >>>I see two ways to fix it. Either make the declarations in the header >>>depend on ! _GLIBCXX_INLINE_VERSION (so they're disabled for >>>gnu-versioned namespace) or fix the code in floating_from_chars to not >>>use a pmr::memory_resource for allocation in the versioned namespace >>>build. >> >>Here's a patch for the second way. >> >>A third way to fix it would be to make basic_string work with C++ >>allocators, so that pmr::string is usable for the gnu-versioned >>namespace. >> >>And the fourth would be to switch the versioned namespace to use the >>new ABI unconditionally, instead of using the old ABI unconditionally. >> >> >Can I commit this one once tested then ? Yes please. >I'll try to put the fourth way in place however. N.B. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83077 tracks that. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Fix gnu-versioned-namespace build 2020-10-30 18:48 ` Jonathan Wakely @ 2020-10-31 17:16 ` François Dumont 0 siblings, 0 replies; 17+ messages in thread From: François Dumont @ 2020-10-31 17:16 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 2076 bytes --] After completion of test in normal and versioned namespace mode I committed the attached patch. Note that there are still failures in versioned namespace mode do to missing symbol exports for which I'll propose an other patch. On 30/10/20 7:48 pm, Jonathan Wakely wrote: > On 30/10/20 18:51 +0100, François Dumont wrote: >> On 30/10/20 2:37 pm, Jonathan Wakely wrote: >>> On 30/10/20 13:23 +0000, Jonathan Wakely wrote: >>>> On 30/10/20 13:59 +0100, François Dumont via Libstdc++ wrote: >>>>> The gnu-versioned-namespace build is broken. >>>>> >>>>> The fix in charconv/floating_from_chars.cc is quite trivial. I am >>>>> not so sure about the fix in sstream-inst.cc. >>>> >>>> The change for src/c++20/sstream-inst.cc is OK to commit. It would >>>> probably be better to not build that file at all if the cxx11 ABI is >>>> not supported at all, but then the src/c++20 directory would be empty >>>> and I'm not sure if that would work. So just making the file empty is >>>> fine. >>>> >>>> The change for from_chars is not OK. With your change the <charconv> >>>> header doesn't declare those functions if included by a file using the >>>> old ABI. That's wrong, they should be declared unconditionally. >>>> >>>> I see two ways to fix it. Either make the declarations in the header >>>> depend on ! _GLIBCXX_INLINE_VERSION (so they're disabled for >>>> gnu-versioned namespace) or fix the code in floating_from_chars to not >>>> use a pmr::memory_resource for allocation in the versioned namespace >>>> build. >>> >>> Here's a patch for the second way. >>> >>> A third way to fix it would be to make basic_string work with C++ >>> allocators, so that pmr::string is usable for the gnu-versioned >>> namespace. >>> >>> And the fourth would be to switch the versioned namespace to use the >>> new ABI unconditionally, instead of using the old ABI unconditionally. >>> >>> >> Can I commit this one once tested then ? > > Yes please. > >> I'll try to put the fourth way in place however. > > N.B. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83077 tracks that. > [-- Attachment #2: version_ns.patch --] [-- Type: text/x-patch, Size: 3677 bytes --] diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc b/libstdc++-v3/src/c++17/floating_from_chars.cc index d52c0a937b9..c279809cf35 100644 --- a/libstdc++-v3/src/c++17/floating_from_chars.cc +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc @@ -27,6 +27,9 @@ // 23.2.9 Primitive numeric input conversion [utility.from.chars] // +// Prefer to use std::pmr::string if possible, which requires the cxx11 ABI. +#define _GLIBCXX_USE_CXX11_ABI 1 + #include <charconv> #include <string> #include <memory_resource> @@ -87,6 +90,12 @@ namespace void* m_ptr = nullptr; }; +#if _GLIBCXX_USE_CXX11_ABI + using buffered_string = std::pmr::string; +#else + using buffered_string = std::string; +#endif + inline bool valid_fmt(chars_format fmt) { return fmt != chars_format{} @@ -130,7 +139,7 @@ namespace // Returns a nullptr if a valid pattern is not present. const char* pattern(const char* const first, const char* last, - chars_format& fmt, pmr::string& buf) + chars_format& fmt, buffered_string& buf) { // fmt has the value of one of the enumerators of chars_format. __glibcxx_assert(valid_fmt(fmt)); @@ -359,6 +368,22 @@ namespace return result; } +#if ! _GLIBCXX_USE_CXX11_ABI + inline bool + reserve_string(std::string& s) noexcept + { + __try + { + s.reserve(buffer_resource::guaranteed_capacity()); + } + __catch (const std::bad_alloc&) + { + return false; + } + return true; + } +#endif + } // namespace // FIXME: This should be reimplemented so it doesn't use strtod and newlocale. @@ -369,10 +394,16 @@ from_chars_result from_chars(const char* first, const char* last, float& value, chars_format fmt) noexcept { + errc ec = errc::invalid_argument; +#if _GLIBCXX_USE_CXX11_ABI buffer_resource mr; pmr::string buf(&mr); +#else + string buf; + if (!reserve_string(buf)) + return make_result(first, 0, {}, ec); +#endif size_t len = 0; - errc ec = errc::invalid_argument; __try { if (const char* pat = pattern(first, last, fmt, buf)) [[likely]] @@ -389,10 +420,16 @@ from_chars_result from_chars(const char* first, const char* last, double& value, chars_format fmt) noexcept { + errc ec = errc::invalid_argument; +#if _GLIBCXX_USE_CXX11_ABI buffer_resource mr; pmr::string buf(&mr); +#else + string buf; + if (!reserve_string(buf)) + return make_result(first, 0, {}, ec); +#endif size_t len = 0; - errc ec = errc::invalid_argument; __try { if (const char* pat = pattern(first, last, fmt, buf)) [[likely]] @@ -409,10 +446,16 @@ from_chars_result from_chars(const char* first, const char* last, long double& value, chars_format fmt) noexcept { + errc ec = errc::invalid_argument; +#if _GLIBCXX_USE_CXX11_ABI buffer_resource mr; pmr::string buf(&mr); +#else + string buf; + if (!reserve_string(buf)) + return make_result(first, 0, {}, ec); +#endif size_t len = 0; - errc ec = errc::invalid_argument; __try { if (const char* pat = pattern(first, last, fmt, buf)) [[likely]] diff --git a/libstdc++-v3/src/c++20/sstream-inst.cc b/libstdc++-v3/src/c++20/sstream-inst.cc index e04560d28c8..8c6840115c5 100644 --- a/libstdc++-v3/src/c++20/sstream-inst.cc +++ b/libstdc++-v3/src/c++20/sstream-inst.cc @@ -29,6 +29,7 @@ // Instantiations in this file are only for the new SSO std::string ABI #include <sstream> +#if _GLIBCXX_USE_CXX11_ABI namespace std _GLIBCXX_VISIBILITY(default) { _GLIBCXX_BEGIN_NAMESPACE_VERSION @@ -106,3 +107,5 @@ basic_stringstream<wchar_t>::view() const noexcept; _GLIBCXX_END_NAMESPACE_VERSION } + +#endif //_GLIBCXX_USE_CXX11_ABI ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-10-31 17:16 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-11 7:29 [PATCH] Fix gnu-versioned-namespace build François Dumont 2019-12-11 11:16 ` Jonathan Wakely 2019-12-11 11:22 ` Jonathan Wakely 2019-12-11 20:26 ` [PATCH] Fix gnu-versioned-namespace tr1 declaration François Dumont 2019-12-11 21:04 ` Jonathan Wakely 2019-12-11 20:23 ` [PATCH] Fix gnu-versioned-namespace build François Dumont 2019-12-11 20:44 ` Jonathan Wakely 2019-12-11 21:28 ` François Dumont 2019-12-11 21:33 ` Jonathan Wakely 2019-12-11 16:48 ` Jonathan Wakely 2020-10-30 12:59 François Dumont 2020-10-30 13:23 ` Jonathan Wakely 2020-10-30 13:37 ` Jonathan Wakely 2020-10-30 14:11 ` Jonathan Wakely 2020-10-30 17:51 ` François Dumont 2020-10-30 18:48 ` Jonathan Wakely 2020-10-31 17:16 ` François Dumont
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).