Re: [PATCH, Fortran] pad char to int conversions with spaces instead of zeros (legacy)

[Mark Eggleston <mark.eggleston@codethink.co.uk>]

On 04/12/2018 15:11, Jakub Jelinek wrote:
> On Tue, Dec 04, 2018 at 02:47:25PM +0000, Mark Eggleston wrote:
>> Here is a patch to considered for incorporation into gfortran adding to its
>> legacy support. It pads character to integer conversions using spaces
>> instead of zeros when enabled.
>>
>> The pad character is 'undefined' or 'processor dependent' depending on which
>> standard you read. This makes it 0x20 which matches the Oracle Fortran
>> compiler.
> Trying fortran.godbolt.org, I think ifort pads this with spaces too.
>
>> Enabled using -fdec-pad-with-spaces and -fdec.
> Just a couple of random comments.
> -fdec-pad-with-spaces option name doesn't look right, because it doesn't say
> what the option affects.  So perhaps have transfer in the option name?
> Wouldn't it be better to allow specifying whatever character you want to pad
> with, so that users can choose something even different?
Fritz Reese agrees with you here and suggests -ftransfer-pad-char= with 
-fdec setting it to 0x20.
>
>> --- a/gcc/fortran/simplify.c
>> +++ b/gcc/fortran/simplify.c
>> @@ -7830,7 +7830,7 @@ gfc_simplify_transfer (gfc_expr *source, gfc_expr *mold, gfc_expr *size)
>>     /* Allocate the buffer to store the binary version of the source.  */
>>     buffer_size = MAX (source_size, result_size);
>>     buffer = (unsigned char*)alloca (buffer_size);
>> -  memset (buffer, 0, buffer_size);
>> +  memset (buffer, (flag_dec_pad_with_spaces ? 0x20 : 0x0), buffer_size);
> This affects just the simplification when the argument is a known constant.
> Shouldn't we handle it also when it is not a constant?  Like I've tried:
> program test
>    integer(kind=8) :: a, b, c
>    character(len=4) :: e
>    a = transfer("ABCE", 1_8)
>    e = "ABCE"
>    b = transfer(e, 1_8)
>    c = transfer("ABCE    ", 1_8)
>    print *, a, b, c
> end
> and for a the result is on little-endian indeed z'45434241', for b
> the upper 32 bits are completely random:
>              D.3854 = 4;
>              D.3856 = 8;
>              _1 = MIN_EXPR <D.3856, D.3854>;
>              _2 = MAX_EXPR <_1, 0>;
>              _3 = (unsigned long) _2;
>              __builtin_memcpy (&transfer.0, &e, _3);
>              transfer.3_4 = transfer.0;
>              b = transfer.3_4;
> and for c it is the padding with zeros I assume you want for -fdec.
Clearly insufficient testing let this through. The padding should be 
done for both literals and variables.
> So, what does Oracle fortran or ifort do for this b case above?
Don't have access to either of those compilers. It may be possible to 
check the ifort compiler.
>
>> --- /dev/null
>> +++ b/gcc/testsuite/gfortran.dg/dec-pad-with-spaces-1.f90
>> @@ -0,0 +1,17 @@
>> +! { dg-do run }
>> +! { dg-options "-fdec-pad-with-spaces" }
>> +!
>> +! Test case contributed by Mark Eggleston <mark.eggleston@codethink.com>
>> +
>> +program test
>> +  integer(kind=8) :: a
>> +  a = transfer("ABCE", 1_8)
>> +  ! If a has not been converted into big endian
>> +  ! or little endian integer it has failed.
>> +  if ((a.ne.int(z'4142434520202020',kind=8)).and. &
>> +      (a.ne.int(z'2020202045434241',kind=8))) then
> The tests look too much big-endian vs. little-endian dependent and
> ascii dependent.  We have pdp-endian and doesn't s390x TPF use EBCDIC?
I hadn't considered those.
>
> Wouldn't it be better to compare transfer("ABCE", 1_8) with transfer("ABCE    ", 1_8)
> ?
That simplifies things.
>
> 	Jakub

Thanks for the feedback. I inherited this patch, my addition of 
-fdec-pad-with-spaces and improved testing were insufficient.

Will resubmit the patch after taking these issues into account.

Mark.

-- 
https://www.codethink.co.uk/privacy.html