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 <h...@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
