[RFC] Fixing PR102685 and testsuite issues

[RFC] Fixing PR102685 and testsuite issues

[Harald Anlauf]

Dear Fortranners,

while working on a fix for PR102685, I encountered issues with the testsuite.
In this cases I think the testcase code is invalid, and it is correctly
rejected by e.g. Intel although currently accepted by gfortran.

Strangely enough.

(1) gfortran.dg/derived_constructor_char_1.f90

The constructor is shorter than the array component txt in DT t5.

Commit r0-101989.

@Tobias: can you comment?

(2) gfortran.dg/pr70931.f90

Committed by Richard Biener, fixing an ICE on an invalid testcase by Gerhard.
I think we safely can mark this one as invalid and emit an error, right?

(3) gfortran.dg/transfer_simplify_2.f90

The constructor has more elements than the array component in the DT and is
invalid.

Commit r0-80854.

@Paul: can you comment?


To proceed, I think we can treat (2) as erroneous and emit an error instead of
silently accepting the testcase, which has too many elements in the constructor.

Along these lines, I think we should always reject code having too many elements
in a constructor.  Is there agreement on this?  This would need fixing case (3).

Is there a reason to accept cases where the constructor is shorter than the array
component in the DT?  Some extension?  Shall we give a warning for these cases
instead of an error?  Based on -std flags?

Any constructive comment appreciated!

Thanks,
Harald

Re: [RFC] Fixing PR102685 and testsuite issues

[Tobias Burnus]

Dear all, hello Harald,

On 12.10.21 22:50, Harald Anlauf wrote:
> while working on a fix for PR102685, I encountered issues with the testsuite.
...
> (1) gfortran.dg/derived_constructor_char_1.f90
> The constructor is shorter than the array component txt in DT t5.
> Commit r0-101989.
> @Tobias: can you comment?
Simply change 'txt(4)' to 'txt(2)'.
> (2) gfortran.dg/pr70931.f90
> Committed by Richard Biener, fixing an ICE on an invalid testcase by Gerhard.
> I think we safely can mark this one as invalid and emit an error, right?

No - we still want to check the bug fix in gcc/dwarf2out.c's native_encode_initializer,
i.e.
-             if (val
+             if (val && fieldsize != 0

I think you should just change:
+   type(t), parameter :: z = t(1, [2])
to ... = t(1, [])

> (3) gfortran.dg/transfer_simplify_2.f90
>
> The constructor has more elements than the array component in the DT and is
> invalid.
>
> Commit r0-80854.

We had already before:
"Fixed invalid initialization" and "Fixed invalid array constructor."
for that file. Thus, changing it again to fix a bug is fine :-)

That looks like a combined pasto + thinko. In
    subroutine dt_to_integer1
     real, parameter       :: r1(4) = (/1.0_4,2.0_4,3.0_4,4.0_4/)
     type :: mytype
       integer(4) :: i(4)
       real(4) :: x(4)
     end type mytype

and in the next one:
   subroutine character16_to_dt
     character(16), parameter ::  c1 = "abcdefghijklmnop"
     character(16)            ::  c2 = c1
     type :: mytype
       real(4) :: x(2)
     end type mytype

     type (mytype), parameter :: dt1(2) = transfer (c1, mytype ((/1.0,2.0,3.0,4.0/)), 2)
...
     if (any (dt1(1)%x .ne. dt2(1)%x)) STOP 12
     if (any (dt1(2)%x .ne. dt2(2)%x)) STOP 13
   end subroutine character16_to_dt

I think you can simply remove the ',3.0, 4.0' in the 'dt1' line.

> Along these lines, I think we should always reject code having too many elements
> in a constructor.  Is there agreement on this?  This would need fixing case (3).
>
> Is there a reason to accept cases where the constructor is shorter than the array
> component in the DT?  Some extension?  Shall we give a warning for these cases
> instead of an error?  Based on -std flags?

At the moment I do not see any good reason for having
too many or too few elements. Also the testsuite use
indicates rather bugs and not intentional use.

Thus, I would simply give an error in either case.
I think that also matches the behavior of other
compilers such as ifort but I have not tested it.
(I think you alluded that you did test it with ifort
and it gave errors for both too long and too short
array constructors.)

I would change the testcases as mentioned, but maybe
it makes sense to add the invalid use in the new
testcase as some might exercise some corner cases
(size = 0, ...)?

As you had "derived_constructor_char_1.f90" in the list:

Having ["abc", "a", "ab cdefg"] is also invalid – as either the
strings have to be changed (space-padded on the right) to have
the same length or a typespec as in
   [ character(len=<n>) :: "....", ".." ]
has to be used. But I can see that it easily can happen that the
strings are of different length and the user then expects that
the compiler handles it correctly™, especially before the
typespec was permitted. (GCC attempts to do this, including
warnings/errors in that case.)

But for array elements, it seems to be unlikely that there is
size mismatch on purpose or by accidentally expecting that
the compiler corrects this.

> Any constructive comment appreciated!

I wonder why you are only interested in constructive ones ... ;-)

