On 3/27/24 1:42 PM, Harald Anlauf wrote:
> Dear all,
>
> the attached patch fixes a 10+ regression for cases where a
> derived type with a pointer component is used in a DATA statement.
> The failure looked obscure, see testcase comments, and pointed
> to a possible issue in the resolution (order). For the failing
> test, the target variable was seen with ts.type == BT_PROCEDURE
> instead of its actual type. For this reason, I restricted the
> fixup as much as possible.
>
> For details, please see the commit message.
>
> Testcase cross-checked with NAG.
>
> Regtested on x86_64-pc-linux-gnu. OK for mainline?
>
> If this fix survives broader testing, I would like to backport.
>
> Thanks,
> Harald
>
> P.S.: while trying to extend coverage of conforming code, I had
> much fun also with other compilers (e.g. NAG panicking...)
>
OK, for trunk and we will see how it survives for a bit before back port.
Jerry -
[-- Attachment #1: Type: text/plain, Size: 757 bytes --] Dear all, the attached patch fixes a 10+ regression for cases where a derived type with a pointer component is used in a DATA statement. The failure looked obscure, see testcase comments, and pointed to a possible issue in the resolution (order). For the failing test, the target variable was seen with ts.type == BT_PROCEDURE instead of its actual type. For this reason, I restricted the fixup as much as possible. For details, please see the commit message. Testcase cross-checked with NAG. Regtested on x86_64-pc-linux-gnu. OK for mainline? If this fix survives broader testing, I would like to backport. Thanks, Harald P.S.: while trying to extend coverage of conforming code, I had much fun also with other compilers (e.g. NAG panicking...) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: pr114474.diff --] [-- Type: text/x-patch, Size: 4619 bytes --] From d5fda38243a22e1aef4367653d92521e53f2000d Mon Sep 17 00:00:00 2001 From: Harald Anlauf <anlauf@gmx.de> Date: Wed, 27 Mar 2024 21:18:04 +0100 Subject: [PATCH] Fortran: fix DATA and derived types with pointer components [PR114474] When matching actual arguments in match_actual_arg, these are initially treated as a possible dummy procedure, assuming that the correct type is determined later. This resolution could fail when the procedure is a derived type constructor with a pointer component and appears in a DATA statement, where the pointer shall be associated with an initial data target. Check for those cases where the type obviously has not been resolved yet, and which were missed because there was no component reference. gcc/fortran/ChangeLog: PR fortran/114474 * primary.cc (gfc_variable_attr): Catch variables used in structure constructors within DATA statements that are still tagged with a temporary type BT_PROCEDURE from match_actual_arg and which have the target attribute, and fix their typespec. gcc/testsuite/ChangeLog: PR fortran/114474 * gfortran.dg/data_pointer_3.f90: New test. --- gcc/fortran/primary.cc | 12 +++ gcc/testsuite/gfortran.dg/data_pointer_3.f90 | 77 ++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 gcc/testsuite/gfortran.dg/data_pointer_3.f90 diff --git a/gcc/fortran/primary.cc b/gcc/fortran/primary.cc index 0ab69bb9dce..5dd6875a4a6 100644 --- a/gcc/fortran/primary.cc +++ b/gcc/fortran/primary.cc @@ -2804,6 +2804,18 @@ gfc_variable_attr (gfc_expr *expr, gfc_typespec *ts) if (ts != NULL && expr->ts.type == BT_UNKNOWN) *ts = sym->ts; + /* Catch left-overs from match_actual_arg, where an actual argument of a + procedure is given a temporary ts.type == BT_PROCEDURE. The fixup is + needed for structure constructors in DATA statements, where a pointer + is associated with a data target, and the argument has not been fully + resolved yet. Components references are dealt with further below. */ + if (ts != NULL + && expr->ts.type == BT_PROCEDURE + && expr->ref == NULL + && attr.flavor != FL_PROCEDURE + && attr.target) + *ts = sym->ts; + has_inquiry_part = false; for (ref = expr->ref; ref; ref = ref->next) if (ref->type == REF_INQUIRY) diff --git a/gcc/testsuite/gfortran.dg/data_pointer_3.f90 b/gcc/testsuite/gfortran.dg/data_pointer_3.f90 new file mode 100644 index 00000000000..f0325cd5bcb --- /dev/null +++ b/gcc/testsuite/gfortran.dg/data_pointer_3.f90 @@ -0,0 +1,77 @@ +! { dg-do compile } +! PR fortran/114474 - DATA and derived types with pointer components + +program pr114474 + implicit none + integer, target :: ii = 42 ! initial data target + + integer, target :: jj = 24 + integer, pointer :: qq => jj + ! ii and jj resolve slightly differently when the data statement below + ! is reached, as jj is resolved outside the structure constructor first + + type t + integer, pointer :: h + end type t + + integer, target :: kk(7) = 23 + integer, pointer :: ll(:) => kk + + type t1 + integer :: m(7) + end type t1 + + type(t) :: x1, x2, x3, x4, x5 + type(t), parameter :: z1 = t(null()) + + type(t1), target :: tt = t1([1,2,3,4,5,6,7]) + type(t1), parameter :: vv = t1(22) + type(t1) :: w1, w2 + integer, pointer :: p1(:) => tt% m + + data x1 / t(null()) / + data x2 / t(ii) / ! ii is initial data target + data x3 / t(jj) / ! jj is resolved differently... + data x4 / t(tt%m(3)) / ! pointer association with 3rd element + + data w1 / t1(12) / + data w2 / t1(vv%m) / + + if ( associated (x1% h)) stop 1 + if (.not. associated (x2% h)) stop 2 + if (.not. associated (x3% h)) stop 3 + if (.not. associated (x4% h)) stop 4 + if (x2% h /= 42) stop 5 + if (x3% h /= 24) stop 6 + if (x4% h /= 3) stop 7 + + if (any (w1%m /= 12 )) stop 8 + if (any (w2%m /= vv%m)) stop 9 +end + + +subroutine sub + implicit none + + interface + real function myfun (x) + real, intent(in) :: x + end function myfun + end interface + + type u + procedure(myfun), pointer, nopass :: p + end type u + + type(u) :: u3 = u(null()) + type(u), parameter :: u4 = u(null()) + type(u) :: u1, u2 + + data u1 / u(null()) / + data u2 / u(myfun) / +end + +real function myfun (x) + real, intent(in) :: x + myfun = x +end function myfun -- 2.35.3
On Wed, Mar 27, 2024 at 04:30:42PM +0100, Mikael Morin wrote:
> Hell(o),
>
> it didn't take long for my recent patch for PR111781 to show a regression.
> The fix proposed here is actually the one Harald posted in the PR.
> I can't imagine a case where it wouldn't do the right thing.
> Regression tested on x86_64-pc-linux-gnu.
> OK for master?
>
Yes. Thank you (and Harald) for the quick fix.
--
Steve
Hell(o), it didn't take long for my recent patch for PR111781 to show a regression. The fix proposed here is actually the one Harald posted in the PR. I can't imagine a case where it wouldn't do the right thing. Regression tested on x86_64-pc-linux-gnu. OK for master? -- >8 -- The patch fixing PR111781 made the check of specification expressions more restrictive, disallowing local variables in specification expressions of dummy arguments. PR114475 showed an example where that change regressed, disallowing in submodules expressions that had been allowed in the parent module. In submodules indeed, the hierarchy of namespaces inherited from the parent module is not reproduced so the host-association of symbols can't be recognized by checking the nesting of namespaces. This change fixes the problem by allowing in specification expressions all the symbols in a submodule that are inherited from the parent module. PR fortran/111781 PR fortran/114475 gcc/fortran/ChangeLog: * expr.cc (check_restricted): In submodules, allow variables host- associated from the parent module. gcc/testsuite/ChangeLog: * gfortran.dg/spec_expr_10.f90: New test. Co-authored-by: Harald Anlauf <anlauf@gmx.de> --- gcc/fortran/expr.cc | 1 + gcc/testsuite/gfortran.dg/spec_expr_10.f90 | 46 ++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 gcc/testsuite/gfortran.dg/spec_expr_10.f90 diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc index 9a042cd7040..09d1ebd95d2 100644 --- a/gcc/fortran/expr.cc +++ b/gcc/fortran/expr.cc @@ -3517,6 +3517,7 @@ check_restricted (gfc_expr *e) if (e->error || sym->attr.in_common || sym->attr.use_assoc + || sym->attr.used_in_submodule || sym->attr.dummy || sym->attr.implied_index || sym->attr.flavor == FL_PARAMETER diff --git a/gcc/testsuite/gfortran.dg/spec_expr_10.f90 b/gcc/testsuite/gfortran.dg/spec_expr_10.f90 new file mode 100644 index 00000000000..287b5a8d6cc --- /dev/null +++ b/gcc/testsuite/gfortran.dg/spec_expr_10.f90 @@ -0,0 +1,46 @@ +! { dg-do compile } +! +! PR fortran/114475 +! The array specification of PP in OL_EVAL used to be rejected in the submodule +! because the compiler was not able to see the host-association of N_EXTERNAL +! there. +! +! Contributed by Jürgen Reuter <juergen.reuter@desy.de>. + +module t1 + use, intrinsic :: iso_c_binding + implicit none + private + public :: t1_t + integer :: N_EXTERNAL = 0 + + type :: t1_t + contains + procedure :: set_n_external => t1_set_n_external + end type t1_t + + abstract interface + subroutine ol_eval (id, pp, emitter) bind(C) + import + real(kind = c_double), intent(in) :: pp(5 * N_EXTERNAL) + end subroutine ol_eval + end interface + interface + module subroutine t1_set_n_external (object, n) + class(t1_t), intent(inout) :: object + integer, intent(in) :: n + end subroutine t1_set_n_external + end interface + +end module t1 + +submodule (t1) t1_s + implicit none +contains + module subroutine t1_set_n_external (object, n) + class(t1_t), intent(inout) :: object + integer, intent(in) :: n + N_EXTERNAL = n + end subroutine t1_set_n_external + +end submodule t1_s -- 2.43.0
Hi Jerry,
Am 26.03.24 um 04:18 schrieb Jerry D:
> Hi all,
>
> There has been a bit of discussio on which way to go on this.
>
> I took a look today and this trivial patch gives the behavior concluded
> on Fortran Discourse. See the bugzilla for all the relevant information.
>
> Regresion tested on x86-64.
>
> I will do the appropriate changelog.
>
> OK for trunk?
>
> Attached is a new test case and the patch here:
>
> diff --git a/libgfortran/io/file_pos.c b/libgfortran/io/file_pos.c
> index 2bc05b293f8..d169961f997 100644
> --- a/libgfortran/io/file_pos.c
> +++ b/libgfortran/io/file_pos.c
> @@ -352,7 +352,6 @@ st_endfile (st_parameter_filepos *fpp)
> dtp.common = fpp->common;
> memset (&dtp.u.p, 0, sizeof (dtp.u.p));
> dtp.u.p.current_unit = u;
> - next_record (&dtp, 1);
> }
>
> unit_truncate (u, stell (u->s), &fpp->common);
this is OK from my side.
Given the discussion on "dg-do run", wouldn't this be a perfect
example where it is sufficient to run the testcase just once?
The change is in libgfortran, not in the frontend or middle-end.
Thanks for the patch!
Harald
Le 25/03/2024 à 23:30, Harald Anlauf a écrit : >> On 3/25/24 12:53 PM, Harald Anlauf wrote: >>>>>>> (...) >>>>>> >>>>>> One failure after fixing all the spaces ( sed is our friend ). >>>>>> >>>>>> FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times optimized "_gfortran_matmul" 0 >>> >>> This does actually point to an issue with the testcase: >>> it only works properly with optimization enabled. >>> >>> Manual inspection of this test and the expected dump suggests >>> that e.g. -O1 could have been added to the dg-options directive. >>> >>> Shouldn't we fix at least the dg-options of that testcase? >>> For what it's worth, the scan-tree-dump-times directive can be conditionalized on target __OPTIMIZE__, so that it's active with optimization and inactive with -O0. This would make the test pass regardless of compiler flags. Of course if the goal is lowering the test execution time, it wouldn't help. (...) > > I find it somewhat unsatisfactory though, to have a behavior of the > testsuite harness that is so intranparent. > Agreed.
On Tue, Mar 26, 2024 at 2:24 AM Jerry D <jvdelisle2@gmail.com> wrote: > > On 3/25/24 3:30 PM, Harald Anlauf wrote: > >> On 3/25/24 12:53 PM, Harald Anlauf wrote: > >>>>>>> I noticed by chance that we have quite a lot of improperly specified do-do > >>>>>>> directives in the testsuite. > >>>>>>> > >>>>>>> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l > >>>>>>> 83 > >>>>>>> > >>>>> > >>>>> I think this is on purpose. > >>>>> The idea to use a "feature" in dejagnu to only iterate once and not > >>>>> over all possible options. So execution time can be lowered a bit. > >>>>> > >>>>> But I don't know if this hack still works, it definitely did work some years ago. > >>>>> > >>>>> Cheers, > >>>>> Manfred > >>> > >>> Is this "feature" documented somewhere? I don't see it on > >>> > >>> https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gccint/Directives.html > >>> > >>> Given that the dg-directives are important and possibly fragile, > >>> and since we had issues in the past, can we check that a test > >>> that was added works the way intended? > >>> > >>>>>>> Note that with two blanks instead of just one a testcase does not get executed. > >>>>>>> > >>>>>>> Does anybody want to earn the honors to change the directives and > >>>>>>> check for "fallout" in the testsuite? > >>>>>>> > >>>>>>> Cheers, > >>>>>>> Harald > >>>>>>> > >>>>>> > >>>>>> One failure after fixing all the spaces ( sed is our friend ). > >>>>>> > >>>>>> FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times optimized "_gfortran_matmul" 0 > >>> > >>> This does actually point to an issue with the testcase: > >>> it only works properly with optimization enabled. > >>> > >>> Manual inspection of this test and the expected dump suggests > >>> that e.g. -O1 could have been added to the dg-options directive. > >>> > >>> Shouldn't we fix at least the dg-options of that testcase? > >>> > >>> Cheers, > >>> Harald > >>> > >> > >> I restored the one test that appeared to fail so that it had the two > >> spaces 'trick'. When run in the test suite, it is compiled with -O > >> which does invoke the optimization. I manually checked the tree dump > >> with this option and it indeed has all the _gfortran_matmul calls removed. > >> > >> I am inclined to leave these all untouched with the two spaces in place. > >> > >> From the test log: > >> > >> PASS: gfortran.dg/inline_matmul_1.f90 -O execution test > >> PASS: gfortran.dg/inline_matmul_1.f90 -O scan-tree-dump-times > >> optimized "_gfortran_matmul" 0 > > > > Alright, then leave it that way. > > > > I find it somewhat unsatisfactory though, to have a behavior of the > > testsuite harness that is so intranparent. > > > > If it is a simple oversight that the behavior of double space was > > never documented, it could simply be fixed, for the sake of everybody. > > > > The take-home message for me is - whenever I write a testcase that > > relies on this behavior - to add a comment in the header that this > > is intended behavior, and set all compiler flags appropriately... > > > > Cheers, > > Harald > > > > > > With two spaces there are 9 tests run at -O, with one space there are 54 > variations executed. Specifying the optimization option in the > dg-options also runs 54 variations. It appears that the "feature" was > done on purpose, but it is frustrating that this is not documented anywhere. It would be nice to document it in sourcebuild.texi and to put a comment before the dg-do with two spaces indicating that two spaces are on purpose. I also don't remember this being implemented ... Richard. > I do remember discussions way back about some tests taking a long time > to run and needing a way to speed up the overall testing. I will look > over the documentation and see if we can get this addressed. > > For my own test cases, I have always specified the options I want and > for front-end work, optimizations don't usually matter except in this > case. I don't have time to go through all 83 tests. > > From my system here: > > Testing of trunk complete..... one space > > real 4m56.294s > user 35m51.082s > sys 12m51.283s > > > Testing of trunk complete..... two spaces > > real 4m44.421s > user 34m3.215s > sys 12m38.778s > > Not a huge difference. > > Jerry > >
[-- Attachment #1: Type: text/plain, Size: 803 bytes --] Hi all, There has been a bit of discussio on which way to go on this. I took a look today and this trivial patch gives the behavior concluded on Fortran Discourse. See the bugzilla for all the relevant information. Regresion tested on x86-64. I will do the appropriate changelog. OK for trunk? Attached is a new test case and the patch here: diff --git a/libgfortran/io/file_pos.c b/libgfortran/io/file_pos.c index 2bc05b293f8..d169961f997 100644 --- a/libgfortran/io/file_pos.c +++ b/libgfortran/io/file_pos.c @@ -352,7 +352,6 @@ st_endfile (st_parameter_filepos *fpp) dtp.common = fpp->common; memset (&dtp.u.p, 0, sizeof (dtp.u.p)); dtp.u.p.current_unit = u; - next_record (&dtp, 1); } unit_truncate (u, stell (u->s), &fpp->common); [-- Attachment #2: endfile_5.f90 --] [-- Type: text/x-fortran, Size: 650 bytes --] ! { dg-do run } ! PR107031 Check that endfile truncates at end of record 5. program test_truncate integer :: num_rec, tmp, i, nr, j open(10, file="in.dat", action='readwrite') do i=1,10 write(10, *) i end do rewind (10) num_rec = 5 i = 1 ioerr = 0 do while (i <= num_rec .and. ioerr == 0) read(10, *, iostat=ioerr) tmp i = i + 1 enddo endfile(10) rewind (10) i = 0 ioerr = 0 do while (i <= num_rec + 1 .and. ioerr == 0) read(10, *, iostat=ioerr) j i = i + 1 end do close(10, status='delete') if (i - 1 /= 5) stop 1 end program test_truncate
On 3/25/24 3:30 PM, Harald Anlauf wrote:
>> On 3/25/24 12:53 PM, Harald Anlauf wrote:
>>>>>>> I noticed by chance that we have quite a lot of improperly specified do-do
>>>>>>> directives in the testsuite.
>>>>>>>
>>>>>>> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l
>>>>>>> 83
>>>>>>>
>>>>>
>>>>> I think this is on purpose.
>>>>> The idea to use a "feature" in dejagnu to only iterate once and not
>>>>> over all possible options. So execution time can be lowered a bit.
>>>>>
>>>>> But I don't know if this hack still works, it definitely did work some years ago.
>>>>>
>>>>> Cheers,
>>>>> Manfred
>>>
>>> Is this "feature" documented somewhere? I don't see it on
>>>
>>> https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gccint/Directives.html
>>>
>>> Given that the dg-directives are important and possibly fragile,
>>> and since we had issues in the past, can we check that a test
>>> that was added works the way intended?
>>>
>>>>>>> Note that with two blanks instead of just one a testcase does not get executed.
>>>>>>>
>>>>>>> Does anybody want to earn the honors to change the directives and
>>>>>>> check for "fallout" in the testsuite?
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Harald
>>>>>>>
>>>>>>
>>>>>> One failure after fixing all the spaces ( sed is our friend ).
>>>>>>
>>>>>> FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times optimized "_gfortran_matmul" 0
>>>
>>> This does actually point to an issue with the testcase:
>>> it only works properly with optimization enabled.
>>>
>>> Manual inspection of this test and the expected dump suggests
>>> that e.g. -O1 could have been added to the dg-options directive.
>>>
>>> Shouldn't we fix at least the dg-options of that testcase?
>>>
>>> Cheers,
>>> Harald
>>>
>>
>> I restored the one test that appeared to fail so that it had the two
>> spaces 'trick'. When run in the test suite, it is compiled with -O
>> which does invoke the optimization. I manually checked the tree dump
>> with this option and it indeed has all the _gfortran_matmul calls removed.
>>
>> I am inclined to leave these all untouched with the two spaces in place.
>>
>> From the test log:
>>
>> PASS: gfortran.dg/inline_matmul_1.f90 -O execution test
>> PASS: gfortran.dg/inline_matmul_1.f90 -O scan-tree-dump-times
>> optimized "_gfortran_matmul" 0
>
> Alright, then leave it that way.
>
> I find it somewhat unsatisfactory though, to have a behavior of the
> testsuite harness that is so intranparent.
>
> If it is a simple oversight that the behavior of double space was
> never documented, it could simply be fixed, for the sake of everybody.
>
> The take-home message for me is - whenever I write a testcase that
> relies on this behavior - to add a comment in the header that this
> is intended behavior, and set all compiler flags appropriately...
>
> Cheers,
> Harald
>
>
With two spaces there are 9 tests run at -O, with one space there are 54
variations executed. Specifying the optimization option in the
dg-options also runs 54 variations. It appears that the "feature" was
done on purpose, but it is frustrating that this is not documented anywhere.
I do remember discussions way back about some tests taking a long time
to run and needing a way to speed up the overall testing. I will look
over the documentation and see if we can get this addressed.
For my own test cases, I have always specified the options I want and
for front-end work, optimizations don't usually matter except in this
case. I don't have time to go through all 83 tests.
From my system here:
Testing of trunk complete..... one space
real 4m56.294s
user 35m51.082s
sys 12m51.283s
Testing of trunk complete..... two spaces
real 4m44.421s
user 34m3.215s
sys 12m38.778s
Not a huge difference.
Jerry
> On 3/25/24 12:53 PM, Harald Anlauf wrote: > >>>>> I noticed by chance that we have quite a lot of improperly specified do-do > >>>>> directives in the testsuite. > >>>>> > >>>>> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l > >>>>> 83 > >>>>> > >>> > >>> I think this is on purpose. > >>> The idea to use a "feature" in dejagnu to only iterate once and not > >>> over all possible options. So execution time can be lowered a bit. > >>> > >>> But I don't know if this hack still works, it definitely did work some years ago. > >>> > >>> Cheers, > >>> Manfred > > > > Is this "feature" documented somewhere? I don't see it on > > > > https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gccint/Directives.html > > > > Given that the dg-directives are important and possibly fragile, > > and since we had issues in the past, can we check that a test > > that was added works the way intended? > > > >>>>> Note that with two blanks instead of just one a testcase does not get executed. > >>>>> > >>>>> Does anybody want to earn the honors to change the directives and > >>>>> check for "fallout" in the testsuite? > >>>>> > >>>>> Cheers, > >>>>> Harald > >>>>> > >>>> > >>>> One failure after fixing all the spaces ( sed is our friend ). > >>>> > >>>> FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times optimized "_gfortran_matmul" 0 > > > > This does actually point to an issue with the testcase: > > it only works properly with optimization enabled. > > > > Manual inspection of this test and the expected dump suggests > > that e.g. -O1 could have been added to the dg-options directive. > > > > Shouldn't we fix at least the dg-options of that testcase? > > > > Cheers, > > Harald > > > > I restored the one test that appeared to fail so that it had the two > spaces 'trick'. When run in the test suite, it is compiled with -O > which does invoke the optimization. I manually checked the tree dump > with this option and it indeed has all the _gfortran_matmul calls removed. > > I am inclined to leave these all untouched with the two spaces in place. > > From the test log: > > PASS: gfortran.dg/inline_matmul_1.f90 -O execution test > PASS: gfortran.dg/inline_matmul_1.f90 -O scan-tree-dump-times > optimized "_gfortran_matmul" 0 Alright, then leave it that way. I find it somewhat unsatisfactory though, to have a behavior of the testsuite harness that is so intranparent. If it is a simple oversight that the behavior of double space was never documented, it could simply be fixed, for the sake of everybody. The take-home message for me is - whenever I write a testcase that relies on this behavior - to add a comment in the header that this is intended behavior, and set all compiler flags appropriately... Cheers, Harald > Regards, > > Jerry > >
On 3/25/24 12:53 PM, Harald Anlauf wrote:
>>>>> I noticed by chance that we have quite a lot of improperly specified do-do
>>>>> directives in the testsuite.
>>>>>
>>>>> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l
>>>>> 83
>>>>>
>>>
>>> I think this is on purpose.
>>> The idea to use a "feature" in dejagnu to only iterate once and not
>>> over all possible options. So execution time can be lowered a bit.
>>>
>>> But I don't know if this hack still works, it definitely did work some years ago.
>>>
>>> Cheers,
>>> Manfred
>
> Is this "feature" documented somewhere? I don't see it on
>
> https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gccint/Directives.html
>
> Given that the dg-directives are important and possibly fragile,
> and since we had issues in the past, can we check that a test
> that was added works the way intended?
>
>>>>> Note that with two blanks instead of just one a testcase does not get executed.
>>>>>
>>>>> Does anybody want to earn the honors to change the directives and
>>>>> check for "fallout" in the testsuite?
>>>>>
>>>>> Cheers,
>>>>> Harald
>>>>>
>>>>
>>>> One failure after fixing all the spaces ( sed is our friend ).
>>>>
>>>> FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times optimized "_gfortran_matmul" 0
>
> This does actually point to an issue with the testcase:
> it only works properly with optimization enabled.
>
> Manual inspection of this test and the expected dump suggests
> that e.g. -O1 could have been added to the dg-options directive.
>
> Shouldn't we fix at least the dg-options of that testcase?
>
> Cheers,
> Harald
>
I restored the one test that appeared to fail so that it had the two
spaces 'trick'. When run in the test suite, it is compiled with -O
which does invoke the optimization. I manually checked the tree dump
with this option and it indeed has all the _gfortran_matmul calls removed.
I am inclined to leave these all untouched with the two spaces in place.
From the test log:
PASS: gfortran.dg/inline_matmul_1.f90 -O execution test
PASS: gfortran.dg/inline_matmul_1.f90 -O scan-tree-dump-times
optimized "_gfortran_matmul" 0
Regards,
Jerry
> >>> I noticed by chance that we have quite a lot of improperly specified do-do > >>> directives in the testsuite. > >>> > >>> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l > >>> 83 > >>> > > > > I think this is on purpose. > > The idea to use a "feature" in dejagnu to only iterate once and not > > over all possible options. So execution time can be lowered a bit. > > > > But I don't know if this hack still works, it definitely did work some years ago. > > > > Cheers, > > Manfred Is this "feature" documented somewhere? I don't see it on https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gccint/Directives.html Given that the dg-directives are important and possibly fragile, and since we had issues in the past, can we check that a test that was added works the way intended? > >>> Note that with two blanks instead of just one a testcase does not get executed. > >>> > >>> Does anybody want to earn the honors to change the directives and > >>> check for "fallout" in the testsuite? > >>> > >>> Cheers, > >>> Harald > >>> > >> > >> One failure after fixing all the spaces ( sed is our friend ). > >> > >> FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times optimized "_gfortran_matmul" 0 This does actually point to an issue with the testcase: it only works properly with optimization enabled. Manual inspection of this test and the expected dump suggests that e.g. -O1 could have been added to the dg-options directive. Shouldn't we fix at least the dg-options of that testcase? Cheers, Harald > >> Jerry - > > > > Yes it still works, I noticed the tests will do two passes each rather > than 12 per file. > > I will hold off on doing anything with these. > > Regards, > > Jerry - >
[-- Attachment #1: Type: text/plain, Size: 499 bytes --] Hi Harald, Yes, that's a good idea. I'll take a look tomorrow morning to see what I think needs doing and then let's put heads together. Regards Paul On Sun, 24 Mar 2024 at 20:23, Harald Anlauf <anlauf@gmx.de> wrote: > Dear all, > > the gfortran wiki (https://gcc.gnu.org/wiki/GFortran) seems to have been > neglected for quite some time. > > Given that the release of gcc 14.1 is to be expected in the near future, > do we want to bring the wiki pages it up-to-date? > > Cheers, > Harald > >
On 3/25/24 1:27 AM, Manfred Schwarb wrote:
> Am 25.03.24 um 05:53 schrieb Jerry D:
>> On 3/24/24 1:19 PM, Harald Anlauf wrote:
>>> Dear all,
>>>
>>> I noticed by chance that we have quite a lot of improperly specified do-do
>>> directives in the testsuite.
>>>
>>> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l
>>> 83
>>>
>
> I think this is on purpose.
> The idea to use a "feature" in dejagnu to only iterate once and not
> over all possible options. So execution time can be lowered a bit.
>
> But I don't know if this hack still works, it definitely did work some years ago.
>
> Cheers,
> Manfred
>
>
>>> Note that with two blanks instead of just one a testcase does not get executed.
>>>
>>> Does anybody want to earn the honors to change the directives and
>>> check for "fallout" in the testsuite?
>>>
>>> Cheers,
>>> Harald
>>>
>>
>> One failure after fixing all the spaces ( sed is our friend ).
>>
>> FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times optimized "_gfortran_matmul" 0
>>
>> Jerry -
>
Yes it still works, I noticed the tests will do two passes each rather
than 12 per file.
I will hold off on doing anything with these.
Regards,
Jerry -
Am 25.03.24 um 05:53 schrieb Jerry D: > On 3/24/24 1:19 PM, Harald Anlauf wrote: >> Dear all, >> >> I noticed by chance that we have quite a lot of improperly specified do-do >> directives in the testsuite. >> >> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l >> 83 >> I think this is on purpose. The idea to use a "feature" in dejagnu to only iterate once and not over all possible options. So execution time can be lowered a bit. But I don't know if this hack still works, it definitely did work some years ago. Cheers, Manfred >> Note that with two blanks instead of just one a testcase does not get executed. >> >> Does anybody want to earn the honors to change the directives and >> check for "fallout" in the testsuite? >> >> Cheers, >> Harald >> > > One failure after fixing all the spaces ( sed is our friend ). > > FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times optimized "_gfortran_matmul" 0 > > Jerry -
On 3/24/24 1:19 PM, Harald Anlauf wrote:
> Dear all,
>
> I noticed by chance that we have quite a lot of improperly specified do-do
> directives in the testsuite.
>
> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l
> 83
>
> Note that with two blanks instead of just one a testcase does not get executed.
>
> Does anybody want to earn the honors to change the directives and
> check for "fallout" in the testsuite?
>
> Cheers,
> Harald
>
One failure after fixing all the spaces ( sed is our friend ).
FAIL: gfortran.dg/inline_matmul_1.f90 -O0 scan-tree-dump-times
optimized "_gfortran_matmul" 0
Jerry -
On 3/24/24 1:19 PM, Harald Anlauf wrote:
> Dear all,
>
> I noticed by chance that we have quite a lot of improperly specified do-do
> directives in the testsuite.
>
> % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l
> 83
>
> Note that with two blanks instead of just one a testcase does not get executed.
>
> Does anybody want to earn the honors to change the directives and
> check for "fallout" in the testsuite?
>
> Cheers,
> Harald
>
I will do this for the betterment of the order of all good people. I
need the therapy after beating my head against a wall last week eating
separators. :)
Cheers,
Jerry -
Dear all, the gfortran wiki (https://gcc.gnu.org/wiki/GFortran) seems to have been neglected for quite some time. Given that the release of gcc 14.1 is to be expected in the near future, do we want to bring the wiki pages it up-to-date? Cheers, Harald
Dear all, I noticed by chance that we have quite a lot of improperly specified do-do directives in the testsuite. % grep "dg-do run" gcc/testsuite/gfortran.dg/ -rl|wc -l 83 Note that with two blanks instead of just one a testcase does not get executed. Does anybody want to earn the honors to change the directives and check for "fallout" in the testsuite? Cheers, Harald
[-- Attachment #1: Type: text/plain, Size: 587 bytes --] Hi Harald, This is completely fine - if you haven't committed, please do so. Thanks Paul On Fri, 22 Mar 2024 at 17:32, Harald Anlauf <anlauf@gmx.de> wrote: > Dear all, > > here's a simple and obvious patch for a rejects-valid case when > we pass a NULL() actual to an optional dummy for variants where > there is no MOLD argument and it is also not required. > > The testcase is an extended version of PR55978 comment#16 > and cross-checked with Intel and NAG. > > Regtested on x86_64-pc-linux-gnu. > > I intend to commit soon unless there are objections. > > Thanks, > Harald > >
[-- Attachment #1: Type: text/plain, Size: 397 bytes --] Dear all, here's a simple and obvious patch for a rejects-valid case when we pass a NULL() actual to an optional dummy for variants where there is no MOLD argument and it is also not required. The testcase is an extended version of PR55978 comment#16 and cross-checked with Intel and NAG. Regtested on x86_64-pc-linux-gnu. I intend to commit soon unless there are objections. Thanks, Harald [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: pr55978-null-actual.diff --] [-- Type: text/x-patch, Size: 3437 bytes --] From e92244c5539a537cff338b781d15acd58d4c86f1 Mon Sep 17 00:00:00 2001 From: Harald Anlauf <anlauf@gmx.de> Date: Fri, 22 Mar 2024 18:17:15 +0100 Subject: [PATCH] Fortran: no size check passing NULL() without MOLD argument [PR55978] gcc/fortran/ChangeLog: PR fortran/55978 * interface.cc (gfc_compare_actual_formal): Skip size check for NULL() actual without MOLD argument. gcc/testsuite/ChangeLog: PR fortran/55978 * gfortran.dg/null_actual_5.f90: New test. --- gcc/fortran/interface.cc | 4 ++ gcc/testsuite/gfortran.dg/null_actual_5.f90 | 76 +++++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 gcc/testsuite/gfortran.dg/null_actual_5.f90 diff --git a/gcc/fortran/interface.cc b/gcc/fortran/interface.cc index 64b90550be2..7b86a338bc1 100644 --- a/gcc/fortran/interface.cc +++ b/gcc/fortran/interface.cc @@ -3439,6 +3439,10 @@ gfc_compare_actual_formal (gfc_actual_arglist **ap, gfc_formal_arglist *formal, if (f->sym->ts.type == BT_CLASS) goto skip_size_check; + /* Skip size check for NULL() actual without MOLD argument. */ + if (a->expr->expr_type == EXPR_NULL && a->expr->ts.type == BT_UNKNOWN) + goto skip_size_check; + actual_size = get_expr_storage_size (a->expr); formal_size = get_sym_storage_size (f->sym); if (actual_size != 0 && actual_size < formal_size diff --git a/gcc/testsuite/gfortran.dg/null_actual_5.f90 b/gcc/testsuite/gfortran.dg/null_actual_5.f90 new file mode 100644 index 00000000000..1198715b7c8 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/null_actual_5.f90 @@ -0,0 +1,76 @@ +! { dg-do compile } +! PR fortran/55978 +! +! Passing of NULL() with and without MOLD as actual argument +! +! Testcase derived from pr55978 comment#16 + +program pr55978_c16 + implicit none + + integer, pointer :: p(:) + integer, allocatable :: a(:) + character(10), pointer :: c + character(10), pointer :: cp(:) + + type t + integer, pointer :: p(:) + integer, allocatable :: a(:) + end type + + type(t) :: d + + ! (1) pointer + p => null() + call sub (p) + + ! (2) allocatable + call sub (a) + call sub (d%a) + + ! (3) pointer component + d%p => null () + call sub (d%p) + + ! (4) NULL + call sub (null (a)) ! OK + call sub (null (p)) ! OK + call sub (null (d%a)) ! OK + call sub (null (d%p)) ! OK + call sub (null ()) ! was erroneously rejected with: + ! Actual argument contains too few elements for dummy argument 'x' (1/4) + + call bla (null(c)) + call bla (null()) ! was erroneously rejected with: + ! Actual argument contains too few elements for dummy argument 'x' (1/10) + + call foo (null(cp)) + call foo (null()) + + call bar (null(cp)) + call bar (null()) ! was erroneously rejected with: + ! Actual argument contains too few elements for dummy argument 'x' (1/70) + +contains + + subroutine sub(x) + integer, intent(in), optional :: x(4) + if (present (x)) stop 1 + end + + subroutine bla(x) + character(len=10), intent(in), optional :: x + if (present (x)) stop 2 + end + + subroutine foo(x) + character(len=10), intent(in), optional :: x(:) + if (present (x)) stop 3 + end + + subroutine bar(x) + character(len=10), intent(in), optional :: x(7) + if (present (x)) stop 4 + end + +end -- 2.35.3
Hi Mikael,
this looks all good to me. I wouldn't mind the minor side-effects of
better error recovery, as you are (successfully) trying hard to keep
the namespaces sane. So OK for mainline.
Thanks for the patch!
Harald
On 3/21/24 17:27, Mikael Morin wrote:
> Hello,
>
> here is a fix for an ICE caused by dangling pointers to ISO_C_BINDING's
> C_PTR symbol in the global intrinsic symbol for C_LOC.
> I tried to fix it by making the intrinsic symbol use its own copy of
> C_PTR, but it regressed heavily.
>
> Instead, I propose this which is based on a patch I attached to the PR
> one year ago. It's sufficient to remove the access to freed memory.
>
> However, an underlying problem remains that successive use-associations
> of ISO_C_BINDING's symbols in different scopes cause the return type
> of the C_LOC global intrinsic symbol to be set to the C_PTR from each
> scope successively, with the last one "winning". Not very pretty.
>
> Anyway, there are two changed messages in the testsuite as a side-effect
> of the proposed change. I regard them as acceptable, albeit slightly worse.
> No regression otherwise on x86_64-pc-linux-gnu.
> Ok for 14 master?
>
> Mikael
>
> -- >8 --
>
> This fixes an access to freed memory on the testcase from the PR.
> The problem comes from an invalid subroutine statement in an interface,
> which is ignored and causes the following statements forming the procedure
> body to be rejected. One of them use-associates the intrinsic ISO_C_BINDING
> module, which imports new symbols in a namespace that is freed at the time
> the statement is rejected. However, this creates dangling pointers as
> ISO_C_BINDING is special and its import creates a reference to the imported
> C_PTR symbol in the return type of the global intrinsic symbol for C_LOC
> (see the function create_intrinsic_function).
>
> This change saves and restores the list of use statements, so that rejected
> use statements are removed before they have a chance to be applied to the
> current namespace and create dangling pointers.
>
> PR fortran/107426
>
> gcc/fortran/ChangeLog:
>
> * gfortran.h (gfc_save_module_list, gfc_restore_old_module_list):
> New declarations.
> * module.cc (old_module_list_tail): New global variable.
> (gfc_save_module_list, gfc_restore_old_module_list): New functions.
> (gfc_use_modules): Set module_list and old_module_list_tail.
> * parse.cc (next_statement): Save module_list before doing any work.
> (reject_statement): Restore module_list to its saved value.
>
> gcc/testsuite/ChangeLog:
>
> * gfortran.dg/pr89943_3.f90: Update error pattern.
> * gfortran.dg/pr89943_4.f90: Likewise.
> * gfortran.dg/use_31.f90: New test.
> ---
> gcc/fortran/gfortran.h | 2 ++
> gcc/fortran/module.cc | 31 +++++++++++++++++++++++++
> gcc/fortran/parse.cc | 4 ++++
> gcc/testsuite/gfortran.dg/pr89943_3.f90 | 2 +-
> gcc/testsuite/gfortran.dg/pr89943_4.f90 | 2 +-
> gcc/testsuite/gfortran.dg/use_31.f90 | 25 ++++++++++++++++++++
> 6 files changed, 64 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gfortran.dg/use_31.f90
>
> diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
> index c7039730fad..fec7b53ff1a 100644
> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
> @@ -3926,6 +3926,8 @@ void gfc_module_done_2 (void);
> void gfc_dump_module (const char *, int);
> bool gfc_check_symbol_access (gfc_symbol *);
> void gfc_free_use_stmts (gfc_use_list *);
> +void gfc_save_module_list ();
> +void gfc_restore_old_module_list ();
> const char *gfc_dt_lower_string (const char *);
> const char *gfc_dt_upper_string (const char *);
>
> diff --git a/gcc/fortran/module.cc b/gcc/fortran/module.cc
> index d1de53cbdb4..c565b84d61b 100644
> --- a/gcc/fortran/module.cc
> +++ b/gcc/fortran/module.cc
> @@ -195,7 +195,12 @@ static const char *module_name;
> /* The name of the .smod file that the submodule will write to. */
> static const char *submodule_name;
>
> +/* The list of use statements to apply to the current namespace
> + before parsing the non-use statements. */
> static gfc_use_list *module_list;
> +/* The end of the MODULE_LIST list above at the time the recognition
> + of the current statement started. */
> +static gfc_use_list **old_module_list_tail;
>
> /* If we're reading an intrinsic module, this is its ID. */
> static intmod_id current_intmod;
> @@ -7561,6 +7566,8 @@ gfc_use_modules (void)
> gfc_use_module (module_list);
> free (module_list);
> }
> + module_list = NULL;
> + old_module_list_tail = &module_list;
> gfc_rename_list = NULL;
> }
>
> @@ -7584,6 +7591,30 @@ gfc_free_use_stmts (gfc_use_list *use_stmts)
> }
>
>
> +/* Remember the end of the MODULE_LIST list, so that the list can be restored
> + to its previous state if the current statement is erroneous. */
> +
> +void
> +gfc_save_module_list ()
> +{
> + gfc_use_list **tail = &module_list;
> + while (*tail != NULL)
> + tail = &(*tail)->next;
> + old_module_list_tail = tail;
> +}
> +
> +
> +/* Restore the MODULE_LIST list to its previous value and free the use
> + statements that are no longer part of the list. */
> +
> +void
> +gfc_restore_old_module_list ()
> +{
> + gfc_free_use_stmts (*old_module_list_tail);
> + *old_module_list_tail = NULL;
> +}
> +
> +
> void
> gfc_module_init_2 (void)
> {
> diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc
> index a2bf328f681..79c810c86ba 100644
> --- a/gcc/fortran/parse.cc
> +++ b/gcc/fortran/parse.cc
> @@ -1800,6 +1800,7 @@ next_statement (void)
> locus old_locus;
>
> gfc_enforce_clean_symbol_state ();
> + gfc_save_module_list ();
>
> gfc_new_block = NULL;
>
> @@ -3104,6 +3105,9 @@ reject_statement (void)
>
> gfc_reject_data (gfc_current_ns);
>
> + /* Don't queue use-association of a module if we reject the use statement. */
> + gfc_restore_old_module_list ();
> +
> gfc_new_block = NULL;
> gfc_undo_symbols ();
> gfc_clear_warning ();
> diff --git a/gcc/testsuite/gfortran.dg/pr89943_3.f90 b/gcc/testsuite/gfortran.dg/pr89943_3.f90
> index 38b723e2458..84a9fb74741 100644
> --- a/gcc/testsuite/gfortran.dg/pr89943_3.f90
> +++ b/gcc/testsuite/gfortran.dg/pr89943_3.f90
> @@ -22,7 +22,7 @@ submodule(Foo_mod) Foo_smod
> module subroutine runFoo4C(ndim) bind(C, name="runFu") ! { dg-error "Mismatch in BIND" }
> use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement" }
> implicit none ! { dg-error "Unexpected IMPLICIT NONE statement" }
> - integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Unexpected data declaration" }
> + integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Symbol 'c_int32_t' at .1. has no IMPLICIT type" }
> end subroutine runFoo4C ! { dg-error " Expecting END SUBMODULE" }
>
> end submodule Foo_smod
> diff --git a/gcc/testsuite/gfortran.dg/pr89943_4.f90 b/gcc/testsuite/gfortran.dg/pr89943_4.f90
> index 8eba2eda171..cb955d01c88 100644
> --- a/gcc/testsuite/gfortran.dg/pr89943_4.f90
> +++ b/gcc/testsuite/gfortran.dg/pr89943_4.f90
> @@ -23,7 +23,7 @@ submodule(Foo_mod) Foo_smod
> module function runFoo4C(ndim) bind(C, name="runFu") ! { dg-error "Mismatch in BIND" }
> use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement in" }
> implicit none ! { dg-error "Unexpected IMPLICIT NONE statement" }
> - integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Unexpected data declaration" }
> + integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Symbol 'c_int32_t' at .1. has no IMPLICIT type" }
> end function runFoo4C ! { dg-error "Expecting END SUBMODULE" }
>
> end submodule Foo_smod
> diff --git a/gcc/testsuite/gfortran.dg/use_31.f90 b/gcc/testsuite/gfortran.dg/use_31.f90
> new file mode 100644
> index 00000000000..818f2e30b09
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/use_31.f90
> @@ -0,0 +1,25 @@
> +! { dg-do compile }
> +!
> +! PR fortran/107426
> +! This example used to generate an ICE, caused by the use stmt from the nested
> +! procedure declaration wrongly applying to the host procedure and overwriting
> +! the symbols there.
> +!
> +! Contributed by Gerhard Steinmetz <gscfq@t-online.de>
> +
> +module m
> +contains
> + subroutine p() bind(c)
> + use, intrinsic :: iso_c_binding
> + integer, target :: a = 1
> + type(c_ptr) :: z
> + interface
> + subroutine s(x) bind(cc) ! { dg-error "Missing closing paren" }
> + use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement in INTERFACE block" }
> + integer(c_int), value :: x ! { dg-error "Parameter 'c_int' at .1. has not been declared" }
> + end ! { dg-error "END INTERFACE statement expected" }
> + end interface
> + z = c_loc(a)
> + call s(z)
> + end
> +end
Hello, here is a fix for an ICE caused by dangling pointers to ISO_C_BINDING's C_PTR symbol in the global intrinsic symbol for C_LOC. I tried to fix it by making the intrinsic symbol use its own copy of C_PTR, but it regressed heavily. Instead, I propose this which is based on a patch I attached to the PR one year ago. It's sufficient to remove the access to freed memory. However, an underlying problem remains that successive use-associations of ISO_C_BINDING's symbols in different scopes cause the return type of the C_LOC global intrinsic symbol to be set to the C_PTR from each scope successively, with the last one "winning". Not very pretty. Anyway, there are two changed messages in the testsuite as a side-effect of the proposed change. I regard them as acceptable, albeit slightly worse. No regression otherwise on x86_64-pc-linux-gnu. Ok for 14 master? Mikael -- >8 -- This fixes an access to freed memory on the testcase from the PR. The problem comes from an invalid subroutine statement in an interface, which is ignored and causes the following statements forming the procedure body to be rejected. One of them use-associates the intrinsic ISO_C_BINDING module, which imports new symbols in a namespace that is freed at the time the statement is rejected. However, this creates dangling pointers as ISO_C_BINDING is special and its import creates a reference to the imported C_PTR symbol in the return type of the global intrinsic symbol for C_LOC (see the function create_intrinsic_function). This change saves and restores the list of use statements, so that rejected use statements are removed before they have a chance to be applied to the current namespace and create dangling pointers. PR fortran/107426 gcc/fortran/ChangeLog: * gfortran.h (gfc_save_module_list, gfc_restore_old_module_list): New declarations. * module.cc (old_module_list_tail): New global variable. (gfc_save_module_list, gfc_restore_old_module_list): New functions. (gfc_use_modules): Set module_list and old_module_list_tail. * parse.cc (next_statement): Save module_list before doing any work. (reject_statement): Restore module_list to its saved value. gcc/testsuite/ChangeLog: * gfortran.dg/pr89943_3.f90: Update error pattern. * gfortran.dg/pr89943_4.f90: Likewise. * gfortran.dg/use_31.f90: New test. --- gcc/fortran/gfortran.h | 2 ++ gcc/fortran/module.cc | 31 +++++++++++++++++++++++++ gcc/fortran/parse.cc | 4 ++++ gcc/testsuite/gfortran.dg/pr89943_3.f90 | 2 +- gcc/testsuite/gfortran.dg/pr89943_4.f90 | 2 +- gcc/testsuite/gfortran.dg/use_31.f90 | 25 ++++++++++++++++++++ 6 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gfortran.dg/use_31.f90 diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h index c7039730fad..fec7b53ff1a 100644 --- a/gcc/fortran/gfortran.h +++ b/gcc/fortran/gfortran.h @@ -3926,6 +3926,8 @@ void gfc_module_done_2 (void); void gfc_dump_module (const char *, int); bool gfc_check_symbol_access (gfc_symbol *); void gfc_free_use_stmts (gfc_use_list *); +void gfc_save_module_list (); +void gfc_restore_old_module_list (); const char *gfc_dt_lower_string (const char *); const char *gfc_dt_upper_string (const char *); diff --git a/gcc/fortran/module.cc b/gcc/fortran/module.cc index d1de53cbdb4..c565b84d61b 100644 --- a/gcc/fortran/module.cc +++ b/gcc/fortran/module.cc @@ -195,7 +195,12 @@ static const char *module_name; /* The name of the .smod file that the submodule will write to. */ static const char *submodule_name; +/* The list of use statements to apply to the current namespace + before parsing the non-use statements. */ static gfc_use_list *module_list; +/* The end of the MODULE_LIST list above at the time the recognition + of the current statement started. */ +static gfc_use_list **old_module_list_tail; /* If we're reading an intrinsic module, this is its ID. */ static intmod_id current_intmod; @@ -7561,6 +7566,8 @@ gfc_use_modules (void) gfc_use_module (module_list); free (module_list); } + module_list = NULL; + old_module_list_tail = &module_list; gfc_rename_list = NULL; } @@ -7584,6 +7591,30 @@ gfc_free_use_stmts (gfc_use_list *use_stmts) } +/* Remember the end of the MODULE_LIST list, so that the list can be restored + to its previous state if the current statement is erroneous. */ + +void +gfc_save_module_list () +{ + gfc_use_list **tail = &module_list; + while (*tail != NULL) + tail = &(*tail)->next; + old_module_list_tail = tail; +} + + +/* Restore the MODULE_LIST list to its previous value and free the use + statements that are no longer part of the list. */ + +void +gfc_restore_old_module_list () +{ + gfc_free_use_stmts (*old_module_list_tail); + *old_module_list_tail = NULL; +} + + void gfc_module_init_2 (void) { diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc index a2bf328f681..79c810c86ba 100644 --- a/gcc/fortran/parse.cc +++ b/gcc/fortran/parse.cc @@ -1800,6 +1800,7 @@ next_statement (void) locus old_locus; gfc_enforce_clean_symbol_state (); + gfc_save_module_list (); gfc_new_block = NULL; @@ -3104,6 +3105,9 @@ reject_statement (void) gfc_reject_data (gfc_current_ns); + /* Don't queue use-association of a module if we reject the use statement. */ + gfc_restore_old_module_list (); + gfc_new_block = NULL; gfc_undo_symbols (); gfc_clear_warning (); diff --git a/gcc/testsuite/gfortran.dg/pr89943_3.f90 b/gcc/testsuite/gfortran.dg/pr89943_3.f90 index 38b723e2458..84a9fb74741 100644 --- a/gcc/testsuite/gfortran.dg/pr89943_3.f90 +++ b/gcc/testsuite/gfortran.dg/pr89943_3.f90 @@ -22,7 +22,7 @@ submodule(Foo_mod) Foo_smod module subroutine runFoo4C(ndim) bind(C, name="runFu") ! { dg-error "Mismatch in BIND" } use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement" } implicit none ! { dg-error "Unexpected IMPLICIT NONE statement" } - integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Unexpected data declaration" } + integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Symbol 'c_int32_t' at .1. has no IMPLICIT type" } end subroutine runFoo4C ! { dg-error " Expecting END SUBMODULE" } end submodule Foo_smod diff --git a/gcc/testsuite/gfortran.dg/pr89943_4.f90 b/gcc/testsuite/gfortran.dg/pr89943_4.f90 index 8eba2eda171..cb955d01c88 100644 --- a/gcc/testsuite/gfortran.dg/pr89943_4.f90 +++ b/gcc/testsuite/gfortran.dg/pr89943_4.f90 @@ -23,7 +23,7 @@ submodule(Foo_mod) Foo_smod module function runFoo4C(ndim) bind(C, name="runFu") ! { dg-error "Mismatch in BIND" } use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement in" } implicit none ! { dg-error "Unexpected IMPLICIT NONE statement" } - integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Unexpected data declaration" } + integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Symbol 'c_int32_t' at .1. has no IMPLICIT type" } end function runFoo4C ! { dg-error "Expecting END SUBMODULE" } end submodule Foo_smod diff --git a/gcc/testsuite/gfortran.dg/use_31.f90 b/gcc/testsuite/gfortran.dg/use_31.f90 new file mode 100644 index 00000000000..818f2e30b09 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/use_31.f90 @@ -0,0 +1,25 @@ +! { dg-do compile } +! +! PR fortran/107426 +! This example used to generate an ICE, caused by the use stmt from the nested +! procedure declaration wrongly applying to the host procedure and overwriting +! the symbols there. +! +! Contributed by Gerhard Steinmetz <gscfq@t-online.de> + +module m +contains + subroutine p() bind(c) + use, intrinsic :: iso_c_binding + integer, target :: a = 1 + type(c_ptr) :: z + interface + subroutine s(x) bind(cc) ! { dg-error "Missing closing paren" } + use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement in INTERFACE block" } + integer(c_int), value :: x ! { dg-error "Parameter 'c_int' at .1. has not been declared" } + end ! { dg-error "END INTERFACE statement expected" } + end interface + z = c_loc(a) + call s(z) + end +end -- 2.43.0
Le 20/03/2024 à 21:24, Harald Anlauf a écrit : > Hi Mikael, all, > > here's now the third version of the patch that implements the following > scheme: > > On 3/15/24 20:29, Mikael Morin wrote: >> Le 15/03/2024 à 18:26, Harald Anlauf a écrit : >>> OK, that sounds interesting. To clarify the options: >>> >>> - for ordinary array x it would stay 'x' >>> >>> - when z is a DT scalar, and z%x is the array in question, use 'z%x' >>> (here z...%x would look strange to me) >>> >> Yes, the ellipsis would look strange to me as well. >> >>> - when z is a DT array, and x some component further down, 'z...%x' >>> >> This case also applies when z is a DT scalar and x is more than one >> level deep. >> >>> I would rather not make the error message text vary too much to avoid >>> to run into issues with translation. Would it be fine with you to have >>> >>> ... dimension 1 of array 'z...%x' above array bound ... >>> >>> only? >>> >> OK, let's drop "component". >> >>> Anything else? >>> >> No, I think you covered everything. > > I've created a new helper function that centralizes the generation of > the abbreviated name of the array (component) and use it to simplify > related code in multiple places. If we change our mind how a bounds > violation error message should look like, it will be easier to adjust > in the future. > > Is this OK for 14-mainline? > Yes, thanks. > Thanks, > Harald > >