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.
