[PATCH] Fix gnu-versioned-namespace build

[PATCH] Fix gnu-versioned-namespace build

[François Dumont]

[-- 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

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

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

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

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.

[PATCH] Fix gnu-versioned-namespace tr1 declaration

[François Dumont]

[-- 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

Re: [PATCH] Fix gnu-versioned-namespace tr1 declaration

[Jonathan Wakely]

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.

Re: [PATCH] Fix gnu-versioned-namespace build

[François Dumont]

[-- 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

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

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?

Re: [PATCH] Fix gnu-versioned-namespace build

[François Dumont]

[-- 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);

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

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);

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

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.

[PATCH] Fix gnu-versioned-namespace build

[François Dumont]

[-- 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

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

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.

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

[-- 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]]

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

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.

Re: [PATCH] Fix gnu-versioned-namespace build

[François Dumont]

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

Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely]

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.

Re: [PATCH] Fix gnu-versioned-namespace build

[François Dumont]

[-- 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