This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/9] Use STRFMON_LDBL_IS_DBL instead of __ldbl_is_dbl.
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 <adhemerval.zanella@linaro.org>
> ---
> 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 <stdarg.h>
>
> -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. */
>