From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22591 invoked by alias); 12 Mar 2018 20:36:55 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 22296 invoked by uid 89); 12 Mar 2018 20:36:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=6087, HX-Received:sk:s124mr1, Hx-spam-relays-external:209.85.220.196, H*M:4b0f X-HELO: mail-qk0-f196.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=B1baoYk6uElyMeZhs9Alt0wIa6JXCjqoZtVBSeciF4M=; b=KhNLDDaVBfBb/eA/ije8hhc5ESpJrt8v8yhEBP9D5NBBNapH0Eh0Mpw0Iiz8VT6yR3 u8r+KbhqOr8ddog0utIZjmdWx9GlydVg7Gb8D1+ZD4UA5SG8Cy7fUJkQ8Fo6AnpIpqDO Z4s1fQCxv7Me6QabYC4Ef1MKvVsqdEY8YvzW3fRG62wNaYSRyfS+J1jFzDIb0wm6dLVX zB5h8qzjDl5C7hAiPvM3SxduPBzgEbC1waRzELZiPbiuokxcM7Up3O09b/kNPStd3WWC sexDC9asUJsE8cjYVzvugNDGMCsbkG/2d+PO2olw8/gOFlxtCeGacUWHZ07qnF+yLe00 LJoA== X-Gm-Message-State: AElRT7HhItgGlnlXMvWXYTFhxv+bT+MhssIap2aediYIK9cvogCzgA4R zufl7aUtpV1hFwfCN07vcMTshyhCb0I= X-Google-Smtp-Source: AG47ELvEqj85oHFKvEP78SCxdNIq3J+yCCUYKLQPzL8mV9dh239EG6dwhVDfKsAhJBmYlb9HVa0/bQ== X-Received: by 10.55.94.130 with SMTP id s124mr13240359qkb.316.1520887010038; Mon, 12 Mar 2018 13:36:50 -0700 (PDT) Subject: Re: [PATCH 1/9] Use STRFMON_LDBL_IS_DBL instead of __ldbl_is_dbl. To: libc-alpha@sourceware.org References: <20180307193205.4751-1-zackw@panix.com> <20180307193205.4751-2-zackw@panix.com> From: Adhemerval Zanella Message-ID: Date: Mon, 12 Mar 2018 20:36:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180307193205.4751-2-zackw@panix.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2018-03/txt/msg00296.txt.bz2 On 07/03/2018 16:31, Zack Weinberg wrote: > This patch takes the first step toward removing the global flag > __ldbl_is_dbl, creating a __vstrfmon_l_internal that takes a flags > parameter instead. > > This change arguably makes the generated code slightly worse on > architectures where __ldbl_is_dbl is never true; right now, on those > architectures, it's a compile-time constant; after this change, the > compiler could theoretically prove that __vstrfmon_l_internal was > never called with a nonzero flags argument, but it would probably need > LTO to do it. This is not performance critical code and I tend to > think that the maintainability benefits of removing action at a > distance are worth it. However, we _could_ wrap the runtime flag > check with a macro that was defined to ignore its argument and always > return false on architectures where __ldbl_is_dbl is never true, if > people think the codegen benefits are important. > > * include/monetary.h (STRFMON_LDBL_IS_DBL): New constant. > (__vstrfmon_l): Rename to __vstrfmon_l_internal and add flags > argument. > * stdlib/strfmon_l.c (__vstrfmon_l): Rename to __vstrfmon_l_internal > and add flags argument. Check flags instead of __ldbl_is_dbl when > deciding whether to set is_long_double. > (__strfmon_l): Call __vstrfmon_l_internal instead of __vstrfmon_l, > passing zero for flags argument. > * stdlib/strfmon.c (strfmon): Same change as made to __strfmon_l. > > * sysdeps/ieee754/ldbl-opt/nldbl-compat.c > (__nldbl___vstrfmon, __nldbl___vstrfmon_l) > (__nldbl_strfmon, __nldbl___strfmon_l): Call __vstrfmon_l_internal > directly, passing STRFMON_LDBL_IS_DBL for flags argument. Normalize > variable names. Remove libc_hidden_def/libc_hidden_proto. > * sysdeps/ieee754/ldbl-opt/nldbl-compat.h: Don't use NLDBL_DECL > for __nldbl___vstrfmon_l. > > * manual/locale.texi: Update a reference to vstrfmon_l in comments. LGTM with a style remark below. Reviewed-by: Adhemerval Zanella > --- > include/monetary.h | 10 +++++++--- > manual/locale.texi | 9 +++++---- > stdlib/strfmon.c | 3 ++- > stdlib/strfmon_l.c | 8 ++++---- > sysdeps/ieee754/ldbl-opt/nldbl-compat.c | 34 ++++++++++++--------------------- > sysdeps/ieee754/ldbl-opt/nldbl-compat.h | 8 +++++--- > 6 files changed, 35 insertions(+), 37 deletions(-) > > diff --git a/include/monetary.h b/include/monetary.h > index c130ed56a3..d12ae03dd3 100644 > --- a/include/monetary.h > +++ b/include/monetary.h > @@ -2,7 +2,11 @@ > #ifndef _ISOMAC > #include > > -extern ssize_t __vstrfmon_l (char *s, size_t maxsize, locale_t loc, > - const char *format, va_list ap) > - attribute_hidden; > +extern ssize_t __vstrfmon_l_internal (char *s, size_t maxsize, locale_t loc, > + const char *format, va_list ap, > + unsigned int flags); > + > +/* Flags for __vstrfmon_l_internal. */ > +#define STRFMON_LDBL_IS_DBL 0x0001 > + > #endif > diff --git a/manual/locale.texi b/manual/locale.texi > index dabb959f9e..720e0ca952 100644 > --- a/manual/locale.texi > +++ b/manual/locale.texi > @@ -1209,10 +1209,11 @@ numbers according to these rules. > > @deftypefun ssize_t strfmon (char *@var{s}, size_t @var{maxsize}, const char *@var{format}, @dots{}) > @safety{@prelim{}@mtsafe{@mtslocale{}}@asunsafe{@ascuheap{}}@acunsafe{@acsmem{}}} > -@c It (and strfmon_l) both call vstrfmon_l, which, besides accessing the > -@c locale object passed to it, accesses the active locale through > -@c isdigit (but to_digit assumes ASCII digits only). It may call > -@c __printf_fp (@mtslocale @ascuheap @acsmem) and guess_grouping (safe). > +@c It (and strfmon_l) both call __vstrfmon_l_internal, which, besides > +@c accessing the locale object passed to it, accesses the active > +@c locale through isdigit (but to_digit assumes ASCII digits only). > +@c It may call __printf_fp (@mtslocale @ascuheap @acsmem) and > +@c guess_grouping (safe). > The @code{strfmon} function is similar to the @code{strftime} function > in that it takes a buffer, its size, a format string, > and values to write into the buffer as text in a form specified > diff --git a/stdlib/strfmon.c b/stdlib/strfmon.c > index 01980d3e15..2b742c7ad7 100644 > --- a/stdlib/strfmon.c > +++ b/stdlib/strfmon.c > @@ -30,7 +30,8 @@ __strfmon (char *s, size_t maxsize, const char *format, ...) > > va_start (ap, format); > > - ssize_t res = __vstrfmon_l (s, maxsize, _NL_CURRENT_LOCALE, format, ap); > + ssize_t res = __vstrfmon_l_internal (s, maxsize, _NL_CURRENT_LOCALE, > + format, ap, 0); > > va_end (ap); > > diff --git a/stdlib/strfmon_l.c b/stdlib/strfmon_l.c > index cd3796ced9..f0ebd99bd3 100644 > --- a/stdlib/strfmon_l.c > +++ b/stdlib/strfmon_l.c > @@ -76,8 +76,8 @@ > too. Some of the information contradicts the information which can > be specified in format string. */ > ssize_t > -__vstrfmon_l (char *s, size_t maxsize, locale_t loc, const char *format, > - va_list ap) > +__vstrfmon_l_internal (char *s, size_t maxsize, locale_t loc, > + const char *format, va_list ap, unsigned int flags) > { > struct __locale_data *current = loc->__locales[LC_MONETARY]; > _IO_strfile f; > @@ -268,7 +268,7 @@ __vstrfmon_l (char *s, size_t maxsize, locale_t loc, const char *format, > if (*fmt == 'L') > { > ++fmt; > - if (!__ldbl_is_dbl) > + if (__glibc_likely ((flags & STRFMON_LDBL_IS_DBL) == 0)) > is_long_double = 1; > } > > @@ -608,7 +608,7 @@ ___strfmon_l (char *s, size_t maxsize, locale_t loc, const char *format, ...) > > va_start (ap, format); > > - ssize_t res = __vstrfmon_l (s, maxsize, loc, format, ap); > + ssize_t res = __vstrfmon_l_internal (s, maxsize, loc, format, ap, 0); > > va_end (ap); > > diff --git a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c > index bf54090d4f..7d19eaba8d 100644 > --- a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c > +++ b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c > @@ -50,8 +50,6 @@ libc_hidden_proto (__nldbl___vswprintf_chk) > libc_hidden_proto (__nldbl___vasprintf_chk) > libc_hidden_proto (__nldbl___vdprintf_chk) > libc_hidden_proto (__nldbl___obstack_vprintf_chk) > -libc_hidden_proto (__nldbl___vstrfmon) > -libc_hidden_proto (__nldbl___vstrfmon_l) > libc_hidden_proto (__nldbl___isoc99_vsscanf) > libc_hidden_proto (__nldbl___isoc99_vfscanf) > libc_hidden_proto (__nldbl___isoc99_vswscanf) > @@ -779,12 +777,13 @@ attribute_compat_text_section > __nldbl_strfmon (char *s, size_t maxsize, const char *format, ...) > { > va_list ap; > - ssize_t res; > + ssize_t ret; > > va_start (ap, format); > - res = __nldbl___vstrfmon (s, maxsize, format, ap); > + ret = __vstrfmon_l_internal (s, maxsize, _NL_CURRENT_LOCALE, format, ap, > + STRFMON_LDBL_IS_DBL); > va_end (ap); > - return res; > + return ret; > } I tend to frown at variable names changes such this case, it just add diff lines without improvement in code readability. > > ssize_t > @@ -793,12 +792,13 @@ __nldbl___strfmon_l (char *s, size_t maxsize, locale_t loc, > const char *format, ...) > { > va_list ap; > - ssize_t res; > + ssize_t ret; > > va_start (ap, format); > - res = __nldbl___vstrfmon_l (s, maxsize, loc, format, ap); > + ret = __vstrfmon_l_internal (s, maxsize, loc, format, ap, > + STRFMON_LDBL_IS_DBL); > va_end (ap); > - return res; > + return ret; > } > weak_alias (__nldbl___strfmon_l, __nldbl_strfmon_l) > > @@ -806,28 +806,18 @@ ssize_t > attribute_compat_text_section > __nldbl___vstrfmon (char *s, size_t maxsize, const char *format, va_list ap) > { > - ssize_t res; > - __no_long_double = 1; > - res = __vstrfmon_l (s, maxsize, _NL_CURRENT_LOCALE, format, ap); > - __no_long_double = 0; > - va_end (ap); > - return res; > + return __vstrfmon_l_internal (s, maxsize, _NL_CURRENT_LOCALE, format, ap, > + STRFMON_LDBL_IS_DBL); > } > -libc_hidden_def (__nldbl___vstrfmon) > > ssize_t > attribute_compat_text_section > __nldbl___vstrfmon_l (char *s, size_t maxsize, locale_t loc, > const char *format, va_list ap) > { > - ssize_t res; > - __no_long_double = 1; > - res = __vstrfmon_l (s, maxsize, loc, format, ap); > - __no_long_double = 0; > - va_end (ap); > - return res; > + return __vstrfmon_l_internal (s, maxsize, loc, format, ap, > + STRFMON_LDBL_IS_DBL); > } > -libc_hidden_def (__nldbl___vstrfmon_l) > > void > attribute_compat_text_section > diff --git a/sysdeps/ieee754/ldbl-opt/nldbl-compat.h b/sysdeps/ieee754/ldbl-opt/nldbl-compat.h > index 74f0e459fa..a9a77dce99 100644 > --- a/sysdeps/ieee754/ldbl-opt/nldbl-compat.h > +++ b/sysdeps/ieee754/ldbl-opt/nldbl-compat.h > @@ -60,7 +60,6 @@ NLDBL_DECL (vsyslog); > NLDBL_DECL (qecvt); > NLDBL_DECL (qfcvt); > NLDBL_DECL (qgcvt); > -NLDBL_DECL (__vstrfmon_l); > NLDBL_DECL (__isoc99_scanf); > NLDBL_DECL (__isoc99_fscanf); > NLDBL_DECL (__isoc99_sscanf); > @@ -74,10 +73,13 @@ NLDBL_DECL (__isoc99_vwscanf); > NLDBL_DECL (__isoc99_vfwscanf); > NLDBL_DECL (__isoc99_vswscanf); > > -/* This one does not exist in the normal interface, only > - __nldbl___vstrfmon really exists. */ > +/* These do not exist in the normal interface, but must exist in the > + __nldbl interface so that they can be called from libnldbl. */ > extern ssize_t __nldbl___vstrfmon (char *, size_t, const char *, va_list) > __THROW; > +extern ssize_t __nldbl___vstrfmon_l (char *, size_t, locale_t, const char *, > + va_list) > + __THROW; > > /* These don't use __typeof because they were not declared by the headers, > since we don't compile with _FORTIFY_SOURCE. */ >