Message ID | 20191203170540.18428-2-gabriel@inconstante.net.br |
---|---|
State | New |
Headers | show |
Series | Add IEEE long double <-> string functions for powerpc64le | expand |
On 12/3/19 11:05 AM, Gabriel F. T. Gomes wrote: > From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> > > New since v2. > > -- 8< -- > Since commit > > commit 03992356e6fedc5a5e9d32df96c1a2c79ea28a8f > Author: Zack Weinberg <zackw@panix.com> > Date: Sat Feb 10 11:58:35 2018 -0500 > > Use C99-compliant scanf under _GNU_SOURCE with modern compilers. > > the selection of the GNU versions of scanf functions requires both > _GNU_SOURCE and -std=c89. This patch changes the tests in > ldbl-128ibm-compat so that they actually test the GNU versions (without > this change, the redirection to the ISO C99 version always happens, so > GNU versions of the new implementation (e.g. __scanfieee128) were left > untested). Good catch Zach, thanks. I wasn't aware of this behavior and verified it too. LGTM.
On Mon, Dec 9, 2019 at 11:46 AM Paul E Murphy <murphyp@linux.ibm.com> wrote: > On 12/3/19 11:05 AM, Gabriel F. T. Gomes wrote: > > From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> > > > > New since v2. > > > > -- 8< -- > > Since commit > > > > commit 03992356e6fedc5a5e9d32df96c1a2c79ea28a8f > > Author: Zack Weinberg <zackw@panix.com> > > Date: Sat Feb 10 11:58:35 2018 -0500 > > > > Use C99-compliant scanf under _GNU_SOURCE with modern compilers. > > > > the selection of the GNU versions of scanf functions requires both > > _GNU_SOURCE and -std=c89. This patch changes the tests in > > ldbl-128ibm-compat so that they actually test the GNU versions (without > > this change, the redirection to the ISO C99 version always happens, so > > GNU versions of the new implementation (e.g. __scanfieee128) were left > > untested). > > Good catch Zach, thanks. Credit is actually due to Gabriel; if I understand correctly, I _introduced_ a bug here! zw
On 12/9/19 10:49 AM, Zack Weinberg wrote: > On Mon, Dec 9, 2019 at 11:46 AM Paul E Murphy <murphyp@linux.ibm.com> wrote: >> On 12/3/19 11:05 AM, Gabriel F. T. Gomes wrote: >>> From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> >>> >>> New since v2. >>> >>> -- 8< -- >>> Since commit >>> >>> commit 03992356e6fedc5a5e9d32df96c1a2c79ea28a8f >>> Author: Zack Weinberg <zackw@panix.com> >>> Date: Sat Feb 10 11:58:35 2018 -0500 >>> >>> Use C99-compliant scanf under _GNU_SOURCE with modern compilers. >>> >>> the selection of the GNU versions of scanf functions requires both >>> _GNU_SOURCE and -std=c89. This patch changes the tests in >>> ldbl-128ibm-compat so that they actually test the GNU versions (without >>> this change, the redirection to the ISO C99 version always happens, so >>> GNU versions of the new implementation (e.g. __scanfieee128) were left >>> untested). >> >> Good catch Zach, thanks. > > Credit is actually due to Gabriel; if I understand correctly, I > _introduced_ a bug here! > > zw > I stand corrected. I wonder how frail some of these tests might become as they age. Is framework available today to verify tests are linking against the desired variants of a symbol?
On Mon, 09 Dec 2019, Paul E Murphy wrote: > >On 12/9/19 10:49 AM, Zack Weinberg wrote: >> >> Credit is actually due to Gabriel; if I understand correctly, I >> _introduced_ a bug here! Zack's patch did the right thing... However, I started working on powerpc's IEEE long double implementation before his patch landed on master and I didn't notice the change in default behaviour. The potential bug never really showed up, because ldbl-128ibm-compat is not yet in the Implies file, so the __*ieee128 functions were never really exposed. >I stand corrected. I wonder how frail some of these tests might become >as they age. Is framework available today to verify tests are linking >against the desired variants of a symbol? The tests in the subsequent patch (2/11) try to make that better, by trying to read a long double value with %as when in ISO C99 mode. Now I'm wondering that it should try to read a string (and allocate it automatically) in the -D_GNU_SOURCE -std=c89 case, i.e.: also use %as, but expect a different output. Since each case should produce different outputs (even if the input is the same), I think that will help verify that the correct symbol gets selected.
On 12/9/19 12:16 PM, Gabriel F. T. Gomes wrote: > On Mon, 09 Dec 2019, Paul E Murphy wrote: >> >> On 12/9/19 10:49 AM, Zack Weinberg wrote: >>> >>> Credit is actually due to Gabriel; if I understand correctly, I >>> _introduced_ a bug here! > > Zack's patch did the right thing... However, I started working on > powerpc's IEEE long double implementation before his patch landed on > master and I didn't notice the change in default behaviour. The potential > bug never really showed up, because ldbl-128ibm-compat is not yet in the > Implies file, so the __*ieee128 functions were never really exposed. > >> I stand corrected. I wonder how frail some of these tests might become >> as they age. Is framework available today to verify tests are linking >> against the desired variants of a symbol? > > The tests in the subsequent patch (2/11) try to make that better, by > trying to read a long double value with %as when in ISO C99 mode. Now I'm > wondering that it should try to read a string (and allocate it > automatically) in the -D_GNU_SOURCE -std=c89 case, i.e.: also use %as, but > expect a different output. > > Since each case should produce different outputs (even if the input is the > same), I think that will help verify that the correct symbol gets selected. > There are two tests wrapped in each: One to verify the headers select the appropriate variant, secondly verifying the long double implementation is correct. The former is sometimes more complicated than ibm128 or ieee128. I am not suggesting this work be included in this patchset, but would it be approaching something like `assert (&(scanf) == &FPTR_EXPECTED (scanf))` to this and similar printf tests (e.g fortified printf)? Such could quickly tell us if we need to update our tests.
On Mon, 09 Dec 2019, Paul E Murphy wrote: >There are two tests wrapped in each: One to verify the headers select >the appropriate variant, secondly verifying the long double >implementation is correct. The former is sometimes more complicated >than ibm128 or ieee128. Right. >I am not suggesting this work be included in this patchset, but would it >be approaching something like `assert (&(scanf) == &FPTR_EXPECTED >(scanf))` to this and similar printf tests (e.g fortified printf)? Such >could quickly tell us if we need to update our tests. Sounds good, maybe we could write something similar to elf/tst-addr1.c [1], with hardcoded function names. [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/tst-addr1.c;h=68ff74aabd4dbe5b4dc1c98ebd2df2f66e080890;hb=HEAD
diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/Makefile b/sysdeps/ieee754/ldbl-128ibm-compat/Makefile index b54eb04f22..21332bfbbb 100644 --- a/sysdeps/ieee754/ldbl-128ibm-compat/Makefile +++ b/sysdeps/ieee754/ldbl-128ibm-compat/Makefile @@ -75,14 +75,14 @@ CFLAGS-test-obstack-chk-ieee128.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi CFLAGS-test-obstack-chk-ibm128.c += -mabi=ibmlongdouble -Wno-psabi tests-internal += test-scanf-ieee128 test-scanf-ibm128 -CFLAGS-test-scanf-ieee128.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi -CFLAGS-test-scanf-ibm128.c += -mabi=ibmlongdouble -Wno-psabi +CFLAGS-test-scanf-ieee128.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi -std=c89 -D_GNU_SOURCE +CFLAGS-test-scanf-ibm128.c += -mabi=ibmlongdouble -Wno-psabi -std=c89 -D_GNU_SOURCE $(objpfx)test-scanf-ieee128: gnulib-tests += $(f128-loader-link) tests-internal += test-wscanf-ieee128 test-wscanf-ibm128 -CFLAGS-test-wscanf-ieee128.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi -CFLAGS-test-wscanf-ibm128.c += -mabi=ibmlongdouble -Wno-psabi +CFLAGS-test-wscanf-ieee128.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi -std=c89 -D_GNU_SOURCE +CFLAGS-test-wscanf-ibm128.c += -mabi=ibmlongdouble -Wno-psabi -std=c89 -D_GNU_SOURCE $(objpfx)test-wscanf-ieee128: gnulib-tests += $(f128-loader-link) diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/test-scanf-ldbl-compat.c b/sysdeps/ieee754/ldbl-128ibm-compat/test-scanf-ldbl-compat.c index 0759d8afab..116808dffd 100644 --- a/sysdeps/ieee754/ldbl-128ibm-compat/test-scanf-ldbl-compat.c +++ b/sysdeps/ieee754/ldbl-128ibm-compat/test-scanf-ldbl-compat.c @@ -1,3 +1,7 @@ +/* Include stdio.h from libio/, because include/stdio.h and -std=c89 do + not work together. */ +#include <libio/stdio.h> + #define CHAR char #define L(x) x #define FSCANF fscanf diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/test-wscanf-ldbl-compat.c b/sysdeps/ieee754/ldbl-128ibm-compat/test-wscanf-ldbl-compat.c index e93cf3b9bd..4b8fd1b442 100644 --- a/sysdeps/ieee754/ldbl-128ibm-compat/test-wscanf-ldbl-compat.c +++ b/sysdeps/ieee754/ldbl-128ibm-compat/test-wscanf-ldbl-compat.c @@ -1,3 +1,7 @@ +/* Include stdio.h from libio/, because include/stdio.h and -std=c89 do + not work together. */ +#include <libio/stdio.h> + #define CHAR wchar_t #define L(x) L##x #define FSCANF fwscanf
From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> New since v2. -- 8< -- Since commit commit 03992356e6fedc5a5e9d32df96c1a2c79ea28a8f Author: Zack Weinberg <zackw@panix.com> Date: Sat Feb 10 11:58:35 2018 -0500 Use C99-compliant scanf under _GNU_SOURCE with modern compilers. the selection of the GNU versions of scanf functions requires both _GNU_SOURCE and -std=c89. This patch changes the tests in ldbl-128ibm-compat so that they actually test the GNU versions (without this change, the redirection to the ISO C99 version always happens, so GNU versions of the new implementation (e.g. __scanfieee128) were left untested). Tested for powerpc64le. --- sysdeps/ieee754/ldbl-128ibm-compat/Makefile | 8 ++++---- .../ieee754/ldbl-128ibm-compat/test-scanf-ldbl-compat.c | 4 ++++ .../ieee754/ldbl-128ibm-compat/test-wscanf-ldbl-compat.c | 4 ++++ 3 files changed, 12 insertions(+), 4 deletions(-)