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

["François Dumont" <frs.dumont@gmail.com>]

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