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

[Patrick Palka <ppalka@redhat.com>]

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