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 3CB32398B17A for ; Tue, 11 May 2021 16:51:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3CB32398B17A 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-430-DwvpDR5fMyGSN2YtiBJe-A-1; Tue, 11 May 2021 12:51:27 -0400 X-MC-Unique: DwvpDR5fMyGSN2YtiBJe-A-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 47A241008061; Tue, 11 May 2021 16:51:26 +0000 (UTC) Received: from localhost (unknown [10.33.36.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id CEDE15D6D1; Tue, 11 May 2021 16:51:25 +0000 (UTC) Date: Tue, 11 May 2021 17:51:25 +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: <20210511165125.GX3008@redhat.com> References: <20210511142427.221984-1-ppalka@redhat.com> <75e0367d-4fc-4de5-c96b-fe839cf547@idea> MIME-Version: 1.0 In-Reply-To: <75e0367d-4fc-4de5-c96b-fe839cf547@idea> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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=-14.3 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: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 May 2021 16:51:32 -0000 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? >+ >+int >+main() >+{ >+ char x[64]; >+ std::to_chars(x, x+64, 42.L, std::chars_format::scientific); >+} >-- >2.31.1.527.g2d677e5b15 >