diff mbox

[v3,01/11] ldbl-128ibm-compat: Fix selection of GNU and ISO C99 scanf

Message ID 20191203170540.18428-2-gabriel@inconstante.net.br
State New
Headers show

Commit Message

Gabriel F. T. Gomes Dec. 3, 2019, 5:05 p.m. UTC
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(-)

Comments

Paul E Murphy Dec. 9, 2019, 4:45 p.m. UTC | #1
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.
Zack Weinberg Dec. 9, 2019, 4:49 p.m. UTC | #2
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
Paul E Murphy Dec. 9, 2019, 5:08 p.m. UTC | #3
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?
Gabriel F. T. Gomes Dec. 9, 2019, 6:16 p.m. UTC | #4
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.
Paul E Murphy Dec. 9, 2019, 8:31 p.m. UTC | #5
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.
Gabriel F. T. Gomes Dec. 12, 2019, 1:42 p.m. UTC | #6
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 mbox

Patch

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