Thanks,

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: [RFC] Fixing PR102685 and testsuite issues

[Paul Richard Thomas]

Hi Harald,

The overfilled constructor in transfer_simplify_2.f90 is indeed an error.

The error is picked up correctly for arrays in
expr.c(gfc_check_conformance):3579 but not for array components.

Regards

Paul


On Thu, 14 Oct 2021 at 10:26, Tobias Burnus <tobias@codesourcery.com> wrote:

> Dear all, hello Harald,
>
> On 12.10.21 22:50, Harald Anlauf wrote:
> > while working on a fix for PR102685, I encountered issues with the
> testsuite.
> ...
> > (1) gfortran.dg/derived_constructor_char_1.f90
> > The constructor is shorter than the array component txt in DT t5.
> > Commit r0-101989.
> > @Tobias: can you comment?
> Simply change 'txt(4)' to 'txt(2)'.
> > (2) gfortran.dg/pr70931.f90
> > Committed by Richard Biener, fixing an ICE on an invalid testcase by
> Gerhard.
> > I think we safely can mark this one as invalid and emit an error, right?
>
> No - we still want to check the bug fix in gcc/dwarf2out.c's
> native_encode_initializer,
> i.e.
> -             if (val
> +             if (val && fieldsize != 0
>
> I think you should just change:
> +   type(t), parameter :: z = t(1, [2])
> to ... = t(1, [])
>
> > (3) gfortran.dg/transfer_simplify_2.f90
> >
> > The constructor has more elements than the array component in the DT and
> is
> > invalid.
> >
> > Commit r0-80854.
>
> We had already before:
> "Fixed invalid initialization" and "Fixed invalid array constructor."
> for that file. Thus, changing it again to fix a bug is fine :-)
>
> That looks like a combined pasto + thinko. In
>     subroutine dt_to_integer1
>      real, parameter       :: r1(4) = (/1.0_4,2.0_4,3.0_4,4.0_4/)
>      type :: mytype
>        integer(4) :: i(4)
>        real(4) :: x(4)
>      end type mytype
>
> and in the next one:
>    subroutine character16_to_dt
>      character(16), parameter ::  c1 = "abcdefghijklmnop"
>      character(16)            ::  c2 = c1
>      type :: mytype
>        real(4) :: x(2)
>      end type mytype
>
>      type (mytype), parameter :: dt1(2) = transfer (c1, mytype
> ((/1.0,2.0,3.0,4.0/)), 2)
> ...
>      if (any (dt1(1)%x .ne. dt2(1)%x)) STOP 12
>      if (any (dt1(2)%x .ne. dt2(2)%x)) STOP 13
>    end subroutine character16_to_dt
>
> I think you can simply remove the ',3.0, 4.0' in the 'dt1' line.
>
> > Along these lines, I think we should always reject code having too many
> elements
> > in a constructor.  Is there agreement on this?  This would need fixing
> case (3).
> >
> > Is there a reason to accept cases where the constructor is shorter than
> the array
> > component in the DT?  Some extension?  Shall we give a warning for these
> cases
> > instead of an error?  Based on -std flags?
>
> At the moment I do not see any good reason for having
> too many or too few elements. Also the testsuite use
> indicates rather bugs and not intentional use.
>
> Thus, I would simply give an error in either case.
> I think that also matches the behavior of other
> compilers such as ifort but I have not tested it.
> (I think you alluded that you did test it with ifort
> and it gave errors for both too long and too short
> array constructors.)
>
> I would change the testcases as mentioned, but maybe
> it makes sense to add the invalid use in the new
> testcase as some might exercise some corner cases
> (size = 0, ...)?
>
> As you had "derived_constructor_char_1.f90" in the list:
>
> Having ["abc", "a", "ab cdefg"] is also invalid – as either the
> strings have to be changed (space-padded on the right) to have
> the same length or a typespec as in
>    [ character(len=<n>) :: "....", ".." ]
> has to be used. But I can see that it easily can happen that the
> strings are of different length and the user then expects that
> the compiler handles it correctly™, especially before the
> typespec was permitted. (GCC attempts to do this, including
> warnings/errors in that case.)
>
> But for array elements, it seems to be unlikely that there is
> size mismatch on purpose or by accidentally expecting that
> the compiler corrects this.
>
> > Any constructive comment appreciated!
>
> I wonder why you are only interested in constructive ones ... ;-)
>
> Thanks,
>
> Tobias
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201,
> 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer:
> Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München;
> Registergericht München, HRB 106955
>


