[v2,1/8] Use STRFMON_LDBL_IS_DBL instead of __ldbl_is_dbl.
Commit Message
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.
2018-10-11 Zack Weinberg <zackw@panix.com>
Gabriel F. T. Gomes <gabriel@inconstante.eti.br>
* 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 from
__nldbl___vstrfmon and __nldbl___vstrfmon_l, because they are no
longer called from within the library.
* sysdeps/ieee754/ldbl-opt/nldbl-compat.h: Don't use NLDBL_DECL
for __nldbl___vstrfmon_l, declare it explicitly.
* manual/locale.texi: Update a reference to vstrfmon_l in comments.
Signed-off-by: Zack Weinberg <zackw@panix.com>
Signed-off-by: Gabriel F. T. Gomes <gabriel@inconstante.eti.br>
---
include/monetary.h | 12 +++++++++---
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, 37 insertions(+), 37 deletions(-)
Comments
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.
@@ -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
@@ -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
@@ -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);
@@ -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);
@@ -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
@@ -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. */