diff mbox series

[_GLIBCXX_INLINE_VERSION] Adapt to_chars/from_chars symbols

Message ID bd8a96c6-216d-d774-8356-dad6c9150f15@gmail.com
State Superseded
Headers
Series [_GLIBCXX_INLINE_VERSION] Adapt to_chars/from_chars symbols |

Commit Message

François Dumont Nov. 28, 2022, 6:07 a.m. UTC 
  This patch is fixing those tests:

20_util/to_chars/float128_c++23.cc
std/format/formatter/requirements.cc
std/format/functions/format.cc
std/format/functions/format_to_n.cc
std/format/functions/size.cc
std/format/functions/vformat_to.cc
std/format/string.cc

Note that symbols used in <format> for __ibm128 and __iee128 are untested.

I even wonder if the normal mode ones are because I cannot find the 
symbols used in gnu.ver.


     libstdc++: [_GLIBCXX_INLINE_VERSION] Add to_chars/from_chars 
symbols export

     libstdc++-v3/ChangeLog

             * include/std/format [_GLIBCXX_INLINE_VERSION](to_chars): 
Adapt __asm symbol
             specifications.
             * config/abi/pre/gnu-versioned-namespace.ver: Add 
to_chars/from_chars symbols
             export.

Ok to commit ?

François

Comments

Jonathan Wakely Nov. 28, 2022, 10:10 a.m. UTC | #1 
On Mon, 28 Nov 2022 at 06:07, François Dumont via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

> This patch is fixing those tests:
>
> 20_util/to_chars/float128_c++23.cc
> std/format/formatter/requirements.cc
> std/format/functions/format.cc
> std/format/functions/format_to_n.cc
> std/format/functions/size.cc
> std/format/functions/vformat_to.cc
> std/format/string.cc
>
> Note that symbols used in <format> for __ibm128 and __iee128 are untested.
>
> I even wonder if the normal mode ones are because I cannot find the
> symbols used in gnu.ver.
>
>
>      libstdc++: [_GLIBCXX_INLINE_VERSION] Add to_chars/from_chars
> symbols export
>
>      libstdc++-v3/ChangeLog
>
>              * include/std/format [_GLIBCXX_INLINE_VERSION](to_chars):
> Adapt __asm symbol
>              specifications.
>              * config/abi/pre/gnu-versioned-namespace.ver: Add
> to_chars/from_chars symbols
>              export.
>
> Ok to commit ?
>
>

Why are changes needed to the linker script?

Those functions should already match the general wildcard:

    # Names inside the 'extern' block are demangled names.
    extern "C++"
    {
      std::*;
      std::__8::*;
    };
Jonathan Wakely Nov. 28, 2022, 10:21 a.m. UTC | #2 
On Mon, 28 Nov 2022 at 10:10, Jonathan Wakely <jwakely@redhat.com> wrote:

>
>
> On Mon, 28 Nov 2022 at 06:07, François Dumont via Libstdc++ <
> libstdc++@gcc.gnu.org> wrote:
>
>> This patch is fixing those tests:
>>
>> 20_util/to_chars/float128_c++23.cc
>> std/format/formatter/requirements.cc
>> std/format/functions/format.cc
>> std/format/functions/format_to_n.cc
>> std/format/functions/size.cc
>> std/format/functions/vformat_to.cc
>> std/format/string.cc
>>
>> Note that symbols used in <format> for __ibm128 and __iee128 are untested.
>>
>> I even wonder if the normal mode ones are because I cannot find the
>> symbols used in gnu.ver.
>>
>>
>>      libstdc++: [_GLIBCXX_INLINE_VERSION] Add to_chars/from_chars
>> symbols export
>>
>>      libstdc++-v3/ChangeLog
>>
>>              * include/std/format [_GLIBCXX_INLINE_VERSION](to_chars):
>> Adapt __asm symbol
>>              specifications.
>>              * config/abi/pre/gnu-versioned-namespace.ver: Add
>> to_chars/from_chars symbols
>>              export.
>>
>> Ok to commit ?
>>
>>
>
> Why are changes needed to the linker script?
>
> Those functions should already match the general wildcard:
>
>     # Names inside the 'extern' block are demangled names.
>     extern "C++"
>     {
>       std::*;
>       std::__8::*;
>     };
>
>
>

Instead of nine separate #if blocks, can we just do:

