| Message ID | 20180524043553.23569-1-gabriel@inconstante.eti.br |
|---|---|
| Headers | show |
| Series | *printf/*scanf functions for the long double migration on powerpc64le | expand |
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.
Hi, Joseph, Thanks for your detailed reply and explanations... On Thu, 24 May 2018, Joseph Myers wrote: >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. Yes, indeed. And you have already explained that before, so, sorry about that (I could try to explain myself saying that I induced myself to error, because I was thinking of additions to the ABI for the *IBM* format, but now I finally understood that even in that case there would be no need for new *_IO_* symbols. Really sorry). >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.) Yes. Sorry about this, too. >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.) OK. I'll probably keep this header (stdio-ieee128.h) in a *local branch*, because it allows us to progress in smaller sets of functions. In future patch submissions (before all functions are ready), I'll not send it, but I could make it available if someone wants to test the functions. Then, when all functions are ready, we'll send a patch to do as you suggested (i.e.: one which adjusts bits/stdio-ldbl.h with no extra headers). >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.) You presumed correctly, indeed. I think that Zack's proposal is the right thing to have in the end. However, I also expected that rebuilding these files with -mabi=ieeelongdouble had the advantage of being potentially easier to get right, because I wouldn't need to touch anything from the current implementation. Besides that, I also expect that it will be faster to get done, because it only adds files to the build, and since this is useful for POWER9 users, getting it done sooner than later is something that we need. Taking this "timing" perspective into account and also that there seems to be other problems that would need fixing before Zack's proposal can be fully complete [1], would it be OK to have these functions added as new implementations in a first step? Afterwards, with "timing" concerns out of the way, we could work together (if Zack is OK with that) on the cleanup (for nldbl and ieee128-on-powerpc). [1] https://sourceware.org/ml/libc-alpha/2018-03/msg00554.html >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. For the same reason (getting the new format available sooner than later), I thought of starting with a copy of vfprintf (adjusted to use a copy of __printf_fp).
On Mon, 28 May 2018, Gabriel F. T. Gomes wrote: > You presumed correctly, indeed. I think that Zack's proposal is the right > thing to have in the end. However, I also expected that rebuilding these > files with -mabi=ieeelongdouble had the advantage of being potentially > easier to get right, because I wouldn't need to touch anything from the > current implementation. Besides that, I also expect that it will be > faster to get done, because it only adds files to the build, and since this > is useful for POWER9 users, getting it done sooner than later is something > that we need. > > Taking this "timing" perspective into account and also that there seems to > be other problems that would need fixing before Zack's proposal can be > fully complete [1], would it be OK to have these functions added as new > implementations in a first step? Afterwards, with "timing" concerns out > of the way, we could work together (if Zack is OK with that) on the > cleanup (for nldbl and ieee128-on-powerpc). > > [1] https://sourceware.org/ml/libc-alpha/2018-03/msg00554.html I don't think tests for hidden annotation issues (or fixes for existing such issues) are needed in order to make the move to explicit flags here; the patches just need reviewing / updating to follow the rules described at <https://sourceware.org/ml/libc-alpha/2018-03/msg00552.html> for any new internal functions they add. I'm also not convinced that building files twice, and making sure that all calls within those files end up pointed to the new ieee128 internal functions when needed, is any easier to get right than passing explicit flags, and making sure that calls within those files call appropriate other internal functions that take those flags - especially given that Zack's patches already do most of what's required regarding passing those flags around (new work for argp.h / err.h / error.h printf-like functions being needed in any case). So rather than going down the dead-end route of building the files twice and so complicating the subsequent cleanup to use explicit flags, I think the support for explicit flags (along the lines of Zack's patches) should be added first. That is, first in the commit sequence on master. You could of course maintain multiple public branches, one with the latest version of Zack's patches and one with ieee128 patches built on top of that, working on them in parallel and posting for review patches from each branch as and when they are ready, with patches from the ieee128 branch not actually being able to go on master until all the changes they depend on are also in master. Some changes could be independent of both of the above to some extent - for example, the argp.h / err.h / error.h ldbl-opt support, or the header changes to support calling appropriate ieee128 functions on the basis that the actual changes defining new macros to activate the new header code for powerpc64le would go in last of all along with the actual addition of all the new functions to the ABI. So there could be rather more than two branches involved while development is active, all being developed and having patches posted, reviewed and revised in parallel.
On Thu, May 31, 2018 at 5:23 PM, Joseph Myers <joseph@codesourcery.com> wrote: > On Mon, 28 May 2018, Gabriel F. T. Gomes wrote: > >> You presumed correctly, indeed. I think that Zack's proposal is the right >> thing to have in the end. ... > I don't think tests for hidden annotation issues (or fixes for existing > such issues) are needed in order to make the move to explicit flags here; > the patches just need reviewing / updating to follow the rules described > at <https://sourceware.org/ml/libc-alpha/2018-03/msg00552.html> for any > new internal functions they add. For the record, I might not get to it but it is my intention to update my __no_long_double removal patchset this weekend, adding appropriate hidden annotations and addressing any other review comments that are still outstanding. I did write some tests to expose existing issues with missing hidden annotations; the problem was they exposed a _lot_ of missing hidden annotations, and fixing them all, well, that rabbit hole bottomed out at Adhemerval's thread cancellation patches! zw
Each RFC patch in this thread has its own comments, but I'd like to point out that: * Patch 1/5 is currently under review on libc-alpha, but I added it to this series, because it makes the series usable (testable). * Patches 2/5 and 3/5 are new cleanups (required by the next patch). * Patch 4/5 is the bulk of the RFC. * Patch 5/5 is there just to allow people to test the series. Gabriel F. T. Gomes (4): Do not define and undefine vfprintf in vfprintf.c Do not declare __mpn_extract_float128 when long double has binary128 format RFC: Two vfprintf implementations (IBM and IEEE 128) RFC: powerpc64le: Convert default long double format to IEEE binary128 Tulio Magno Quites Machado Filho (1): powerpc: Move around math-related Implies include/gmp.h | 2 +- libio/stdio.h | 9 ++++ stdio-common/vfprintf.c | 9 ++-- sysdeps/ieee754/ldbl-128ibm-compat/Makefile | 20 +++++++++ sysdeps/ieee754/ldbl-128ibm-compat/Versions | 12 +++++ .../ldbl-128ibm-compat/bits/stdio-ieee128.h | 30 +++++++++++++ .../ieee754/ldbl-128ibm-compat/ieee128-printf_fp.c | 16 +++++++ .../ldbl-128ibm-compat/ieee128-printf_fphex.c | 16 +++++++ .../ieee754/ldbl-128ibm-compat/ieee128-vfprintf.c | 13 ++++++ .../ldbl-128ibm-compat/test-printf-ibm128.c | 1 + .../ldbl-128ibm-compat/test-printf-ieee128.c | 1 + .../ldbl-128ibm-compat/test-printf-ldbl-compat.c | 51 ++++++++++++++++++++++ sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h | 27 +++++++++++- .../powerpc/{Implies => powerpc32/Implies-after} | 0 sysdeps/powerpc/powerpc64/be/Implies-after | 5 +++ sysdeps/powerpc/powerpc64/le/Implies-before | 6 +++ .../powerpc/powerpc64/le/ldbl-128ibm-compat-abi.h | 8 ++++ .../sysv/linux/powerpc/powerpc64/libc-le.abilist | 3 ++ 18 files changed, 221 insertions(+), 8 deletions(-) create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/Makefile create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/Versions create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/bits/stdio-ieee128.h create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/ieee128-printf_fp.c create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/ieee128-printf_fphex.c create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/ieee128-vfprintf.c create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/test-printf-ibm128.c create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/test-printf-ieee128.c create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/test-printf-ldbl-compat.c rename sysdeps/powerpc/{Implies => powerpc32/Implies-after} (100%) create mode 100644 sysdeps/powerpc/powerpc64/be/Implies-after create mode 100644 sysdeps/unix/sysv/linux/powerpc/powerpc64/le/ldbl-128ibm-compat-abi.h