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

[Mark Eggleston <mark dot eggleston at codethink dot co dot 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