-- 
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein

Re: [RFC] Fixing PR102685 and testsuite issues

[Harald Anlauf]

Dear Tobias, Paul, all,

thanks for the feedback!

> Gesendet: Donnerstag, 14. Oktober 2021 um 11:26 Uhr
> Von: "Tobias Burnus" <tobias@codesourcery.com>

> > (1) gfortran.dg/derived_constructor_char_1.f90
> > The constructor is shorter than the array component txt in DT t5.
> > Commit r0-101989.
> > @Tobias: can you comment?
> Simply change 'txt(4)' to 'txt(2)'.

Will do.

> > (2) gfortran.dg/pr70931.f90
> > Committed by Richard Biener, fixing an ICE on an invalid testcase by Gerhard.
> > I think we safely can mark this one as invalid and emit an error, right?
> 
> No - we still want to check the bug fix in gcc/dwarf2out.c's native_encode_initializer,
> i.e.
> -             if (val
> +             if (val && fieldsize != 0
> 
> I think you should just change:
> +   type(t), parameter :: z = t(1, [2])
> to ... = t(1, [])

Will change to ... = t(1, [integer::]), as [] has no valid type.

> > (3) gfortran.dg/transfer_simplify_2.f90

> I think you can simply remove the ',3.0, 4.0' in the 'dt1' line.

Will do.

> > Along these lines, I think we should always reject code having too many elements
> > in a constructor.  Is there agreement on this?  This would need fixing case (3).
> >
> > Is there a reason to accept cases where the constructor is shorter than the array
> > component in the DT?  Some extension?  Shall we give a warning for these cases
> > instead of an error?  Based on -std flags?
> 
> At the moment I do not see any good reason for having
> too many or too few elements. Also the testsuite use
> indicates rather bugs and not intentional use.

Good.

> Thus, I would simply give an error in either case.
> I think that also matches the behavior of other
> compilers such as ifort but I have not tested it.
> (I think you alluded that you did test it with ifort
> and it gave errors for both too long and too short
> array constructors.)

Testcase with e.g. too few elements:

program p
  type t
     integer :: a(2)
  end type
  type(t), parameter :: x(2) = t([2])
end

Intel:

pr102685.f90(5): error #6595: The shape of a component in a structure constructor differs from the shape of the component of the derived type.   [T]
  type(t), parameter :: x(2) = t([2])
-------------------------------^
compilation aborted for pr102685.f90 (code 1)

NAG:

NAG Fortran Compiler Release 7.0(Yurakucho) Build 7009
Warning: pr102685.f90, line 5: Unused PARAMETER X
Error: pr102685.f90, line 5: Dimension 1 of value for array A has extent 1 instead of 2
Errors in declarations, no further processing for P

NVIDIA:

NVFORTRAN-S-0066-Too few data constants in initialization statement (pr102685.f90: 5)
NVFORTRAN-S-0066-Too few data constants in initialization statement (pr102685.f90: 5)
NVFORTRAN-S-0066-Too few data constants in initialization statement (pr102685.f90: 5)
NVFORTRAN-S-0066-Too few data constants in initialization statement (pr102685.f90: 5)
  0 inform,   0 warnings,   4 severes, 0 fatal for p

Now that one is really convincing!

> I would change the testcases as mentioned, but maybe
> it makes sense to add the invalid use in the new
> testcase as some might exercise some corner cases
> (size = 0, ...)?

Will do.

> As you had "derived_constructor_char_1.f90" in the list:
> 
> Having ["abc", "a", "ab cdefg"] is also invalid – as either the
> strings have to be changed (space-padded on the right) to have
> the same length or a typespec as in
>    [ character(len=<n>) :: "....", ".." ]
> has to be used. But I can see that it easily can happen that the
> strings are of different length and the user then expects that
> the compiler handles it correctly™, especially before the
> typespec was permitted. (GCC attempts to do this, including
> warnings/errors in that case.)

There's at least one related nasty PR on my list...

> > Any constructive comment appreciated!
> 
> I wonder why you are only interested in constructive ones ... ;-)

... that I got distracted from by the almost constant inflow of
PRs with alleged regressions...

Thanks,
Harald

> Thanks,
> 
> Tobias
> 
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
>