#if _GLIBCXX_INLINE_VERSION
# define _GLIBCXX_ALIAS(S) __asm("_ZNSt3__8" S)
#else
# define _GLIBCXX_ALIAS(S) __asm("_ZNSt" S)
#endif

 And then use:

  _GLIBCXX_ALIAS("8to_charsPcS_eSt12chars_format");

and finally:

#undef _GLIBCXX_ALIAS
Jonathan Wakely Nov. 28, 2022, 6:35 p.m. UTC | #3 
On Mon, 28 Nov 2022 at 06:07, François Dumont via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

> This patch is fixing those tests:
>
> 20_util/to_chars/float128_c++23.cc
> std/format/formatter/requirements.cc
> std/format/functions/format.cc
> std/format/functions/format_to_n.cc
> std/format/functions/size.cc
> std/format/functions/vformat_to.cc
> std/format/string.cc
>
> Note that symbols used in <format> for __ibm128 and __iee128 are untested.
>

We don't need to do this for those symbols, the ALT128 config is
incompatible with versioned namespace. If you're using the versioned
namespace, you don't need backwards compatibility with the old long double
ABI.
François Dumont Nov. 28, 2022, 6:43 p.m. UTC | #4 
On 28/11/22 11:21, Jonathan Wakely wrote:
>
>
> On Mon, 28 Nov 2022 at 10:10, Jonathan Wakely <jwakely@redhat.com> wrote:
>
>
>
>     On Mon, 28 Nov 2022 at 06:07, François Dumont via Libstdc++
>     <libstdc++@gcc.gnu.org <mailto:libstdc%2B%2B@gcc.gnu.org>> wrote:
>
>         This patch is fixing those tests:
>
>         20_util/to_chars/float128_c++23.cc
>         std/format/formatter/requirements.cc
>         std/format/functions/format.cc
>         std/format/functions/format_to_n.cc
>         std/format/functions/size.cc
>         std/format/functions/vformat_to.cc
>         std/format/string.cc
>
>         Note that symbols used in <format> for __ibm128 and __iee128
>         are untested.
>
>         I even wonder if the normal mode ones are because I cannot
>         find the
>         symbols used in gnu.ver.
>
>
>              libstdc++: [_GLIBCXX_INLINE_VERSION] Add to_chars/from_chars
>         symbols export
>
>              libstdc++-v3/ChangeLog
>
>                      * include/std/format
>         [_GLIBCXX_INLINE_VERSION](to_chars):
>         Adapt __asm symbol
>                      specifications.
>                      * config/abi/pre/gnu-versioned-namespace.ver: Add
>         to_chars/from_chars symbols
>                      export.
>
>         Ok to commit ?
>
>
>
>     Why are changes needed to the linker script?
>
>     Those functions should already match the general wildcard:
>
>         # Names inside the 'extern' block are demangled names.
>         extern "C++"
>         {
>           std::*;
>           std::__8::*;
>         };
>
>
>
No idear, my guess was that it has something to do with the __asm usages 
in <format> and with the commnt:

   // These overloads exist in the library, but are not declared for C++20.
   // Make them available as std::__format::to_chars.

Maybe they exist in the library but are unused so not exported unless 
specified in the linker script ?


>
> Instead of nine separate #if blocks, can we just do:
>
> #if _GLIBCXX_INLINE_VERSION
> # define _GLIBCXX_ALIAS(S) __asm("_ZNSt3__8" S)
> #else
> # define _GLIBCXX_ALIAS(S) __asm("_ZNSt" S)
> #endif
>
>  And then use:
>
>   _GLIBCXX_ALIAS("8to_charsPcS_eSt12chars_format");
>
> and finally:
>
> #undef _GLIBCXX_ALIAS
>
>
I tried and as expected it's not working because the diff in the symbol 
is not limited to the '3__8' pattern. 'chars_format' is also defined in 
versioned namespace which might perhaps explain some mangling diff.

Here is an updated patch though, I had forgotten to replace a _DF128 
with a __ieee128 in the untested part of this patch.

If you prefer to take a closer look later I'll just re-submit my patch 
to move versioned namespace mode to cxx11 abi knowing that those tests 
are already FAIL.

François
François Dumont Nov. 28, 2022, 6:45 p.m. UTC | #5 
I forgot to add the patch but as you already made another feedback I'll 
clean my patch first.


