From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x331.google.com (mail-ot1-x331.google.com [IPv6:2607:f8b0:4864:20::331]) by sourceware.org (Postfix) with ESMTPS id B4D793856DC0 for ; Fri, 21 Oct 2022 13:08:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B4D793856DC0 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-ot1-x331.google.com with SMTP id z11-20020a05683020cb00b00661a95cf920so1774343otq.5 for ; Fri, 21 Oct 2022 06:08:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=7ezAbgsjZYMoU6hF8im8EuAR1VnrkcFERvRUnyEF+80=; b=hZDPHsvSLgfrCssLLqZqH1zNFHs6cHcrcHYuRiw47GtWZ1KodERYVPM48vpc0vFBxw GryOfyfrgmhLG7hrLKoFetHHNA026DrPLIhVQCQQBlxb1HzqxPkkdG63KI6qKJl49pi7 gk9ZtymdByaPZl3FD2cOljHA3V9reFEmLOW8f7hPuVpyQNl8AKZ7ThpmmlWWSWcZuCRs a3djAo/l8CTR+eYohhTD84WVh8hOmBOPZ210qyL3/AnDxltsIuqSkOv4wvhrvg3jKCSo 1pqDCsNbYRHw5gqDIV6gsBfBLmXXQMkpWy03evEfsbgBb/xOaRkr8EGUfMQdVDNvVETs GETQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=7ezAbgsjZYMoU6hF8im8EuAR1VnrkcFERvRUnyEF+80=; b=rw//4qkkHb0CMSaVOtG5X50j13KEtB9pWMugoSSMcJK4jWsNWz/RK6Pps8E3XcrTJK XwLZDUKQBj1IvXEWgEEEZu+3tfTRIvtXiAkPrub0ee0jeeV2RCQTAUvR/C7uQgiUdloN 6e+MMFW61xzCof3toHA/e1uJbdoC5TT7mBNOy1zAHCa5TAFa4GogdEV7BSWyo6WGztsj MR+ihzTlxOOMF587Uwc5939FAJMai2DRXvuzFfeh8c0Nx3yVo1c6FsiAZAZVdw7qIoj7 fxajvciTtvU5VpsfHIdae9KexcZ5yUoNjqM7d7qd3DXvqcUdmbTvoM/LdWIW4dj/0REs l1WA== X-Gm-Message-State: ACrzQf0CmuJWYDqYRuOpjM+UJ3I5MXpu3cf30CNQ5Ezx7iJiWE4vmggM zpqwMK9+AdOtg0N9cV3FeE/ZIQ== X-Google-Smtp-Source: AMsMyM79+GvMWIH76OsfWBydJhskvqXfYFUUzsnyzQpSG8tJbg2yrvd/oBECqVqyFGWPbzdSr4iNVg== X-Received: by 2002:a05:6830:6315:b0:661:8cab:7ab0 with SMTP id cg21-20020a056830631500b006618cab7ab0mr9513927otb.264.1666357694845; Fri, 21 Oct 2022 06:08:14 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c3:7d19:5909:4ed0:fbc6:5cfd? ([2804:1b3:a7c3:7d19:5909:4ed0:fbc6:5cfd]) by smtp.gmail.com with ESMTPSA id m4-20020a4ae3c4000000b00475d676d2d4sm3791661oov.16.2022.10.21.06.08.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 21 Oct 2022 06:08:14 -0700 (PDT) Message-ID: <2618159f-b3f2-3819-5817-e5f02490350a@linaro.org> Date: Fri, 21 Oct 2022 10:08:11 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 Subject: Re: [PATCH v5] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501) Content-Language: en-US To: Letu Ren , libc-alpha@sourceware.org Cc: fweimer@redhat.com, schwab@suse.de, joseph@codesourcery.com References: <20221006150137.18691-1-fantasquex@gmail.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <20221006150137.18691-1-fantasquex@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 06/10/22 12:01, Letu Ren via Libc-alpha wrote: > According to the specification of ISO/IEC TS 18661-1:2014, > > The strfromd, strfromf, and strfroml functions are equivalent to > snprintf(s, n, format, fp) (7.21.6.5), except the format string contains only > the character %, an optional precision that does not contain an asterisk *, and > one of the conversion specifiers a, A, e, E, f, F, g, or G, which applies to > the type (double, float, or long double) indicated by the function suffix > (rather than by a length modifier). Use of these functions with any other 20 > format string results in undefined behavior. > > strfromf will convert the arguement with type float to double first. > > According to the latest version of IEEE754 which is published in 2019, > > Conversion of a quiet NaN from a narrower format to a wider format in the same > radix, and then back to the same narrower format, should not change the quiet > NaN payload in any way except to make it canonical. > > When either an input or result is a NaN, this standard does not interpret the > sign of a NaN. However, operations on bit strings—copy, negate, abs, > copySign—specify the sign bit of a NaN result, sometimes based upon the sign > bit of a NaN operand. The logical predicates totalOrder and isSignMinus are > also affected by the sign bit of a NaN operand. For all other operations, this > standard does not specify the sign bit of a NaN result, even when there is only > one input NaN, or when the NaN is produced from an invalid operation. > > converting NAN or -NAN with type float to double doesn't need to keep > the signbit. As a result, this test case isn't mandatory. > > The problem is that according to RISC-V ISA manual in chapter 11.3 of > riscv-isa-20191213, > > Except when otherwise stated, if the result of a floating-point operation is > NaN, it is the canonical NaN. The canonical NaN has a positive sign and all > significand bits clear except the MSB, a.k.a. the quiet bit. For > single-precision floating-point, this corresponds to the pattern 0x7fc00000. > > which means that conversion -NAN from float to double won't keep the signbit. > > Since glibc ought to be consistent here between types and architectures, this > patch adds copysign to fix this problem if the string is NAN. This patch > adds two different functions under sysdeps directory to work around the > issue. > > This patch has been tested on x86_64 and riscv64. > > Resolves: BZ #29501 > > v2: Change from macros to different inline functions. > v3: Add unlikely check to isnan. > v4: Fix wrong commit message header. > v5: Fix style: add space before parentheses. > Signed-off-by: Letu Ren The fix-float-double-convert-nan.h misses the Copyright headers, otherwise LGTM. > --- > I'm not sure about copyright of the new file I created and the name of > the new function. Please give me some advice. Thanks. > > stdlib/strfrom-skeleton.c | 3 ++- > .../generic/fix-float-double-convert-nan.h | 15 +++++++++++++ > .../riscv/rvd/fix-float-double-convert-nan.h | 21 +++++++++++++++++++ > 3 files changed, 38 insertions(+), 1 deletion(-) > create mode 100644 sysdeps/generic/fix-float-double-convert-nan.h > create mode 100644 sysdeps/riscv/rvd/fix-float-double-convert-nan.h > > diff --git a/stdlib/strfrom-skeleton.c b/stdlib/strfrom-skeleton.c > index 1fba04bf6a..36e9adcad5 100644 > --- a/stdlib/strfrom-skeleton.c > +++ b/stdlib/strfrom-skeleton.c > @@ -27,6 +27,7 @@ > #include > #include > #include > +#include > > #define UCHAR_T char > #define L_(Str) Str > @@ -61,7 +62,7 @@ STRFROM (char *dest, size_t size, const char *format, FLOAT f) > because __printf_fp and __printf_fphex only accept double and long double > as the floating-point argument. */ > if (__builtin_types_compatible_p (FLOAT, float)) > - fpnum.flt = f; > + fpnum.flt = keep_sign_conversion (f); > else > fpnum.value = f; > > diff --git a/sysdeps/generic/fix-float-double-convert-nan.h b/sysdeps/generic/fix-float-double-convert-nan.h > new file mode 100644 > index 0000000000..0949dda102 > --- /dev/null > +++ b/sysdeps/generic/fix-float-double-convert-nan.h > @@ -0,0 +1,15 @@ > +/* Fix for conversion of float NAN to double. Generic version. */ > + It is missing the usual Copyright as for other fix-* files. > +#ifndef FIX_FLOAT_DOUBLE_CONVERT_NAN_H > +#define FIX_FLOAT_DOUBLE_CONVERT_NAN_H > + > +/* This function aims to work around conversions of float -NAN > + to double returning NAN instead of the correct -NAN in some > + architectures. */ > +static inline double __attribute__ ((always_inline)) > +keep_sign_conversion (float flt) > +{ > + return flt; > +} > + > +#endif > diff --git a/sysdeps/riscv/rvd/fix-float-double-convert-nan.h b/sysdeps/riscv/rvd/fix-float-double-convert-nan.h > new file mode 100644 > index 0000000000..59806052a8 > --- /dev/null > +++ b/sysdeps/riscv/rvd/fix-float-double-convert-nan.h > @@ -0,0 +1,21 @@ > +/* Fix for conversion of float NAN to double. RISC-V version. */ > + > +#ifndef FIX_FLOAT_DOUBLE_CONVERT_NAN_H > +#define FIX_FLOAT_DOUBLE_CONVERT_NAN_H > + > +#include > + > +/* RISC-V rvd instructions do not preserve the signbit of NAN > + when converting from float to double. */ > +static inline double > +keep_sign_conversion (float flt) > +{ > + if (__glibc_unlikely (isnan (flt))) > + { > + float x = copysignf (1.f, flt); > + return copysign ((double) flt, (double) x); > + } > + return flt; > +} > + > +#endif