diff mbox series

Fix gnu-versioned-namespace build

Message ID c14dba45-25e4-de0c-8f5a-75d7b094a0fb@gmail.com
State New
Headers show
Series Fix gnu-versioned-namespace build | expand

Commit Message

François Dumont Oct. 30, 2020, 12:59 p.m. UTC 
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

Comments

Jonathan Wakely Oct. 30, 2020, 1:23 p.m. UTC | #1 
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.
Jonathan Wakely Oct. 30, 2020, 1:37 p.m. UTC | #2 
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.
Jonathan Wakely Oct. 30, 2020, 2:11 p.m. UTC | #3 
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.
François Dumont Oct. 30, 2020, 5:51 p.m. UTC | #4 
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
Jonathan Wakely Oct. 30, 2020, 6:48 p.m. UTC | #5 
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.
François Dumont Oct. 31, 2020, 5:16 p.m. UTC | #6 
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.
>
diff mbox series

Patch

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