On 28/11/22 19:43, François Dumont wrote:
> On 28/11/22 11:21, Jonathan Wakely wrote:
>>
>>
>> On Mon, 28 Nov 2022 at 10:10, Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>>
>>
>>     On Mon, 28 Nov 2022 at 06:07, François Dumont via Libstdc++
>>     <libstdc++@gcc.gnu.org <mailto:libstdc%2B%2B@gcc.gnu.org>> wrote:
>>
>>         This patch is fixing those tests:
>>
>>         20_util/to_chars/float128_c++23.cc
>>         std/format/formatter/requirements.cc
>>         std/format/functions/format.cc
>>         std/format/functions/format_to_n.cc
>>         std/format/functions/size.cc
>>         std/format/functions/vformat_to.cc
>>         std/format/string.cc
>>
>>         Note that symbols used in <format> for __ibm128 and __iee128
>>         are untested.
>>
>>         I even wonder if the normal mode ones are because I cannot
>>         find the
>>         symbols used in gnu.ver.
>>
>>
>>              libstdc++: [_GLIBCXX_INLINE_VERSION] Add
>>         to_chars/from_chars
>>         symbols export
>>
>>              libstdc++-v3/ChangeLog
>>
>>                      * include/std/format
>>         [_GLIBCXX_INLINE_VERSION](to_chars):
>>         Adapt __asm symbol
>>                      specifications.
>>                      * config/abi/pre/gnu-versioned-namespace.ver: Add
>>         to_chars/from_chars symbols
>>                      export.
>>
>>         Ok to commit ?
>>
>>
>>
>>     Why are changes needed to the linker script?
>>
>>     Those functions should already match the general wildcard:
>>
>>         # Names inside the 'extern' block are demangled names.
>>         extern "C++"
>>         {
>>           std::*;
>>           std::__8::*;
>>         };
>>
>>
>>
> No idear, my guess was that it has something to do with the __asm 
> usages in <format> and with the commnt:
>
>   // These overloads exist in the library, but are not declared for C++20.
>   // Make them available as std::__format::to_chars.
>
> Maybe they exist in the library but are unused so not exported unless 
> specified in the linker script ?
>
>
>>
>> Instead of nine separate #if blocks, can we just do:
>>
>> #if _GLIBCXX_INLINE_VERSION
>> # define _GLIBCXX_ALIAS(S) __asm("_ZNSt3__8" S)
>> #else
>> # define _GLIBCXX_ALIAS(S) __asm("_ZNSt" S)
>> #endif
>>
>>  And then use:
>>
>>   _GLIBCXX_ALIAS("8to_charsPcS_eSt12chars_format");
>>
>> and finally:
>>
>> #undef _GLIBCXX_ALIAS
>>
>>
> I tried and as expected it's not working because the diff in the 
> symbol is not limited to the '3__8' pattern. 'chars_format' is also 
> defined in versioned namespace which might perhaps explain some 
> mangling diff.
>
> Here is an updated patch though, I had forgotten to replace a _DF128 
> with a __ieee128 in the untested part of this patch.
>
> If you prefer to take a closer look later I'll just re-submit my patch 
> to move versioned namespace mode to cxx11 abi knowing that those tests 
> are already FAIL.
>
> François
>
>
François Dumont Nov. 28, 2022, 8:44 p.m. UTC | #6 
On 28/11/22 19:35, Jonathan Wakely wrote:
>
>
> On Mon, 28 Nov 2022 at 06:07, François Dumont via Libstdc++ 
> <libstdc++@gcc.gnu.org <mailto:libstdc%2B%2B@gcc.gnu.org>> wrote:
>
>     This patch is fixing those tests:
>
>     20_util/to_chars/float128_c++23.cc
>     std/format/formatter/requirements.cc
>     std/format/functions/format.cc
>     std/format/functions/format_to_n.cc
>     std/format/functions/size.cc
>     std/format/functions/vformat_to.cc
>     std/format/string.cc
>
>     Note that symbols used in <format> for __ibm128 and __iee128 are
>     untested.
>
>
> We don't need to do this for those symbols, the ALT128 config is 
> incompatible with versioned namespace. If you're using the versioned 
> namespace, you don't need backwards compatibility with the old long 
> double ABI.
>
>
>
Here is the simplified patch then.

     libstdc++: [_GLIBCXX_INLINE_VERSION] Add to_chars/from_chars 
symbols export

     libstdc++-v3/ChangeLog

             * include/std/format [_GLIBCXX_INLINE_VERSION](to_chars): 
Adapt __asm symbol
             specifications.
             * config/abi/pre/gnu-versioned-namespace.ver: Add 
to_chars/from_chars symbols
             export.

Ok to commit ?

