Re: [PATCH v2 1/8] Use STRFMON_LDBL_IS_DBL instead of __ldbl_is_dbl.

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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.