Re: [PATCH v2 01/30] ldbl-128ibm-compat: Add regular character printing functions

["Gabriel F. T. Gomes" <gabriel@inconstante.net.br>]

Hi, Paul,

On Tue, 29 Oct 2019, Paul E Murphy wrote:

>On 10/25/19 10:33 AM, Gabriel F. T. Gomes wrote:
>>
>> diff --git a/elf/tst-addr1.c b/elf/tst-addr1.c
>> index 68ff74aabd..ee81acda5b 100644
>> --- a/elf/tst-addr1.c
>> +++ b/elf/tst-addr1.c
>> @@ -19,7 +19,12 @@ do_test (void)
>>   		rather than in the binary.  printf and _IO_printf
>>   		are aliased and which one comes first in the
>>   		hash table is up to the linker.  */
>> -	     && strcmp (i.dli_sname, "_IO_printf") != 0);
>> +	     && strcmp (i.dli_sname, "_IO_printf") != 0
>> +	     /* On architectures where long double with IEEE binary128
>> +		format is available as a third option (initially, true
>> +		for powerpc64le), printf may be redirected to
>> +		__printfieee128.  */
>> +	     && strcmp (i.dli_sname, "__printfieee128") != 0);  
>
>Should there be a guard against this test for architectures which will 
>never support this symbol?

Yeah, sounds good so that we avoid unintended redirections.  On the other
hand, this is also a residue from our initial development effort, when we
added -mabi=ieeelongdouble to a lot more files, than what I have today.
Today, this test case is not built with -mabi=ieeelongdouble, so I think
this hunk should simply go away.

>> diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/test-printf-ieee128.c b/sysdeps/ieee754/ldbl-128ibm-compat/test-printf-ieee128.c
>> new file mode 100644
>> index 0000000000..5de4ea3e7f
>> --- /dev/null
>> +++ b/sysdeps/ieee754/ldbl-128ibm-compat/test-printf-ieee128.c
>> @@ -0,0 +1 @@
>> +#include <test-printf-ldbl-compat.c>
>> diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/test-printf-ldbl-compat.c b/sysdeps/ieee754/ldbl-128ibm-compat/test-printf-ldbl-compat.c  
>...
>> +  long double ld = -1;  
>
>Is this the best value to use for compat tests? Would a value which 
>produces unique output for the respective format if the wrong compiler 
>flags are used? Or, maybe an extra header in *-ibm128.c and *-ieee128.c 
>variants to sanity check __LDBL_MANT_DIG__?

My rationale for using this value is that it makes it simple to debug when
something goes wrong.  Since the implementation for these functions reuse
the code paths added for _Float128, I didn't think it would be useful to
test for more values again (the _Float128 implementation already checks
for many values).  This test is telling me that the parameters are being
passed and read correctly (on the powerpc64le case, that they are going
through VSX registers, as opposed to two FP registers for IBM long double).

>> +   This is used by the Versions and math_ldbl_opt.h files in
>> +   sysdeps/ieee754/ldbl-128ibm-compat/.  It gives the ABI version where
>> +   long double == ibm128 was replaced with long double == _Float128
>> +   for libm *l functions and libc functions using long double.  */
>> +
>> +#define LDBL_IBM128_VERSION		GLIBC_2.31
>> +#define LDBL_IBM128_COMPAT_VERSION	GLIBC_2_31
>
>Should this part of the change be held off until all ldbl == ieee128 
>changes are in?

I guess you're right. I moved it to the last commit, which actually adds
sysdeps/ieee754/ldbl-128ibm-compat to the Implies file.


Thanks,
Gabriel