From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 9FDB5388C014 for ; Tue, 11 May 2021 17:07:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9FDB5388C014 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-392-alq4Six9PBqOvEmSh66fug-1; Tue, 11 May 2021 13:07:21 -0400 X-MC-Unique: alq4Six9PBqOvEmSh66fug-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 236DB801817; Tue, 11 May 2021 17:07:21 +0000 (UTC) Received: from localhost (unknown [10.33.36.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id A2EC519C44; Tue, 11 May 2021 17:07:20 +0000 (UTC) Date: Tue, 11 May 2021 18:07:19 +0100 From: Jonathan Wakely To: Patrick Palka Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [PATCH] libstdc++: Remove extern "C" from Ryu sources Message-ID: <20210511170719.GY3008@redhat.com> References: <20210511142427.221984-1-ppalka@redhat.com> <75e0367d-4fc-4de5-c96b-fe839cf547@idea> <20210511165125.GX3008@redhat.com> <6bdc4ee6-522e-1ccf-3318-1c18e18728ca@idea> MIME-Version: 1.0 In-Reply-To: <6bdc4ee6-522e-1ccf-3318-1c18e18728ca@idea> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-15.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 May 2021 17:07:26 -0000 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 >> > +// . >> > + >> > +// { 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 >> > + >> > +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.