Fix gnu-versioned-namespace build

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

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

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.

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.

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

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?

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.

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