[v3,01/11] ldbl-128ibm-compat: Fix selection of GNU and ISO C99 scanf
Commit Message
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
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
@@ -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)
@@ -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
@@ -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