Message ID | 20191025153410.15405-24-gabriel@inconstante.net.br |
---|---|
State | New |
Headers | show |
Series | Add IEEE long double <-> string functions for powerpc64le | expand |
On 10/25/19 10:34 AM, Gabriel F. T. Gomes wrote: > From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> > > Changes since v1: > > - Adapted after patch v1 19/31 (remove hidden_* uses) was dropped. > - Fixed unintended removal of cvt* symbols on alpha and s390x > (now tested with build-many-glibcs.py). > > -- 8< -- > This patch is to be squashed with the other n/5 refactoring patches. > > This patch replaces the use of the APPEND macro with one new macro being > defined for each of the cvt* functions. This makes it easier to define > functions names for IEEE long double on powerpc64le, e.g. __ecvtieee128. I think the non-trivial changes here to fixup the shuffling and the preceding four patches should be squashed and resent as a single patch to the list. I am guessing they should squash into a somewhat readable patch self contained patch. The slightly more substantial changes needed to support the new ldbl would be more easily reviewed as a separate patch (e.g replacing the __APPEND macro and reworking cvt_symbol) > diff --git a/misc/efgcvt.c b/misc/efgcvt.c > index 81ac60415e..93a0269d5c 100644 > --- a/misc/efgcvt.c > +++ b/misc/efgcvt.c > @@ -16,19 +16,27 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#include "efgcvt.c" I see some noise in the refactoring :). (and in efgcvt_r.c), > diff --git a/misc/efgcvt_r-template.c b/misc/efgcvt_r-template.c ... > -#define FLOOR APPEND(floor, FLOAT_NAME_EXT) > -#define FABS APPEND(fabs, FLOAT_NAME_EXT) > -#define LOG10 APPEND(log10, FLOAT_NAME_EXT) > -#define EXP APPEND(exp, FLOAT_NAME_EXT) These macros don't even appear to be used... OK. > diff --git a/misc/efgcvt_r.c b/misc/efgcvt_r.c > index aa0eb8ca43..5b48ca5b9b 100644 > --- a/misc/efgcvt_r.c > +++ b/misc/efgcvt_r.c > @@ -16,22 +16,24 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#include "efgcvt_r.c" > +#define ECVT_R ecvt_r > +#define FCVT_R fcvt_r > +#define __ECVT_R __ecvt_r > +#define __FCVT_R __fcvt_r > +#include <efgcvt-dbl-macros.h> > +#include <efgcvt_r-template.c> > > #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) > -# define cvt_symbol(symbol) \ > - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ > - APPEND (q, symbol), GLIBC_2_0); \ > - weak_alias (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) > +# define cvt_symbol(local, symbol) \ > + cvt_symbol_1 (libc, local, APPEND (q, symbol), GLIBC_2_0); \ > + weak_alias (local, symbol) > # define cvt_symbol_1(lib, local, symbol, version) \ > libc_hidden_def (local) \ > compat_symbol (lib, local, symbol, version) > #else > -# define cvt_symbol(symbol) \ > - cvt_symbol_1 (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) > -# define cvt_symbol_1(local, symbol) \ > +# define cvt_symbol(local, symbol) \ > libc_hidden_def (local) \ > weak_alias (local, symbol) > #endif > -cvt_symbol (fcvt_r); > -cvt_symbol (ecvt_r); > +cvt_symbol (__fcvt_r, fcvt_r); > +cvt_symbol (__ecvt_r, ecvt_r); > diff --git a/misc/qefgcvt.c b/misc/qefgcvt.c > index ea48c6b48a..903bf93aa4 100644 > --- a/misc/qefgcvt.c > +++ b/misc/qefgcvt.c > @@ -16,18 +16,24 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#include "efgcvt.c" > +#define ECVT qecvt > +#define FCVT qfcvt > +#define GCVT qgcvt > +#define __ECVT __qecvt > +#define __FCVT __qfcvt > +#define __GCVT __qgcvt > +#define __ECVT_R __qecvt_r > +#define __FCVT_R __qfcvt_r > +#include <efgcvt-ldbl-macros.h> > +#include <efgcvt-template.c> > > #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) > -# define cvt_symbol(symbol) \ > - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ > - APPEND (FUNC_PREFIX, symbol), GLIBC_2_4) > -# define cvt_symbol_1(lib, local, symbol, version) \ > - versioned_symbol (lib, local, symbol, version) > +# define cvt_symbol(local, symbol) \ > + versioned_symbol (libc, local, symbol, GLIBC_2_4) > #else > -# define cvt_symbol(symbol) \ > - strong_alias (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) > +# define cvt_symbol(local, symbol) \ > + strong_alias (local, symbol) > #endif > -cvt_symbol(fcvt); > -cvt_symbol(ecvt); > -cvt_symbol(gcvt); > +cvt_symbol (__qfcvt, qfcvt); > +cvt_symbol (__qecvt, qecvt); > +cvt_symbol (__qgcvt, qgcvt); > diff --git a/misc/qefgcvt_r.c b/misc/qefgcvt_r.c > index d2f36e6d2a..b790bf7837 100644 > --- a/misc/qefgcvt_r.c > +++ b/misc/qefgcvt_r.c > @@ -17,21 +17,21 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#include "efgcvt_r.c" > +#define ECVT_R qecvt_r > +#define FCVT_R qfcvt_r > +#define __ECVT_R __qecvt_r > +#define __FCVT_R __qfcvt_r > +#include <efgcvt-ldbl-macros.h> > +#include <efgcvt_r-template.c> > > #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) > -# define cvt_symbol(symbol) \ > - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ > - APPEND (FUNC_PREFIX, symbol), GLIBC_2_4) > -# define cvt_symbol_1(lib, local, symbol, version) \ > +# define cvt_symbol(local, symbol) \ > libc_hidden_def (local) \ > - versioned_symbol (lib, local, symbol, version) > + versioned_symbol (libc, local, symbol, GLIBC_2_4) This isn't a bug with the patch, but should the version tested in LONG_DOUBLE_COMPAT match that passed into versioned_symbol? > #else > -# define cvt_symbol(symbol) \ > - cvt_symbol_1 (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) > -# define cvt_symbol_1(local, symbol) \ > +# define cvt_symbol(local, symbol) \ > libc_hidden_def (local) \ > weak_alias (local, symbol) > #endif The two non-trivial versioning macros get duplicated in two places. Is it possible to unify the four very similar instances into a single shared macro? > -cvt_symbol(fcvt_r); > -cvt_symbol(ecvt_r); > +cvt_symbol (__qfcvt_r, qfcvt_r); > +cvt_symbol (__qecvt_r, qecvt_r); >
On Thu, 14 Nov 2019, Paul E Murphy wrote: > >I think the non-trivial changes here to fixup the shuffling and the >preceding four patches should be squashed and resent as a single patch >to the list. I am guessing they should squash into a somewhat readable >patch self contained patch. I guess you're right. I'll send them squashed in v3. >The slightly more substantial changes needed to support the new ldbl >would be more easily reviewed as a separate patch (e.g replacing the >__APPEND macro and reworking cvt_symbol) Agreed. >On 10/25/19 10:34 AM, Gabriel F. T. Gomes wrote: >> From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> >> >> -#include "efgcvt.c" > >I see some noise in the refactoring :). (and in efgcvt_r.c), :( >> #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) >> -# define cvt_symbol(symbol) \ >> - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ >> - APPEND (FUNC_PREFIX, symbol), GLIBC_2_4) >> -# define cvt_symbol_1(lib, local, symbol, version) \ >> +# define cvt_symbol(local, symbol) \ >> libc_hidden_def (local) \ >> - versioned_symbol (lib, local, symbol, version) >> + versioned_symbol (libc, local, symbol, GLIBC_2_4) > >This isn't a bug with the patch, but should the version tested in >LONG_DOUBLE_COMPAT match that passed into versioned_symbol? We test against GLIBC_2_0, because that's the version that introduced q{efg}cvt symbols for the first time. The LONG_DOUBLE_COMPAT macro then expands it to SHLIB_COMPAT with GLIBC_2_0 in the 'introduced' argument and LONG_DOUBLE_COMPAT_VERSION in the 'obsoleted' argument. I think it would be OK to change GLIBC_2_4 to LONG_DOUBLE_COMPAT_VERSION (maybe that would make it easier to read). For completeness, on powerpc64, replacing GLIBC_2_0 with GLIBC_2_4 makes test cases fail to build, e.g.: misc/./tst-efgcvt-template.c:214: undefined reference to `qfcvt_r' .../misc/tst-ldbl-efgcvt.o:(.toc+0x0): undefined reference to `qecvt' .../misc/tst-ldbl-efgcvt.o:(.toc+0x8): undefined reference to `qfcvt' .../misc/tst-ldbl-efgcvt.o:(.toc+0x10): undefined reference to `qecvt_r' .../misc/tst-ldbl-efgcvt.o:(.toc+0x18): undefined reference to `qfcvt_r' collect2: error: ld returned 1 exit status >The two non-trivial versioning macros get duplicated in two places. Is >it possible to unify the four very similar instances into a single >shared macro? Before this refactoring, the macro definition for {efg}cvt and q{efg}cvt was defined in a single file, but I failed to see any benefit in it (it relied on the LONG_DOUBLE_CVT macro and provided completely different definitions for each case (long double and double). Likewise for the sharing between {efg}cvt_r and q{efg}cvt_r. As for the differences between {efg}cvt and {efg}cvt_r, they seem too large to allow for the sharing of code (one uses weak_symbol where the other uses strong_symbol; only [q]{efg}cvt_r require hidden_def). Likewise for the long double versions. I think a single definition for all cases would make the code harder to read (maybe it would make it easier to maintain, but I actually find it very confusing and hard to maintain when the macros depend on too many things).
diff --git a/misc/efgcvt-template.c b/misc/efgcvt-template.c index aeb4e1ea01..7fabdf264d 100644 --- a/misc/efgcvt-template.c +++ b/misc/efgcvt-template.c @@ -25,8 +25,6 @@ #define APPEND(a, b) APPEND2 (a, b) #define APPEND2(a, b) a##b -#define __APPEND(a, b) __APPEND2 (a, b) -#define __APPEND2(a, b) __##a##b #define FCVT_BUFFER APPEND (FUNC_PREFIX, fcvt_buffer) @@ -39,13 +37,11 @@ static char ECVT_BUFFER[MAXDIG]; libc_freeres_ptr (static char *FCVT_BUFPTR); char * -__APPEND (FUNC_PREFIX, fcvt) (FLOAT_TYPE value, int ndigit, int *decpt, - int *sign) +__FCVT (FLOAT_TYPE value, int ndigit, int *decpt, int *sign) { if (FCVT_BUFPTR == NULL) { - if (__APPEND (FUNC_PREFIX, fcvt_r) (value, ndigit, decpt, sign, - FCVT_BUFFER, MAXDIG) != -1) + if (__FCVT_R (value, ndigit, decpt, sign, FCVT_BUFFER, MAXDIG) != -1) return FCVT_BUFFER; FCVT_BUFPTR = (char *) malloc (FCVT_MAXDIG); @@ -53,25 +49,22 @@ __APPEND (FUNC_PREFIX, fcvt) (FLOAT_TYPE value, int ndigit, int *decpt, return FCVT_BUFFER; } - (void) __APPEND (FUNC_PREFIX, fcvt_r) (value, ndigit, decpt, sign, - FCVT_BUFPTR, FCVT_MAXDIG); + (void) __FCVT_R (value, ndigit, decpt, sign, FCVT_BUFPTR, FCVT_MAXDIG); return FCVT_BUFPTR; } char * -__APPEND (FUNC_PREFIX, ecvt) (FLOAT_TYPE value, int ndigit, int *decpt, - int *sign) +__ECVT (FLOAT_TYPE value, int ndigit, int *decpt, int *sign) { - (void) __APPEND (FUNC_PREFIX, ecvt_r) (value, ndigit, decpt, sign, - ECVT_BUFFER, MAXDIG); + (void) __ECVT_R (value, ndigit, decpt, sign, ECVT_BUFFER, MAXDIG); return ECVT_BUFFER; } char * -__APPEND (FUNC_PREFIX, gcvt) (FLOAT_TYPE value, int ndigit, char *buf) +__GCVT (FLOAT_TYPE value, int ndigit, char *buf) { sprintf (buf, "%.*" FLOAT_FMT_FLAG "g", MIN (ndigit, NDIGIT_MAX), value); return buf; diff --git a/misc/efgcvt.c b/misc/efgcvt.c index 81ac60415e..93a0269d5c 100644 --- a/misc/efgcvt.c +++ b/misc/efgcvt.c @@ -16,19 +16,27 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include "efgcvt.c" +#define ECVT ecvt +#define FCVT fcvt +#define GCVT gcvt +#define __ECVT __ecvt +#define __FCVT __fcvt +#define __GCVT __gcvt +#define __ECVT_R __ecvt_r +#define __FCVT_R __fcvt_r +#include <efgcvt-dbl-macros.h> +#include <efgcvt-template.c> #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) -# define cvt_symbol(symbol) \ - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ - APPEND (q, symbol), GLIBC_2_0); \ - strong_alias (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) +# define cvt_symbol(local, symbol) \ + cvt_symbol_1 (libc, local, APPEND (q, symbol), GLIBC_2_0); \ + strong_alias (local, symbol) # define cvt_symbol_1(lib, local, symbol, version) \ compat_symbol (lib, local, symbol, version) #else -# define cvt_symbol(symbol) \ - strong_alias (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) +# define cvt_symbol(local, symbol) \ + strong_alias (local, symbol) #endif -cvt_symbol (fcvt); -cvt_symbol (ecvt); -cvt_symbol (gcvt); +cvt_symbol (__fcvt, fcvt); +cvt_symbol (__ecvt, ecvt); +cvt_symbol (__gcvt, gcvt); diff --git a/misc/efgcvt_r-template.c b/misc/efgcvt_r-template.c index f215dbd345..977750118c 100644 --- a/misc/efgcvt_r-template.c +++ b/misc/efgcvt_r-template.c @@ -28,18 +28,10 @@ #define APPEND(a, b) APPEND2 (a, b) #define APPEND2(a, b) a##b -#define __APPEND(a, b) __APPEND2 (a, b) -#define __APPEND2(a, b) __##a##b - -#define FLOOR APPEND(floor, FLOAT_NAME_EXT) -#define FABS APPEND(fabs, FLOAT_NAME_EXT) -#define LOG10 APPEND(log10, FLOAT_NAME_EXT) -#define EXP APPEND(exp, FLOAT_NAME_EXT) - int -__APPEND (FUNC_PREFIX, fcvt_r) (FLOAT_TYPE value, int ndigit, int *decpt, - int *sign, char *buf, size_t len) +__FCVT_R (FLOAT_TYPE value, int ndigit, int *decpt, int *sign, + char *buf, size_t len) { ssize_t n; ssize_t i; @@ -133,8 +125,8 @@ __APPEND (FUNC_PREFIX, fcvt_r) (FLOAT_TYPE value, int ndigit, int *decpt, } int -__APPEND (FUNC_PREFIX, ecvt_r) (FLOAT_TYPE value, int ndigit, int *decpt, - int *sign, char *buf, size_t len) +__ECVT_R (FLOAT_TYPE value, int ndigit, int *decpt, int *sign, + char *buf, size_t len) { int exponent = 0; @@ -193,8 +185,8 @@ __APPEND (FUNC_PREFIX, ecvt_r) (FLOAT_TYPE value, int ndigit, int *decpt, *sign = isfinite (value) ? signbit (value) != 0 : 0; } else - if (__APPEND (FUNC_PREFIX, fcvt_r) (value, MIN (ndigit, NDIGIT_MAX) - 1, - decpt, sign, buf, len)) + if (__FCVT_R (value, MIN (ndigit, NDIGIT_MAX) - 1, + decpt, sign, buf, len)) return -1; *decpt += exponent; diff --git a/misc/efgcvt_r.c b/misc/efgcvt_r.c index aa0eb8ca43..5b48ca5b9b 100644 --- a/misc/efgcvt_r.c +++ b/misc/efgcvt_r.c @@ -16,22 +16,24 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include "efgcvt_r.c" +#define ECVT_R ecvt_r +#define FCVT_R fcvt_r +#define __ECVT_R __ecvt_r +#define __FCVT_R __fcvt_r +#include <efgcvt-dbl-macros.h> +#include <efgcvt_r-template.c> #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) -# define cvt_symbol(symbol) \ - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ - APPEND (q, symbol), GLIBC_2_0); \ - weak_alias (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) +# define cvt_symbol(local, symbol) \ + cvt_symbol_1 (libc, local, APPEND (q, symbol), GLIBC_2_0); \ + weak_alias (local, symbol) # define cvt_symbol_1(lib, local, symbol, version) \ libc_hidden_def (local) \ compat_symbol (lib, local, symbol, version) #else -# define cvt_symbol(symbol) \ - cvt_symbol_1 (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) -# define cvt_symbol_1(local, symbol) \ +# define cvt_symbol(local, symbol) \ libc_hidden_def (local) \ weak_alias (local, symbol) #endif -cvt_symbol (fcvt_r); -cvt_symbol (ecvt_r); +cvt_symbol (__fcvt_r, fcvt_r); +cvt_symbol (__ecvt_r, ecvt_r); diff --git a/misc/qefgcvt.c b/misc/qefgcvt.c index ea48c6b48a..903bf93aa4 100644 --- a/misc/qefgcvt.c +++ b/misc/qefgcvt.c @@ -16,18 +16,24 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include "efgcvt.c" +#define ECVT qecvt +#define FCVT qfcvt +#define GCVT qgcvt +#define __ECVT __qecvt +#define __FCVT __qfcvt +#define __GCVT __qgcvt +#define __ECVT_R __qecvt_r +#define __FCVT_R __qfcvt_r +#include <efgcvt-ldbl-macros.h> +#include <efgcvt-template.c> #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) -# define cvt_symbol(symbol) \ - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ - APPEND (FUNC_PREFIX, symbol), GLIBC_2_4) -# define cvt_symbol_1(lib, local, symbol, version) \ - versioned_symbol (lib, local, symbol, version) +# define cvt_symbol(local, symbol) \ + versioned_symbol (libc, local, symbol, GLIBC_2_4) #else -# define cvt_symbol(symbol) \ - strong_alias (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) +# define cvt_symbol(local, symbol) \ + strong_alias (local, symbol) #endif -cvt_symbol(fcvt); -cvt_symbol(ecvt); -cvt_symbol(gcvt); +cvt_symbol (__qfcvt, qfcvt); +cvt_symbol (__qecvt, qecvt); +cvt_symbol (__qgcvt, qgcvt); diff --git a/misc/qefgcvt_r.c b/misc/qefgcvt_r.c index d2f36e6d2a..b790bf7837 100644 --- a/misc/qefgcvt_r.c +++ b/misc/qefgcvt_r.c @@ -17,21 +17,21 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include "efgcvt_r.c" +#define ECVT_R qecvt_r +#define FCVT_R qfcvt_r +#define __ECVT_R __qecvt_r +#define __FCVT_R __qfcvt_r +#include <efgcvt-ldbl-macros.h> +#include <efgcvt_r-template.c> #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) -# define cvt_symbol(symbol) \ - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ - APPEND (FUNC_PREFIX, symbol), GLIBC_2_4) -# define cvt_symbol_1(lib, local, symbol, version) \ +# define cvt_symbol(local, symbol) \ libc_hidden_def (local) \ - versioned_symbol (lib, local, symbol, version) + versioned_symbol (libc, local, symbol, GLIBC_2_4) #else -# define cvt_symbol(symbol) \ - cvt_symbol_1 (__APPEND (FUNC_PREFIX, symbol), APPEND (FUNC_PREFIX, symbol)) -# define cvt_symbol_1(local, symbol) \ +# define cvt_symbol(local, symbol) \ libc_hidden_def (local) \ weak_alias (local, symbol) #endif -cvt_symbol(fcvt_r); -cvt_symbol(ecvt_r); +cvt_symbol (__qfcvt_r, qfcvt_r); +cvt_symbol (__qecvt_r, qecvt_r);
From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> Changes since v1: - Adapted after patch v1 19/31 (remove hidden_* uses) was dropped. - Fixed unintended removal of cvt* symbols on alpha and s390x (now tested with build-many-glibcs.py). -- 8< -- This patch is to be squashed with the other n/5 refactoring patches. This patch replaces the use of the APPEND macro with one new macro being defined for each of the cvt* functions. This makes it easier to define functions names for IEEE long double on powerpc64le, e.g. __ecvtieee128. --- misc/efgcvt-template.c | 19 ++++++------------- misc/efgcvt.c | 28 ++++++++++++++++++---------- misc/efgcvt_r-template.c | 20 ++++++-------------- misc/efgcvt_r.c | 22 ++++++++++++---------- misc/qefgcvt.c | 28 +++++++++++++++++----------- misc/qefgcvt_r.c | 22 +++++++++++----------- 6 files changed, 70 insertions(+), 69 deletions(-)