Re: [RFC PATCH 0/5] *printf/*scanf functions for the long double migration on powerpc64le

[Joseph Myers <joseph at codesourcery dot com>]

I'll comment on general design questions around (1) what the new ABIs 
should be for IEEE 128-bit long double, (2) how the installed headers 
should arrange for those ABIs to be used and (3) how those ABIs should be 
implemented.

The general principle is that public APIs that are supported for IBM long 
double should be supported just the same for IEEE 128-bit long double.  
This in turn requires corresponding ABIs to be included in glibc.  Note 
that this only applies to ABIs accessible from *public* APIs - generally, 
ones used via calling functions or macros that are declared in public 
headers with a name not starting with '_'.


For (1), there are several different groups of ABIs that involve long 
double in some way: (a) ABIs parametrized by floating-point format, (b) 
ABIs parametrized by floating-point type, (c) ABIs not parametrized by 
floating-point type, but still involving long double in some way.

(a) has already been discussed - these are ABIs such as __issignaling* and 
__*_finite, and corresponding *f128 versions already exist and are in the 
implementation namespace, so no new ABI additions are needed to support 
IEEE 128-bit long double (beyond maybe the odd special case such as 
__scalf128_finite).

(b) has also already been discussed - most of these are libm functions, 
and while the *f128 versions already exist, there is still a use for 
adding new versions (such as __*ieee128 or __ieee128_*) as aliases of the 
*f128 functions, because *f128 aren't in the C11 implementation namespace, 
and a few such functions that are somewhat obsolescent (but still 
supported as APIs for long double) so don't have *f128 API variants.  
There are a few complications to consider there such as whether calls to 
gammal for IEEE 128-bit long double can map to the same ABI as lgammal 
(more complicated headers, fewer new ABIs), and similarly whether calls to 
finitel (and other such legacy classification *functions*) for IEEE 
128-bit long double can map to the existing __finitef128 (etc.).  There 
are also libc functions in group (b), some of which already have *f128 
variants (e.g. strtof128) and some of which don't because of obsolescence 
but are still supported APIs (e.g. qecvt).

Somewhere between (b) and (c) are the nexttoward functions - parametrized 
on a floating-point type but with an argument with is always long double 
(and thus needing special variants and redirection logic for IEEE 128-bit 
long double - new variants of nexttowardf and nexttoward are needed, while 
nexttowardl could be redirected to the same function as nextafterl).

The main point of the present patch series is functions in class (c), 
involving long double only implicitly.  These are functions in the printf, 
scanf *and strfmon* families (including, for example, a few functions such 
as printf_size - not just functions with "..." variable arguments or 
taking a va_list for such arguments; and note that syslog, for example, is 
in the printf family for this purpose).  Every such public ABI needs a 
corresponding new ABI variant for IEEE 128-bit long double.

Now, the set of such __nldbl_* ABIs for such functions for 
-mlong-double-64 is a plausible starting point for identifying relevant 
ABIs.  But it's only a starting point: there are functions that are 
missing __nldbl_* ABIs but should have them, and there are __nldbl_* ABIs 
for which the need is questionable.

For the former, as previously discussed, the printf-like argp.h, err.h and 
error.h functions, at least, are missing __nldbl_* variants for 
-mlong-double-64.  For reasons explained below, I think fixing this will 
be needed as part of the preparation for adding IEEE 128-bit long double 
variants of those functions.

For the latter, for example, your patch 4 adds public ABIs for 
__ieee128___printf_fp, __ieee128__IO_vfprintf and __ieee128_vfprintf.  
There is certainly no need for __ieee128__IO_vfprintf, since _IO_vfprintf 
is not declared in any installed header since we stopped installing 
libio.h.  But even if _IO_vfprintf were a public API, it *still* wouldn't 
need __ieee128__IO_vfprintf, because the semantics of _IO_vfprintf are 
exactly those of vfprintf, so just having __ieee128_vfprintf suffices.  
And __printf_fp isn't declared in any installed header, a strong 
indication that it is not a public API or ABI and so no export of 
__ieee128___printf_fp is needed either.  (The comment in 
stdio-common/Versions on __printf_fp says "functions used in other 
libraries", but as far as I can tell it's *not* in fact now used in any 
other glibc library - and in any case, being used in another glibc library 
would only justify an export at GLIBC_PRIVATE under current practice.)

So the __nldbl_* exports need reviewing to identify which are actually 
relevant for new long double formats, and my guess is that 
__nldbl__IO_fprintf __nldbl__IO_printf __nldbl__IO_sprintf 
__nldbl__IO_sscanf __nldbl__IO_vfprintf __nldbl__IO_vfscanf 
__nldbl__IO_vsprintf __nldbl___asprintf __nldbl___printf_fp 
__nldbl___strfmon_l __nldbl___vfscanf __nldbl___vsnprintf 
__nldbl___vsscanf __nldbl___vstrfmon __nldbl___vstrfmon_l all in fact are 
not needed for new formats - either because the non-__nldbl versions 
aren't exported at all (__vstrfrmon, __vstrfmon_l - the __nldbl_* versions 
exist only for use from libnldbl_nonshared.a, I think), or because the 
non-__nldbl versions are not actually public APIs (__printf_fp), or 
because the functions exactly alias variants without __ or _IO_ (and with 
an __ieee128_ prefix you're automatically in the implementation namespace 
and so don't need such aliases).


Now for (2), the installed headers.  We have, for example, 
libio/bits/stdio-ldbl.h (installed as bits/stdio-ldbl.h), with a series of 
__LDBL_REDIR_DECL and __LDBL_REDIR1_DECL calls to redirect particular 
functions to __nldbl_*.  My claim is that *the same* headers should be 
reused, installed under additional conditions, to handle redirections to 
__ieee128_*.

* You'd need to have a different definition of __LDBL_REDIR_DECL that adds 
the __ieee128_ prefix instead of __nldbl_.

* You'd need to change __LDBL_REDIR1_DECL, so that either it takes both 
the -mlong-double-64 and -mabi=ieeelongdouble function names as arguments, 
or so that there are two variants (one that gets called with arguments 
e.g. (scanf, __isoc99_scanf), in the case where __nldbl_ and __ieee128_ 
are the appropriate prefixes; one that gets called with arguments e.g. 
(strtold, strtod), as at present, where the second argument is the right 
name for -mlong-double-64 but __ieee128_ plus the first argument is the 
right name for -mabi=ieeelongdouble).

* And you'd need to change a few calls for cases where I've said above to 
eliminate ABIs that exist for __nldbl_*, e.g. "__LDBL_REDIR_DECL 
(__asprintf)", so that it uses the first new variant of __LDBL_REDIR1_DECL 
to generate calls to __nldbl_asprintf / __ieee128_asprintf, because 
__ieee128___asprintf wouldn't exist.

* Of course where the present includes of these bits/*ldbl.h headers are 
conditional on __LDBL_COMPAT, you'd need separate header conditions for 
"compat for long double = double", "long double = IEEE 128" and "*some* 
remapping of long double names, which might be either of the above, is in 
effect", and to update all the tests of __LDBL_COMPAT in the existing 
installed headers.

* And you need a few new bits/*ldbl.h headers for argp.h, err.h and 
error.h.  Using a shared mechanism for both __nldbl_* and __ieee128_* is 
why I said above that fixing the lack of __nldbl_* for those functions is 
a prerequisite to be able to add __ieee128_* for them.

I think doing things that way - shared long double redirection headers, 
with only the definitions of macros such as __LDBL_REDIR_DECL being 
different in the new IEEE 128-bit case - is clearly much cleaner than 
having a separate set of redirection headers for the new case.  We've had 
enough problems in the past with feature test macro tests in *-ldbl.h 
getting out of sync with those in the including headers, we don't want to 
add a third place that also needs feature test macro handling kept in 
sync.

(Of course that would mean you can't have redirections for some but not 
all functions in the installed headers as a transitional state, because 
such redirections would automatically be present for all or none of the 
relevant functions.)


Now for (3), how the __ieee128_* for printf / scanf / strfmon functions 
are implemented.  As previously discussed, the approach used for many of 
the __nldbl_* functions, of setting a TLS variable __no_long_double, is 
not a good one to emulate.  But that does not necessarily mean the two 
sets of functions should be implemented in different ways.  The key 
question to answer is: is the best way to implement __ieee128_* the same 
as or different from the best way to implement __nldbl_*?  If they are the 
same, changing how __nldbl_* are implemented may well be good preparation 
for implementing __ieee128_*.

Specifically, Zack sent a patch series to move away from __no_long_double 
to explicit flags passed to internal functions.  So if there's some reason 
why you think such explicit flags are not the best approach for 
implementing __ieee128_*, but are the best approach for __nldbl_* you need 
to explain why you think that's the case.  Otherwise, if explicit flags 
would be appropriate for __ieee128_*, it would seem to make sense to work 
on getting Zack's patches into glibc (for example, updating them for your 
review comments and reposting them, if he hasn't had time to update them 
himself) as preparation for __ieee128_* being able to use such flags.  
(I'm presuming you do think the explicit flags are the right approach for 
__nldbl_* since you didn't mention otherwise in your reviews of some of 
Zack's patches.)

I note that, for example, __printf_fp *already* supports _Float128.  So it 
really ought not be necessary to build a second copy of it at all, if you 
can make vfprintf for IEEE 128-bit long double (however implemented) pass 
appropriate data structures for the existing __printf_fp to behave as 
required.

-- 
Joseph S. Myers
joseph@codesourcery.com