Message ID | 20181029121650.24544-2-gabriel@inconstante.eti.br |
---|---|
State | New |
Headers | show |
Series | Use more flags parameters instead of global bits in stdio | expand |
On 29/10/2018 09:16, Gabriel F. T. Gomes wrote: > From: Zack Weinberg <zackw@panix.com> > > Changed since v1: > > - Added attribute hidden to __vstrfmon_l_internal. > On powerpc, code in the call sites changed: > From: > $ objdump -d --reloc VISIBLE-glibc/libc.so.6 | grep __nldbl___vstrfmon_l -A 13 > 001502a0 <__nldbl___vstrfmon_l>: > 1502a0: 94 21 ff f0 stwu r1,-16(r1) > 1502a4: 39 00 00 01 li r8,1 > 1502a8: 7c 08 02 a6 mflr r0 > 1502ac: 42 9f 00 05 bcl 20,4*cr7+so,1502b0 <__nldbl___vstrfmon_l+0x10> > 1502b0: 93 c1 00 08 stw r30,8(r1) > 1502b4: 90 01 00 14 stw r0,20(r1) > 1502b8: 7f c8 02 a6 mflr r30 > 1502bc: 3f de 00 05 addis r30,r30,5 > 1502c0: 3b de fd 44 addi r30,r30,-700 > 1502c4: 4b ef b6 5d bl 4b920 <__vstrfmon_l_internal> > 1502c8: 80 01 00 14 lwz r0,20(r1) > 1502cc: 83 c1 00 08 lwz r30,8(r1) > 1502d0: 38 21 00 10 addi r1,r1,16 > 1502d4: 7c 08 03 a6 mtlr r0 > 1502d8: 4e 80 00 20 blr > 1502dc: 60 00 00 00 nop > To: > $ objdump -d --reloc HIDDEN-glibc/libc.so.6 | grep __nldbl___vstrfmon_l -A 5 > 00150260 <__nldbl___vstrfmon_l>: > 150260: 39 00 00 01 li r8,1 > 150264: 4b ef b6 bc b 4b920 <__vstrfmon_l_internal> > 150268: 60 00 00 00 nop > 15026c: 60 00 00 00 nop > - Replaced blocks of eight spaces with tabs. > - Added signed-off-by field with Zack's and mine names. > - Extended commit message and comments. > > -- 8< -- > On platforms where long double used to have the same format as double, > but later switched to a different format (alpha, s390, sparc, and > powerpc), accessing the older behavior is possible and it happens via > __nldbl_* functions (not on the API, but accessible from header > redirection and from compat symbols). These functions write to the > global flag __ldbl_is_dbl, which tells other functions that long double > variables should be handled as double. This patch takes the first step > towards removing this global flag and creates __vstrfmon_l_internal, > which takes an explicit flags parameter. > > 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. > > Tested for powerpc and powerpc64le. Still LGTM, thanks with some nits below. > diff --git a/include/monetary.h b/include/monetary.h > index c130ed56a3..a226305adf 100644 > --- a/include/monetary.h > +++ b/include/monetary.h > @@ -2,7 +2,13 @@ > #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) > + attribute_hidden; > + > +/* Flags for __vstrfmon_l_internal. */ > +#define STRFMON_LDBL_IS_DBL 0x0001 Please extend the flag comment to describe what it does.
* Gabriel F. T. Gomes: > Signed-off-by: Zack Weinberg <zackw@panix.com> > Signed-off-by: Gabriel F. T. Gomes <gabriel@inconstante.eti.br> We don't use DCO, but copyright assignments, so this is incorrect. Thanks, Florian
On Wed, 07 Nov 2018, Adhemerval Zanella wrote: >On 29/10/2018 09:16, Gabriel F. T. Gomes wrote: >> From: Zack Weinberg <zackw@panix.com> >> >> +extern ssize_t >> +__vstrfmon_l_internal (char *s, size_t maxsize, locale_t loc, >> + const char *format, va_list ap, >> + unsigned int flags) >> + attribute_hidden; >> + >> +/* Flags for __vstrfmon_l_internal. */ >> +#define STRFMON_LDBL_IS_DBL 0x0001 > >Please extend the flag comment to describe what it does. With the proposed text (below) and DCO's signed-off-by statements removed from the commit message, is it OK for master? +/* Flags for __vstrfmon_l_internal. + + STRFMON_LDBL_IS_DBL is a one-bit mask for the flags parameter that + indicates whether long double values are to be handled as having the + same format as double, in which case the flag should be set to one, + or as another format, otherwise. */ +#define STRFMON_LDBL_IS_DBL 0x0001
On 13/11/2018 10:08, Gabriel F. T. Gomes wrote: > On Wed, 07 Nov 2018, Adhemerval Zanella wrote: >> On 29/10/2018 09:16, Gabriel F. T. Gomes wrote: >>> From: Zack Weinberg <zackw@panix.com> >>> >>> +extern ssize_t >>> +__vstrfmon_l_internal (char *s, size_t maxsize, locale_t loc, >>> + const char *format, va_list ap, >>> + unsigned int flags) >>> + attribute_hidden; >>> + >>> +/* Flags for __vstrfmon_l_internal. */ >>> +#define STRFMON_LDBL_IS_DBL 0x0001 >> >> Please extend the flag comment to describe what it does. > > With the proposed text (below) and DCO's signed-off-by statements removed > from the commit message, is it OK for master? > > +/* Flags for __vstrfmon_l_internal. > + > + STRFMON_LDBL_IS_DBL is a one-bit mask for the flags parameter that > + indicates whether long double values are to be handled as having the > + same format as double, in which case the flag should be set to one, > + or as another format, otherwise. */ > +#define STRFMON_LDBL_IS_DBL 0x0001 > LGTM, thanks.
On Thu, 15 Nov 2018, Adhemerval Zanella wrote: >On 13/11/2018 10:08, Gabriel F. T. Gomes wrote: >> >> +/* Flags for __vstrfmon_l_internal. >> + >> + STRFMON_LDBL_IS_DBL is a one-bit mask for the flags parameter that >> + indicates whether long double values are to be handled as having the >> + same format as double, in which case the flag should be set to one, >> + or as another format, otherwise. */ >> +#define STRFMON_LDBL_IS_DBL 0x0001 >> > >LGTM, thanks. Thanks, now committed with this change.
diff --git a/include/monetary.h b/include/monetary.h index c130ed56a3..a226305adf 100644 --- a/include/monetary.h +++ b/include/monetary.h @@ -2,7 +2,13 @@ #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) + attribute_hidden; + +/* 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..3058b3eed1 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..5293104400 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 ffb5fabebe..7a1e89c1a3 100644 --- a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c +++ b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c @@ -51,8 +51,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) @@ -780,12 +778,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; } ssize_t @@ -794,12 +793,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) @@ -807,28 +807,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 b7e938f785..b7606c3c2d 100644 --- a/sysdeps/ieee754/ldbl-opt/nldbl-compat.h +++ b/sysdeps/ieee754/ldbl-opt/nldbl-compat.h @@ -64,7 +64,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); @@ -78,10 +77,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. */