* [PATCH] libstdc++: Remove extern "C" from Ryu sources @ 2021-05-11 14:24 Patrick Palka 2021-05-11 15:16 ` Patrick Palka 0 siblings, 1 reply; 6+ messages in thread From: Patrick Palka @ 2021-05-11 14:24 UTC (permalink / raw) To: gcc-patches; +Cc: libstdc++, Patrick Palka floating_to_chars.cc includes the Ryu sources into an anonymous namespace as a convenient way to give all its symbols internal linkage. But an entity declared extern "C" always has external linkage, even from within an anonymous namespace, so this trick doesn't work in the presence of extern "C", and it causes the Ryu function generic_to_chars to be visible from libstdc++.a. This patch removes the only use of extern "C" from our local copy of Ryu, along with some declarations for never-defined functions that GCC now warns about. Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars is not visible from libstdc++.a. Does this look OK for trunk and the 11 branch? libstdc++-v3/ChangeLog: * src/c++17/ryu/LOCAL_PATCHES: Update. * src/c++17/ryu/ryu_generic_128.h: Remove extern "C". Remove declarations for never-defined functions. --- libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES | 1 + libstdc++-v3/src/c++17/ryu/ryu_generic_128.h | 21 +++----------------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES index 51e504cb6ea..72ffad9662d 100644 --- a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES +++ b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES @@ -1,2 +1,3 @@ r11-6248 r11-7636 +r12-XXX diff --git a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h index 2afbf274e11..6d988ab01eb 100644 --- a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h +++ b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h @@ -18,9 +18,9 @@ #define RYU_GENERIC_128_H -#ifdef __cplusplus -extern "C" { -#endif +// NOTE: These symbols are declared extern "C" upstream, but we don't want that +// because it'd override the internal linkage of the anonymous namespace into +// which this header is included. // This is a generic 128-bit implementation of float to shortest conversion // using the Ryu algorithm. It can handle any IEEE-compatible floating-point @@ -42,18 +42,6 @@ struct floating_decimal_128 { bool sign; }; -struct floating_decimal_128 float_to_fd128(float f); -struct floating_decimal_128 double_to_fd128(double d); - -// According to wikipedia (https://en.wikipedia.org/wiki/Long_double), this likely only works on -// x86 with specific compilers (clang?). May need an ifdef. -struct floating_decimal_128 long_double_to_fd128(long double d); - -// Converts the given binary floating point number to the shortest decimal floating point number -// that still accurately represents it. -struct floating_decimal_128 generic_binary_to_decimal( - const uint128_t bits, const uint32_t mantissaBits, const uint32_t exponentBits, const bool explicitLeadingBit); - // Converts the given decimal floating point number to a string, writing to result, and returning // the number characters written. Does not terminate the buffer with a 0. In the worst case, this // function can write up to 53 characters. @@ -63,8 +51,5 @@ struct floating_decimal_128 generic_binary_to_decimal( // = 1 + 39 + 1 + 1 + 1 + 10 = 53 int generic_to_chars(const struct floating_decimal_128 v, char* const result); -#ifdef __cplusplus -} -#endif #endif // RYU_GENERIC_128_H -- 2.31.1.527.g2d677e5b15 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libstdc++: Remove extern "C" from Ryu sources 2021-05-11 14:24 [PATCH] libstdc++: Remove extern "C" from Ryu sources Patrick Palka @ 2021-05-11 15:16 ` Patrick Palka 2021-05-11 16:51 ` Jonathan Wakely 0 siblings, 1 reply; 6+ messages in thread From: Patrick Palka @ 2021-05-11 15:16 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, libstdc++ On Tue, 11 May 2021, Patrick Palka wrote: > floating_to_chars.cc includes the Ryu sources into an anonymous > namespace as a convenient way to give all its symbols internal linkage. > But an entity declared extern "C" always has external linkage, even > from within an anonymous namespace, so this trick doesn't work in the > presence of extern "C", and it causes the Ryu function generic_to_chars > to be visible from libstdc++.a. > > This patch removes the only use of extern "C" from our local copy of > Ryu, along with some declarations for never-defined functions that GCC > now warns about. > > Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars > is not visible from libstdc++.a. Does this look OK for trunk and the 11 > branch? Now with a testcase: -- >8 -- Subject: [PATCH] libstdc++: Remove extern "C" from Ryu sources floating_to_chars.cc includes the Ryu sources into an anonymous namespace as a convenient way to give all its symbols internal linkage. But an entity declared extern "C" always has external linkage, even from within an anonymous namespace, so this trick doesn't work in the presence of extern "C", and it causes the Ryu function generic_to_chars to be visible from libstdc++.a. This patch removes the only use of extern "C" from our local copy of Ryu along with some declarations for never-defined functions that GCC now warns about. Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars is not visible from libstdc++.a. Does this look OK for trunk and the 11 branch? libstdc++-v3/ChangeLog: * src/c++17/ryu/LOCAL_PATCHES: Update. * src/c++17/ryu/ryu_generic_128.h: Remove extern "C". Remove declarations for never-defined functions. * testsuite/20_util/to_chars/4.cc: New test. --- libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES | 1 + libstdc++-v3/src/c++17/ryu/ryu_generic_128.h | 21 ++---------- libstdc++-v3/testsuite/20_util/to_chars/4.cc | 36 ++++++++++++++++++++ 3 files changed, 40 insertions(+), 18 deletions(-) create mode 100644 libstdc++-v3/testsuite/20_util/to_chars/4.cc diff --git a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES index 51e504cb6ea..72ffad9662d 100644 --- a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES +++ b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES @@ -1,2 +1,3 @@ r11-6248 r11-7636 +r12-XXX diff --git a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h index 2afbf274e11..6d988ab01eb 100644 --- a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h +++ b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h @@ -18,9 +18,9 @@ #define RYU_GENERIC_128_H -#ifdef __cplusplus -extern "C" { -#endif +// NOTE: These symbols are declared extern "C" upstream, but we don't want that +// because it'd override the internal linkage of the anonymous namespace into +// which this header is included. // This is a generic 128-bit implementation of float to shortest conversion // using the Ryu algorithm. It can handle any IEEE-compatible floating-point @@ -42,18 +42,6 @@ struct floating_decimal_128 { bool sign; }; -struct floating_decimal_128 float_to_fd128(float f); -struct floating_decimal_128 double_to_fd128(double d); - -// According to wikipedia (https://en.wikipedia.org/wiki/Long_double), this likely only works on -// x86 with specific compilers (clang?). May need an ifdef. -struct floating_decimal_128 long_double_to_fd128(long double d); - -// Converts the given binary floating point number to the shortest decimal floating point number -// that still accurately represents it. -struct floating_decimal_128 generic_binary_to_decimal( - const uint128_t bits, const uint32_t mantissaBits, const uint32_t exponentBits, const bool explicitLeadingBit); - // Converts the given decimal floating point number to a string, writing to result, and returning // the number characters written. Does not terminate the buffer with a 0. In the worst case, this // function can write up to 53 characters. @@ -63,8 +51,5 @@ struct floating_decimal_128 generic_binary_to_decimal( // = 1 + 39 + 1 + 1 + 1 + 10 = 53 int generic_to_chars(const struct floating_decimal_128 v, char* const result); -#ifdef __cplusplus -} -#endif #endif // RYU_GENERIC_128_H diff --git a/libstdc++-v3/testsuite/20_util/to_chars/4.cc b/libstdc++-v3/testsuite/20_util/to_chars/4.cc new file mode 100644 index 00000000000..96f6e5d010c --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/to_chars/4.cc @@ -0,0 +1,36 @@ +// Copyright (C) 2021 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// { dg-do link { target c++17 } } +// { dg-require-effective-target ieee-floats } +// { dg-require-static-libstdcxx } +// { dg-additional-options "-static-libstdc++" } + +// Verify the Ryu symbol generic_to_chars doesn't inadvertently leak into +// libstdc++.a. If it did, this test would fail at link time with a multiple +// definition error. + +#include <charconv> + +extern "C" void generic_to_chars (void) { __builtin_abort(); } + +int +main() +{ + char x[64]; + std::to_chars(x, x+64, 42.L, std::chars_format::scientific); +} -- 2.31.1.527.g2d677e5b15 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libstdc++: Remove extern "C" from Ryu sources 2021-05-11 15:16 ` Patrick Palka @ 2021-05-11 16:51 ` Jonathan Wakely 2021-05-11 17:04 ` Patrick Palka 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Wakely @ 2021-05-11 16:51 UTC (permalink / raw) To: Patrick Palka; +Cc: libstdc++, gcc-patches On 11/05/21 11:16 -0400, Patrick Palka via Libstdc++ wrote: >On Tue, 11 May 2021, Patrick Palka wrote: > >> floating_to_chars.cc includes the Ryu sources into an anonymous >> namespace as a convenient way to give all its symbols internal linkage. >> But an entity declared extern "C" always has external linkage, even >> from within an anonymous namespace, so this trick doesn't work in the >> presence of extern "C", and it causes the Ryu function generic_to_chars >> to be visible from libstdc++.a. >> >> This patch removes the only use of extern "C" from our local copy of >> Ryu, along with some declarations for never-defined functions that GCC >> now warns about. >> >> Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars >> is not visible from libstdc++.a. Does this look OK for trunk and the 11 >> branch? > >Now with a testcase: > >-- >8 -- > >Subject: [PATCH] libstdc++: Remove extern "C" from Ryu sources > >floating_to_chars.cc includes the Ryu sources into an anonymous >namespace as a convenient way to give all its symbols internal linkage. >But an entity declared extern "C" always has external linkage, even >from within an anonymous namespace, so this trick doesn't work in the >presence of extern "C", and it causes the Ryu function generic_to_chars >to be visible from libstdc++.a. > >This patch removes the only use of extern "C" from our local copy of >Ryu along with some declarations for never-defined functions that GCC >now warns about. > >Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars >is not visible from libstdc++.a. Does this look OK for trunk and the 11 >branch? > >libstdc++-v3/ChangeLog: > > * src/c++17/ryu/LOCAL_PATCHES: Update. > * src/c++17/ryu/ryu_generic_128.h: Remove extern "C". > Remove declarations for never-defined functions. > * testsuite/20_util/to_chars/4.cc: New test. >--- > libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES | 1 + > libstdc++-v3/src/c++17/ryu/ryu_generic_128.h | 21 ++---------- > libstdc++-v3/testsuite/20_util/to_chars/4.cc | 36 ++++++++++++++++++++ > 3 files changed, 40 insertions(+), 18 deletions(-) > create mode 100644 libstdc++-v3/testsuite/20_util/to_chars/4.cc > >diff --git a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES >index 51e504cb6ea..72ffad9662d 100644 >--- a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES >+++ b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES >@@ -1,2 +1,3 @@ > r11-6248 > r11-7636 >+r12-XXX >diff --git a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h >index 2afbf274e11..6d988ab01eb 100644 >--- a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h >+++ b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h >@@ -18,9 +18,9 @@ > #define RYU_GENERIC_128_H > > >-#ifdef __cplusplus >-extern "C" { >-#endif >+// NOTE: These symbols are declared extern "C" upstream, but we don't want that >+// because it'd override the internal linkage of the anonymous namespace into >+// which this header is included. > > // This is a generic 128-bit implementation of float to shortest conversion > // using the Ryu algorithm. It can handle any IEEE-compatible floating-point >@@ -42,18 +42,6 @@ struct floating_decimal_128 { > bool sign; > }; > >-struct floating_decimal_128 float_to_fd128(float f); >-struct floating_decimal_128 double_to_fd128(double d); >- >-// According to wikipedia (https://en.wikipedia.org/wiki/Long_double), this likely only works on >-// x86 with specific compilers (clang?). May need an ifdef. >-struct floating_decimal_128 long_double_to_fd128(long double d); >- >-// Converts the given binary floating point number to the shortest decimal floating point number >-// that still accurately represents it. >-struct floating_decimal_128 generic_binary_to_decimal( >- const uint128_t bits, const uint32_t mantissaBits, const uint32_t exponentBits, const bool explicitLeadingBit); >- > // Converts the given decimal floating point number to a string, writing to result, and returning > // the number characters written. Does not terminate the buffer with a 0. In the worst case, this > // function can write up to 53 characters. >@@ -63,8 +51,5 @@ struct floating_decimal_128 generic_binary_to_decimal( > // = 1 + 39 + 1 + 1 + 1 + 10 = 53 > int generic_to_chars(const struct floating_decimal_128 v, char* const result); > >-#ifdef __cplusplus >-} >-#endif > > #endif // RYU_GENERIC_128_H >diff --git a/libstdc++-v3/testsuite/20_util/to_chars/4.cc b/libstdc++-v3/testsuite/20_util/to_chars/4.cc >new file mode 100644 >index 00000000000..96f6e5d010c >--- /dev/null >+++ b/libstdc++-v3/testsuite/20_util/to_chars/4.cc >@@ -0,0 +1,36 @@ >+// Copyright (C) 2021 Free Software Foundation, Inc. >+// >+// This file is part of the GNU ISO C++ Library. This library is free >+// software; you can redistribute it and/or modify it under the >+// terms of the GNU General Public License as published by the >+// Free Software Foundation; either version 3, or (at your option) >+// any later version. >+ >+// This library is distributed in the hope that it will be useful, >+// but WITHOUT ANY WARRANTY; without even the implied warranty of >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+// GNU General Public License for more details. >+ >+// You should have received a copy of the GNU General Public License along >+// with this library; see the file COPYING3. If not see >+// <http://www.gnu.org/licenses/>. >+ >+// { dg-do link { target c++17 } } >+// { dg-require-effective-target ieee-floats } >+// { dg-require-static-libstdcxx } >+// { dg-additional-options "-static-libstdc++" } >+ >+// Verify the Ryu symbol generic_to_chars doesn't inadvertently leak into >+// libstdc++.a. If it did, this test would fail at link time with a multiple >+// definition error. >+ >+#include <charconv> >+ >+extern "C" void generic_to_chars (void) { __builtin_abort(); } This test is "dg-do link" but that won't check whether this abort is called. Did you mean to dg-do run? >+ >+int >+main() >+{ >+ char x[64]; >+ std::to_chars(x, x+64, 42.L, std::chars_format::scientific); >+} >-- >2.31.1.527.g2d677e5b15 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libstdc++: Remove extern "C" from Ryu sources 2021-05-11 16:51 ` Jonathan Wakely @ 2021-05-11 17:04 ` Patrick Palka 2021-05-11 17:07 ` Jonathan Wakely 0 siblings, 1 reply; 6+ messages in thread From: Patrick Palka @ 2021-05-11 17:04 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Patrick Palka, libstdc++, gcc-patches On Tue, 11 May 2021, Jonathan Wakely wrote: > On 11/05/21 11:16 -0400, Patrick Palka via Libstdc++ wrote: > > On Tue, 11 May 2021, Patrick Palka wrote: > > > > > floating_to_chars.cc includes the Ryu sources into an anonymous > > > namespace as a convenient way to give all its symbols internal linkage. > > > But an entity declared extern "C" always has external linkage, even > > > from within an anonymous namespace, so this trick doesn't work in the > > > presence of extern "C", and it causes the Ryu function generic_to_chars > > > to be visible from libstdc++.a. > > > > > > This patch removes the only use of extern "C" from our local copy of > > > Ryu, along with some declarations for never-defined functions that GCC > > > now warns about. > > > > > > Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars > > > is not visible from libstdc++.a. Does this look OK for trunk and the 11 > > > branch? > > > > Now with a testcase: > > > > -- >8 -- > > > > Subject: [PATCH] libstdc++: Remove extern "C" from Ryu sources > > > > floating_to_chars.cc includes the Ryu sources into an anonymous > > namespace as a convenient way to give all its symbols internal linkage. > > But an entity declared extern "C" always has external linkage, even > > from within an anonymous namespace, so this trick doesn't work in the > > presence of extern "C", and it causes the Ryu function generic_to_chars > > to be visible from libstdc++.a. > > > > This patch removes the only use of extern "C" from our local copy of > > Ryu along with some declarations for never-defined functions that GCC > > now warns about. > > > > Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars > > is not visible from libstdc++.a. Does this look OK for trunk and the 11 > > branch? > > > > libstdc++-v3/ChangeLog: > > > > * src/c++17/ryu/LOCAL_PATCHES: Update. > > * src/c++17/ryu/ryu_generic_128.h: Remove extern "C". > > Remove declarations for never-defined functions. > > * testsuite/20_util/to_chars/4.cc: New test. > > --- > > libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES | 1 + > > libstdc++-v3/src/c++17/ryu/ryu_generic_128.h | 21 ++---------- > > libstdc++-v3/testsuite/20_util/to_chars/4.cc | 36 ++++++++++++++++++++ > > 3 files changed, 40 insertions(+), 18 deletions(-) > > create mode 100644 libstdc++-v3/testsuite/20_util/to_chars/4.cc > > > > diff --git a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES > > b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES > > index 51e504cb6ea..72ffad9662d 100644 > > --- a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES > > +++ b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES > > @@ -1,2 +1,3 @@ > > r11-6248 > > r11-7636 > > +r12-XXX > > diff --git a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h > > b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h > > index 2afbf274e11..6d988ab01eb 100644 > > --- a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h > > +++ b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h > > @@ -18,9 +18,9 @@ > > #define RYU_GENERIC_128_H > > > > > > -#ifdef __cplusplus > > -extern "C" { > > -#endif > > +// NOTE: These symbols are declared extern "C" upstream, but we don't want > > that > > +// because it'd override the internal linkage of the anonymous namespace > > into > > +// which this header is included. > > > > // This is a generic 128-bit implementation of float to shortest conversion > > // using the Ryu algorithm. It can handle any IEEE-compatible floating-point > > @@ -42,18 +42,6 @@ struct floating_decimal_128 { > > bool sign; > > }; > > > > -struct floating_decimal_128 float_to_fd128(float f); > > -struct floating_decimal_128 double_to_fd128(double d); > > - > > -// According to wikipedia (https://en.wikipedia.org/wiki/Long_double), this > > likely only works on > > -// x86 with specific compilers (clang?). May need an ifdef. > > -struct floating_decimal_128 long_double_to_fd128(long double d); > > - > > -// Converts the given binary floating point number to the shortest decimal > > floating point number > > -// that still accurately represents it. > > -struct floating_decimal_128 generic_binary_to_decimal( > > - const uint128_t bits, const uint32_t mantissaBits, const uint32_t > > exponentBits, const bool explicitLeadingBit); > > - > > // Converts the given decimal floating point number to a string, writing to > > result, and returning > > // the number characters written. Does not terminate the buffer with a 0. In > > the worst case, this > > // function can write up to 53 characters. > > @@ -63,8 +51,5 @@ struct floating_decimal_128 generic_binary_to_decimal( > > // = 1 + 39 + 1 + 1 + 1 + 10 = 53 > > int generic_to_chars(const struct floating_decimal_128 v, char* const > > result); > > > > -#ifdef __cplusplus > > -} > > -#endif > > > > #endif // RYU_GENERIC_128_H > > diff --git a/libstdc++-v3/testsuite/20_util/to_chars/4.cc > > b/libstdc++-v3/testsuite/20_util/to_chars/4.cc > > new file mode 100644 > > index 00000000000..96f6e5d010c > > --- /dev/null > > +++ b/libstdc++-v3/testsuite/20_util/to_chars/4.cc > > @@ -0,0 +1,36 @@ > > +// Copyright (C) 2021 Free Software Foundation, Inc. > > +// > > +// This file is part of the GNU ISO C++ Library. This library is free > > +// software; you can redistribute it and/or modify it under the > > +// terms of the GNU General Public License as published by the > > +// Free Software Foundation; either version 3, or (at your option) > > +// any later version. > > + > > +// This library is distributed in the hope that it will be useful, > > +// but WITHOUT ANY WARRANTY; without even the implied warranty of > > +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +// GNU General Public License for more details. > > + > > +// You should have received a copy of the GNU General Public License along > > +// with this library; see the file COPYING3. If not see > > +// <http://www.gnu.org/licenses/>. > > + > > +// { dg-do link { target c++17 } } > > +// { dg-require-effective-target ieee-floats } > > +// { dg-require-static-libstdcxx } > > +// { dg-additional-options "-static-libstdc++" } > > + > > +// Verify the Ryu symbol generic_to_chars doesn't inadvertently leak into > > +// libstdc++.a. If it did, this test would fail at link time with a > > multiple > > +// definition error. > > + > > +#include <charconv> > > + > > +extern "C" void generic_to_chars (void) { __builtin_abort(); } > > This test is "dg-do link" but that won't check whether this abort is > called. Did you mean to dg-do run? I'm not sure if the abort call is necessary since the link step already fails with a multiple definition error (without the fix) even if the function is defined with an empty body. But since Jakub included an abort call in his testcase I carried it over :) Shall I just make it dg-do run, or perhaps keep it dg-do link and make the function body empty? Either seems to do the right thing. > > > > + > > +int > > +main() > > +{ > > + char x[64]; > > + std::to_chars(x, x+64, 42.L, std::chars_format::scientific); > > +} > > -- > > 2.31.1.527.g2d677e5b15 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libstdc++: Remove extern "C" from Ryu sources 2021-05-11 17:04 ` Patrick Palka @ 2021-05-11 17:07 ` Jonathan Wakely 2021-05-11 17:12 ` Jakub Jelinek 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Wakely @ 2021-05-11 17:07 UTC (permalink / raw) To: Patrick Palka; +Cc: libstdc++, gcc-patches On 11/05/21 13:04 -0400, Patrick Palka wrote: >On Tue, 11 May 2021, Jonathan Wakely wrote: > >> On 11/05/21 11:16 -0400, Patrick Palka via Libstdc++ wrote: >> > On Tue, 11 May 2021, Patrick Palka wrote: >> > >> > > floating_to_chars.cc includes the Ryu sources into an anonymous >> > > namespace as a convenient way to give all its symbols internal linkage. >> > > But an entity declared extern "C" always has external linkage, even >> > > from within an anonymous namespace, so this trick doesn't work in the >> > > presence of extern "C", and it causes the Ryu function generic_to_chars >> > > to be visible from libstdc++.a. >> > > >> > > This patch removes the only use of extern "C" from our local copy of >> > > Ryu, along with some declarations for never-defined functions that GCC >> > > now warns about. >> > > >> > > Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars >> > > is not visible from libstdc++.a. Does this look OK for trunk and the 11 >> > > branch? >> > >> > Now with a testcase: >> > >> > -- >8 -- >> > >> > Subject: [PATCH] libstdc++: Remove extern "C" from Ryu sources >> > >> > floating_to_chars.cc includes the Ryu sources into an anonymous >> > namespace as a convenient way to give all its symbols internal linkage. >> > But an entity declared extern "C" always has external linkage, even >> > from within an anonymous namespace, so this trick doesn't work in the >> > presence of extern "C", and it causes the Ryu function generic_to_chars >> > to be visible from libstdc++.a. >> > >> > This patch removes the only use of extern "C" from our local copy of >> > Ryu along with some declarations for never-defined functions that GCC >> > now warns about. >> > >> > Tested on x86_64-pc-linux-gnu, and also verified that generic_to_chars >> > is not visible from libstdc++.a. Does this look OK for trunk and the 11 >> > branch? >> > >> > libstdc++-v3/ChangeLog: >> > >> > * src/c++17/ryu/LOCAL_PATCHES: Update. >> > * src/c++17/ryu/ryu_generic_128.h: Remove extern "C". >> > Remove declarations for never-defined functions. >> > * testsuite/20_util/to_chars/4.cc: New test. >> > --- >> > libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES | 1 + >> > libstdc++-v3/src/c++17/ryu/ryu_generic_128.h | 21 ++---------- >> > libstdc++-v3/testsuite/20_util/to_chars/4.cc | 36 ++++++++++++++++++++ >> > 3 files changed, 40 insertions(+), 18 deletions(-) >> > create mode 100644 libstdc++-v3/testsuite/20_util/to_chars/4.cc >> > >> > diff --git a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES >> > b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES >> > index 51e504cb6ea..72ffad9662d 100644 >> > --- a/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES >> > +++ b/libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES >> > @@ -1,2 +1,3 @@ >> > r11-6248 >> > r11-7636 >> > +r12-XXX >> > diff --git a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h >> > b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h >> > index 2afbf274e11..6d988ab01eb 100644 >> > --- a/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h >> > +++ b/libstdc++-v3/src/c++17/ryu/ryu_generic_128.h >> > @@ -18,9 +18,9 @@ >> > #define RYU_GENERIC_128_H >> > >> > >> > -#ifdef __cplusplus >> > -extern "C" { >> > -#endif >> > +// NOTE: These symbols are declared extern "C" upstream, but we don't want >> > that >> > +// because it'd override the internal linkage of the anonymous namespace >> > into >> > +// which this header is included. >> > >> > // This is a generic 128-bit implementation of float to shortest conversion >> > // using the Ryu algorithm. It can handle any IEEE-compatible floating-point >> > @@ -42,18 +42,6 @@ struct floating_decimal_128 { >> > bool sign; >> > }; >> > >> > -struct floating_decimal_128 float_to_fd128(float f); >> > -struct floating_decimal_128 double_to_fd128(double d); >> > - >> > -// According to wikipedia (https://en.wikipedia.org/wiki/Long_double), this >> > likely only works on >> > -// x86 with specific compilers (clang?). May need an ifdef. >> > -struct floating_decimal_128 long_double_to_fd128(long double d); >> > - >> > -// Converts the given binary floating point number to the shortest decimal >> > floating point number >> > -// that still accurately represents it. >> > -struct floating_decimal_128 generic_binary_to_decimal( >> > - const uint128_t bits, const uint32_t mantissaBits, const uint32_t >> > exponentBits, const bool explicitLeadingBit); >> > - >> > // Converts the given decimal floating point number to a string, writing to >> > result, and returning >> > // the number characters written. Does not terminate the buffer with a 0. In >> > the worst case, this >> > // function can write up to 53 characters. >> > @@ -63,8 +51,5 @@ struct floating_decimal_128 generic_binary_to_decimal( >> > // = 1 + 39 + 1 + 1 + 1 + 10 = 53 >> > int generic_to_chars(const struct floating_decimal_128 v, char* const >> > result); >> > >> > -#ifdef __cplusplus >> > -} >> > -#endif >> > >> > #endif // RYU_GENERIC_128_H >> > diff --git a/libstdc++-v3/testsuite/20_util/to_chars/4.cc >> > b/libstdc++-v3/testsuite/20_util/to_chars/4.cc >> > new file mode 100644 >> > index 00000000000..96f6e5d010c >> > --- /dev/null >> > +++ b/libstdc++-v3/testsuite/20_util/to_chars/4.cc >> > @@ -0,0 +1,36 @@ >> > +// Copyright (C) 2021 Free Software Foundation, Inc. >> > +// >> > +// This file is part of the GNU ISO C++ Library. This library is free >> > +// software; you can redistribute it and/or modify it under the >> > +// terms of the GNU General Public License as published by the >> > +// Free Software Foundation; either version 3, or (at your option) >> > +// any later version. >> > + >> > +// This library is distributed in the hope that it will be useful, >> > +// but WITHOUT ANY WARRANTY; without even the implied warranty of >> > +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > +// GNU General Public License for more details. >> > + >> > +// You should have received a copy of the GNU General Public License along >> > +// with this library; see the file COPYING3. If not see >> > +// <http://www.gnu.org/licenses/>. >> > + >> > +// { dg-do link { target c++17 } } >> > +// { dg-require-effective-target ieee-floats } >> > +// { dg-require-static-libstdcxx } >> > +// { dg-additional-options "-static-libstdc++" } >> > + >> > +// Verify the Ryu symbol generic_to_chars doesn't inadvertently leak into >> > +// libstdc++.a. If it did, this test would fail at link time with a >> > multiple >> > +// definition error. >> > + >> > +#include <charconv> >> > + >> > +extern "C" void generic_to_chars (void) { __builtin_abort(); } >> >> This test is "dg-do link" but that won't check whether this abort is >> called. Did you mean to dg-do run? > >I'm not sure if the abort call is necessary since the link step already >fails with a multiple definition error (without the fix) even if the >function is defined with an empty body. But since Jakub included an >abort call in his testcase I carried it over :) Shall I just make it >dg-do run, or perhaps keep it dg-do link and make the function body >empty? Either seems to do the right thing. OK, if it works as-is then let's leave it as a link test. I think having the abort there is likely to confuse me again in future when I forget this conversation, so let's go with an empty body. OK for trunk and gcc-11, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libstdc++: Remove extern "C" from Ryu sources 2021-05-11 17:07 ` Jonathan Wakely @ 2021-05-11 17:12 ` Jakub Jelinek 0 siblings, 0 replies; 6+ messages in thread From: Jakub Jelinek @ 2021-05-11 17:12 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Patrick Palka, libstdc++, gcc-patches On Tue, May 11, 2021 at 06:07:19PM +0100, Jonathan Wakely via Gcc-patches wrote: > > I'm not sure if the abort call is necessary since the link step already > > fails with a multiple definition error (without the fix) even if the > > function is defined with an empty body. But since Jakub included an > > abort call in his testcase I carried it over :) Shall I just make it > > dg-do run, or perhaps keep it dg-do link and make the function body > > empty? Either seems to do the right thing. > > OK, if it works as-is then let's leave it as a link test. I think > having the abort there is likely to confuse me again in future when I > forget this conversation, so let's go with an empty body. When mentioning it on IRC, I didn't think of it failing already at link time, had the mental model of binary + shared library that just exports that symbol, so kind like a small shared library containing that std::to_chars(x, x+64, 42.L, std::chars_format::scientific); in some function, linked with -shared -fpic -static-libstdc++ and then binary with that generic_to_chars function extern "C" and main calling the shared library case. In the end, both that and the dg-do link testcase should catch it fine. Jakub ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-11 17:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-11 14:24 [PATCH] libstdc++: Remove extern "C" from Ryu sources Patrick Palka 2021-05-11 15:16 ` Patrick Palka 2021-05-11 16:51 ` Jonathan Wakely 2021-05-11 17:04 ` Patrick Palka 2021-05-11 17:07 ` Jonathan Wakely 2021-05-11 17:12 ` Jakub Jelinek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).