Re: [PATCH] libstdc++: Remove extern "C" from Ryu sources

[Jonathan Wakely <jwakely@redhat.com>]

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.