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