François
Jonathan Wakely Jan. 12, 2023, 12:05 p.m. UTC | #7 
On Mon, 28 Nov 2022 at 20:44, François Dumont wrote:
>
> On 28/11/22 19:35, Jonathan Wakely wrote:
>
>
>
> On Mon, 28 Nov 2022 at 06:07, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote:
>>
>> This patch is fixing those tests:
>>
>> 20_util/to_chars/float128_c++23.cc
>> std/format/formatter/requirements.cc
>> std/format/functions/format.cc
>> std/format/functions/format_to_n.cc
>> std/format/functions/size.cc
>> std/format/functions/vformat_to.cc
>> std/format/string.cc
>>
>> Note that symbols used in <format> for __ibm128 and __iee128 are untested.
>
>
> We don't need to do this for those symbols, the ALT128 config is incompatible with versioned namespace. If you're using the versioned namespace, you don't need backwards compatibility with the old long double ABI.
>
>
>
>
> Here is the simplified patch then.
>
>     libstdc++: [_GLIBCXX_INLINE_VERSION] Add to_chars/from_chars symbols export
>
>     libstdc++-v3/ChangeLog
>
>             * include/std/format [_GLIBCXX_INLINE_VERSION](to_chars): Adapt __asm symbol
>             specifications.
>             * config/abi/pre/gnu-versioned-namespace.ver: Add to_chars/from_chars symbols
>             export.
>
> Ok to commit ?

I still don't understand why the linker script change is needed, but I
can investigate that myself later.

OK for trunk, thanks.
diff mbox series

Patch

diff --git a/libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver b/libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver
index 06ccaa80a58..7fc81514808 100644
--- a/libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver
+++ b/libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver
@@ -142,6 +142,12 @@  GLIBCXX_8.0 {
     _ZN14__gnu_parallel9_Settings3getEv;
     _ZN14__gnu_parallel9_Settings3setERS0_;
 
+    # to_chars/from_chars _Float128
+    _ZNSt3__88to_charsEPcS0_DF128_;
+    _ZNSt3__88to_charsEPcS0_DF128_NS_12chars_formatE;
+    _ZNSt3__88to_charsEPcS0_DF128_NS_12chars_formatEi;
+    _ZNSt3__810from_charsEPKcS1_RDF128_NS_12chars_formatE;
+
   local:
     *;
 };
diff --git a/libstdc++-v3/include/std/format b/libstdc++-v3/include/std/format
index 23ffbdabed8..a05f3981622 100644
--- a/libstdc++-v3/include/std/format
+++ b/libstdc++-v3/include/std/format
@@ -1248,27 +1248,51 @@  namespace __format
   // Make them available as std::__format::to_chars.
   to_chars_result
   to_chars(char*, char*, __ibm128) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_e");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_e");
+#  endif
 
   to_chars_result
   to_chars(char*, char*, __ibm128, chars_format) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_eSt12chars_format");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_eNS_12chars_formatE");
+#  endif
 
   to_chars_result
   to_chars(char*, char*, __ibm128, chars_format, int) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_eSt12chars_formati");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_eNS_12chars_formatEi");
+#  endif
 #elif __cplusplus == 202002L
   to_chars_result
   to_chars(char*, char*, __ieee128) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_u9__ieee128");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_DF128_");
+#  endif
 
   to_chars_result
   to_chars(char*, char*, __ieee128, chars_format) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_u9__ieee128St12chars_format");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_u9__ieee128NS_12chars_formatE");
+#  endif
 
   to_chars_result
   to_chars(char*, char*, __ieee128, chars_format, int) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_u9__ieee128St12chars_formati");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_u9__ieee128NS_12chars_formatEi");
+#  endif
 #endif
 
 #elif defined _GLIBCXX_LDOUBLE_IS_IEEE_BINARY128
@@ -1288,15 +1312,27 @@  namespace __format
   // Make them available as std::__format::to_chars.
   to_chars_result
   to_chars(char*, char*, _Float128) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_DF128_");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_DF128_");
+#  endif
 
   to_chars_result
   to_chars(char*, char*, _Float128, chars_format) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_DF128_St12chars_format");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_DF128_NS_12chars_formatE");
+#  endif
 
   to_chars_result
   to_chars(char*, char*, _Float128, chars_format, int) noexcept
+#  if !_GLIBCXX_INLINE_VERSION
     __asm("_ZSt8to_charsPcS_DF128_St12chars_formati");
+#  else
+    __asm("_ZNSt3__88to_charsEPcS0_DF128_NS_12chars_formatEi");
+#  endif
 # endif
 #endif