| Message ID | 20240925020614.2E41E2042C@pchp3.se.axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gfortran testsuite: Remove unit-files in files having open-statements, PR116701 | expand |
Hi Hans-Peter, preface: I am not a testsuite nor an m4 expert. So I may be wrong in arguing that your changes look reasonable. I like the "automatic" clean-up process very much. So by me, ok for mainline. But you may want to wait for one other ok from some one who has more experience in the gfortran testsuite (yes, still wondering who that might be :-( ). Thanks for the patch, Andre On Wed, 25 Sep 2024 04:06:14 +0200 Hans-Peter Nilsson <hp@axis.com> wrote: > Changes since v1: > - Rename gfortran-dg-rmunits to fortran-delete-unit-files. > - Move it to lib/fortran-modules.exp. > - Tweak commit message accordingly and mention cause of placement of > the proc. > - Tweak proc comment to mention why keeping removals unique despite > comment. > > Here's a general approach to handle PR116701. I considered > adding manual deletions as quoted below and mentioned in the > PR, but seeing the handling of "integer 8" in > fortran-torture-execute I decided to follow that example: > better scan the source for open-statements than relying on > manual annotations and people remembering to add them for > new test-cases. > > I hope the inclusion of gfortran-dg.exp in > fortran-torture.exp is not controversial, but there's no > fortran-specific testsuite file common to dg and > classic-torture and also this placement is still in the > "Utility routines" section of gfortran-dg.exp. (BTW, the C > torture-tests changed to the dg framework some time ago - no > more .x-files there and dg-directives actually work - there > are some in gfortran.fortran-torture that are apparently > ignored!) > > There's one further cleanup possible, removing the manual > removal in open_errors_2.f90 (which should have used > "target", not "build") > > Works for cris-elf (no regressions). Version v1 was also > similarly regtested on native x86_64-linux-gnu. Manual > checks have verified the unit-removal. > > Ok to commit? > > -- >8 -- > PR testsuite/116701 shows that left-behind files from > unnamed gfortran open statements (named unit.N, where N = > unit number) can interfere with the result of a subsequent > run. While that's unlikely to happen for a "real" fortran > target or a test with a deleting close-statement, test-cases > should not rely on previous test-cases passing and not > execute along different execution paths depending on earlier > runs, even if the difference is benevolent. > > Most but not all fortran test-cases go through > gfortran-dg-runtest (gfortran.dg) or fortran-torture-execute > (gfortran.fortran-torture). However, the exceptions, with > more complex framework and call-chains, either don't run or > don't have open-statements, so a more complex solution > doesn't seem worthwhile. If test-cases with open-statements > are added later to those parts of the test-suite, calls to > fortran-delete-unit-files at the right spot may be added or > worst case, "manual" cleanup-calls added, like: > ! { dg-final { remote_file target delete "fort.10" } } > Put the new proc in fortran-modules.exp since that's where other > common fortran-testsuite dejagnu-library functions are located. > > PR testsuite/116701 > * lib/fortran-modules.exp (fortran-delete-unit-files): New proc. > * lib/gfortran-dg.exp (gfortran-dg-runtest): Call > fortran-delete-unit-files after executing test. > * lib/fortran-torture.exp (fortran-torture-execute): Ditto. > --- > gcc/testsuite/lib/fortran-modules.exp | 21 +++++++++++++++++++++ > gcc/testsuite/lib/fortran-torture.exp | 2 ++ > gcc/testsuite/lib/gfortran-dg.exp | 1 + > 3 files changed, 24 insertions(+) > > diff --git a/gcc/testsuite/lib/fortran-modules.exp > b/gcc/testsuite/lib/fortran-modules.exp index 158b16bada91..a7196f13ed22 > 100644 --- a/gcc/testsuite/lib/fortran-modules.exp > +++ b/gcc/testsuite/lib/fortran-modules.exp > @@ -172,3 +172,24 @@ proc igrep { args } { > } > return $grep_out > } > + > +# If the code has any "open" statements for numbered units, make sure > +# no corresponding output file remains. Redundant remove operations > +# are ok, but duplicate removals look sloppy, so track for uniqueness. > +proc fortran-delete-unit-files { src } { > + set openpat {open *\( *(?:unit *= *)?([0-9]+)} > + set openmatches [igrep $src $openpat] > + if {![string match "" $openmatches]} { > + # verbose -log "Found \"$openmatches\"" > + set deleted_units {} > + foreach openmatch $openmatches { > + regexp -nocase -- "$openpat" $openmatch match unit > + if {[lsearch $deleted_units $unit] < 0} { > + set rmfile "fort.$unit" > + verbose -log "Deleting $rmfile" > + remote_file target delete "fort.$unit" > + lappend deleted_units $unit > + } > + } > + } > +} > diff --git a/gcc/testsuite/lib/fortran-torture.exp > b/gcc/testsuite/lib/fortran-torture.exp index 66f5bc822232..0727fb4fb0a6 > 100644 --- a/gcc/testsuite/lib/fortran-torture.exp > +++ b/gcc/testsuite/lib/fortran-torture.exp > @@ -332,6 +332,8 @@ proc fortran-torture-execute { src } { > catch { remote_file build delete $executable } > } > $status "$testcase execution, $option" > + > + fortran-delete-unit-files $src > } > cleanup-modules "" > } > diff --git a/gcc/testsuite/lib/gfortran-dg.exp > b/gcc/testsuite/lib/gfortran-dg.exp index fcba95dc3961..2edc09e5c995 100644 > --- a/gcc/testsuite/lib/gfortran-dg.exp > +++ b/gcc/testsuite/lib/gfortran-dg.exp > @@ -160,6 +160,7 @@ proc gfortran-dg-runtest { testcases flags > default-extra-flags } { foreach flags_t $option_list { > verbose "Testing $nshort, $flags $flags_t" 1 > dg-test $test "$flags $flags_t" ${default-extra-flags} > + fortran-delete-unit-files $test > cleanup-modules "" > } > } -- Andre Vehreschild * Email: vehre ad gmx dot de
> Date: Wed, 25 Sep 2024 13:51:07 +0200 > From: Andre Vehreschild <vehre@gmx.de> > Hi Hans-Peter, > > preface: I am not a testsuite nor an m4 expert. Neither am I. Luckily, this has nothing to do with m4, and not really that much to do with tcl or dejagnu either, being just basic code, no language-specific tricks. > So I may be wrong in arguing that your changes look reasonable. I like the > "automatic" clean-up process very much. So by me, ok for mainline. But you may > want to wait for one other ok from some one who has more experience in > the gfortran testsuite (yes, still wondering who that might be :-( ). > > Thanks for the patch, > Andre And thank you for the review! Now, the point of my reply wasn't neither a comical point nor thankfulness (I am, but it counts toward the mailing list traffic), but mostly to FAOD point out that this part: > > I hope the inclusion of gfortran-dg.exp in > > fortran-torture.exp is not controversial, but there's no > > fortran-specific testsuite file common to dg and > > classic-torture and also this placement is still in the > > "Utility routines" section of gfortran-dg.exp. (BTW, the C > > torture-tests changed to the dg framework some time ago - no > > more .x-files there and dg-directives actually work - there > > are some in gfortran.fortran-torture that are apparently > > ignored!) ...slipped by from v1 by copy-pasto accident. Still, it was above the "scissors line" in the patch message, and would not have made it to the commit log. brgds, H-P
On 25 September 2024 13:51:07 CEST, Andre Vehreschild <vehre@gmx.de> wrote: >Hi Hans-Peter, > >preface: I am not a testsuite nor an m4 expert. > >So I may be wrong in arguing that your changes look reasonable. I like the >"automatic" clean-up process very much. So by me, ok for mainline. But you may >want to wait for one other ok from some one who has more experience in >the gfortran testsuite (yes, still wondering who that might be :-( ). LGTM Ok for trunk. thanks, > >Thanks for the patch, > Andre > >On Wed, 25 Sep 2024 04:06:14 +0200 >Hans-Peter Nilsson <hp@axis.com> wrote: > >> Changes since v1: >> - Rename gfortran-dg-rmunits to fortran-delete-unit-files. >> - Move it to lib/fortran-modules.exp. >> - Tweak commit message accordingly and mention cause of placement of >> the proc. >> - Tweak proc comment to mention why keeping removals unique despite >> comment. >> >> Here's a general approach to handle PR116701. I considered >> adding manual deletions as quoted below and mentioned in the >> PR, but seeing the handling of "integer 8" in >> fortran-torture-execute I decided to follow that example: >> better scan the source for open-statements than relying on >> manual annotations and people remembering to add them for >> new test-cases. >> >> I hope the inclusion of gfortran-dg.exp in >> fortran-torture.exp is not controversial, but there's no >> fortran-specific testsuite file common to dg and >> classic-torture and also this placement is still in the >> "Utility routines" section of gfortran-dg.exp. (BTW, the C >> torture-tests changed to the dg framework some time ago - no >> more .x-files there and dg-directives actually work - there >> are some in gfortran.fortran-torture that are apparently >> ignored!) >> >> There's one further cleanup possible, removing the manual >> removal in open_errors_2.f90 (which should have used >> "target", not "build") >> >> Works for cris-elf (no regressions). Version v1 was also >> similarly regtested on native x86_64-linux-gnu. Manual >> checks have verified the unit-removal. >> >> Ok to commit? >> >> -- >8 -- >> PR testsuite/116701 shows that left-behind files from >> unnamed gfortran open statements (named unit.N, where N = >> unit number) can interfere with the result of a subsequent >> run. While that's unlikely to happen for a "real" fortran >> target or a test with a deleting close-statement, test-cases >> should not rely on previous test-cases passing and not >> execute along different execution paths depending on earlier >> runs, even if the difference is benevolent. >> >> Most but not all fortran test-cases go through >> gfortran-dg-runtest (gfortran.dg) or fortran-torture-execute >> (gfortran.fortran-torture). However, the exceptions, with >> more complex framework and call-chains, either don't run or >> don't have open-statements, so a more complex solution >> doesn't seem worthwhile. If test-cases with open-statements >> are added later to those parts of the test-suite, calls to >> fortran-delete-unit-files at the right spot may be added or >> worst case, "manual" cleanup-calls added, like: >> ! { dg-final { remote_file target delete "fort.10" } } >> Put the new proc in fortran-modules.exp since that's where other >> common fortran-testsuite dejagnu-library functions are located. >> >> PR testsuite/116701 >> * lib/fortran-modules.exp (fortran-delete-unit-files): New proc. >> * lib/gfortran-dg.exp (gfortran-dg-runtest): Call >> fortran-delete-unit-files after executing test. >> * lib/fortran-torture.exp (fortran-torture-execute): Ditto. >> --- >> gcc/testsuite/lib/fortran-modules.exp | 21 +++++++++++++++++++++ >> gcc/testsuite/lib/fortran-torture.exp | 2 ++ >> gcc/testsuite/lib/gfortran-dg.exp | 1 + >> 3 files changed, 24 insertions(+) >> >> diff --git a/gcc/testsuite/lib/fortran-modules.exp >> b/gcc/testsuite/lib/fortran-modules.exp index 158b16bada91..a7196f13ed22 >> 100644 --- a/gcc/testsuite/lib/fortran-modules.exp >> +++ b/gcc/testsuite/lib/fortran-modules.exp >> @@ -172,3 +172,24 @@ proc igrep { args } { >> } >> return $grep_out >> } >> + >> +# If the code has any "open" statements for numbered units, make sure >> +# no corresponding output file remains. Redundant remove operations >> +# are ok, but duplicate removals look sloppy, so track for uniqueness. >> +proc fortran-delete-unit-files { src } { >> + set openpat {open *\( *(?:unit *= *)?([0-9]+)} >> + set openmatches [igrep $src $openpat] >> + if {![string match "" $openmatches]} { >> + # verbose -log "Found \"$openmatches\"" >> + set deleted_units {} >> + foreach openmatch $openmatches { >> + regexp -nocase -- "$openpat" $openmatch match unit >> + if {[lsearch $deleted_units $unit] < 0} { >> + set rmfile "fort.$unit" >> + verbose -log "Deleting $rmfile" >> + remote_file target delete "fort.$unit" >> + lappend deleted_units $unit >> + } >> + } >> + } >> +} >> diff --git a/gcc/testsuite/lib/fortran-torture.exp >> b/gcc/testsuite/lib/fortran-torture.exp index 66f5bc822232..0727fb4fb0a6 >> 100644 --- a/gcc/testsuite/lib/fortran-torture.exp >> +++ b/gcc/testsuite/lib/fortran-torture.exp >> @@ -332,6 +332,8 @@ proc fortran-torture-execute { src } { >> catch { remote_file build delete $executable } >> } >> $status "$testcase execution, $option" >> + >> + fortran-delete-unit-files $src >> } >> cleanup-modules "" >> } >> diff --git a/gcc/testsuite/lib/gfortran-dg.exp >> b/gcc/testsuite/lib/gfortran-dg.exp index fcba95dc3961..2edc09e5c995 100644 >> --- a/gcc/testsuite/lib/gfortran-dg.exp >> +++ b/gcc/testsuite/lib/gfortran-dg.exp >> @@ -160,6 +160,7 @@ proc gfortran-dg-runtest { testcases flags >> default-extra-flags } { foreach flags_t $option_list { >> verbose "Testing $nshort, $flags $flags_t" 1 >> dg-test $test "$flags $flags_t" ${default-extra-flags} >> + fortran-delete-unit-files $test >> cleanup-modules "" >> } >> } > > >-- >Andre Vehreschild * Email: vehre ad gmx dot de
diff --git a/gcc/testsuite/lib/fortran-modules.exp b/gcc/testsuite/lib/fortran-modules.exp index 158b16bada91..a7196f13ed22 100644 --- a/gcc/testsuite/lib/fortran-modules.exp +++ b/gcc/testsuite/lib/fortran-modules.exp @@ -172,3 +172,24 @@ proc igrep { args } { } return $grep_out } + +# If the code has any "open" statements for numbered units, make sure +# no corresponding output file remains. Redundant remove operations +# are ok, but duplicate removals look sloppy, so track for uniqueness. +proc fortran-delete-unit-files { src } { + set openpat {open *\( *(?:unit *= *)?([0-9]+)} + set openmatches [igrep $src $openpat] + if {![string match "" $openmatches]} { + # verbose -log "Found \"$openmatches\"" + set deleted_units {} + foreach openmatch $openmatches { + regexp -nocase -- "$openpat" $openmatch match unit + if {[lsearch $deleted_units $unit] < 0} { + set rmfile "fort.$unit" + verbose -log "Deleting $rmfile" + remote_file target delete "fort.$unit" + lappend deleted_units $unit + } + } + } +} diff --git a/gcc/testsuite/lib/fortran-torture.exp b/gcc/testsuite/lib/fortran-torture.exp index 66f5bc822232..0727fb4fb0a6 100644 --- a/gcc/testsuite/lib/fortran-torture.exp +++ b/gcc/testsuite/lib/fortran-torture.exp @@ -332,6 +332,8 @@ proc fortran-torture-execute { src } { catch { remote_file build delete $executable } } $status "$testcase execution, $option" + + fortran-delete-unit-files $src } cleanup-modules "" } diff --git a/gcc/testsuite/lib/gfortran-dg.exp b/gcc/testsuite/lib/gfortran-dg.exp index fcba95dc3961..2edc09e5c995 100644 --- a/gcc/testsuite/lib/gfortran-dg.exp +++ b/gcc/testsuite/lib/gfortran-dg.exp @@ -160,6 +160,7 @@ proc gfortran-dg-runtest { testcases flags default-extra-flags } { foreach flags_t $option_list { verbose "Testing $nshort, $flags $flags_t" 1 dg-test $test "$flags $flags_t" ${default-extra-flags} + fortran-delete-unit-files $test cleanup-modules "" } }
Changes since v1: - Rename gfortran-dg-rmunits to fortran-delete-unit-files. - Move it to lib/fortran-modules.exp. - Tweak commit message accordingly and mention cause of placement of the proc. - Tweak proc comment to mention why keeping removals unique despite comment. Here's a general approach to handle PR116701. I considered adding manual deletions as quoted below and mentioned in the PR, but seeing the handling of "integer 8" in fortran-torture-execute I decided to follow that example: better scan the source for open-statements than relying on manual annotations and people remembering to add them for new test-cases. I hope the inclusion of gfortran-dg.exp in fortran-torture.exp is not controversial, but there's no fortran-specific testsuite file common to dg and classic-torture and also this placement is still in the "Utility routines" section of gfortran-dg.exp. (BTW, the C torture-tests changed to the dg framework some time ago - no more .x-files there and dg-directives actually work - there are some in gfortran.fortran-torture that are apparently ignored!) There's one further cleanup possible, removing the manual removal in open_errors_2.f90 (which should have used "target", not "build") Works for cris-elf (no regressions). Version v1 was also similarly regtested on native x86_64-linux-gnu. Manual checks have verified the unit-removal. Ok to commit? -- >8 -- PR testsuite/116701 shows that left-behind files from unnamed gfortran open statements (named unit.N, where N = unit number) can interfere with the result of a subsequent run. While that's unlikely to happen for a "real" fortran target or a test with a deleting close-statement, test-cases should not rely on previous test-cases passing and not execute along different execution paths depending on earlier runs, even if the difference is benevolent. Most but not all fortran test-cases go through gfortran-dg-runtest (gfortran.dg) or fortran-torture-execute (gfortran.fortran-torture). However, the exceptions, with more complex framework and call-chains, either don't run or don't have open-statements, so a more complex solution doesn't seem worthwhile. If test-cases with open-statements are added later to those parts of the test-suite, calls to fortran-delete-unit-files at the right spot may be added or worst case, "manual" cleanup-calls added, like: ! { dg-final { remote_file target delete "fort.10" } } Put the new proc in fortran-modules.exp since that's where other common fortran-testsuite dejagnu-library functions are located. PR testsuite/116701 * lib/fortran-modules.exp (fortran-delete-unit-files): New proc. * lib/gfortran-dg.exp (gfortran-dg-runtest): Call fortran-delete-unit-files after executing test. * lib/fortran-torture.exp (fortran-torture-execute): Ditto. --- gcc/testsuite/lib/fortran-modules.exp | 21 +++++++++++++++++++++ gcc/testsuite/lib/fortran-torture.exp | 2 ++ gcc/testsuite/lib/gfortran-dg.exp | 1 + 3 files changed, 24 insertions(+)