| Message ID | 20200206200144.14304-1-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] analyzer: gfortran testsuite support | expand |
On 2/6/20 9:01 PM, David Malcolm wrote: > PR analyzer/93405 reports an ICE when attempting to use -fanalyzer on > certain gfortran code. The second patch in this kit fixes that, but > in the meantime I need somewhere to put regression tests for -fanalyzer > with gfortran. > > This patch adds a gfortran.dg/analyzer subdirectory with an analyzer.exp, > setting DEFAULT_FFLAGS on the tests run within it. I have seen no objections against this proposal, so please go ahead. Kind regards,
On Sun, Feb 09, 2020 at 09:15:46PM +0100, Toon Moene wrote: > On 2/6/20 9:01 PM, David Malcolm wrote: > > > PR analyzer/93405 reports an ICE when attempting to use -fanalyzer on > > certain gfortran code. The second patch in this kit fixes that, but > > in the meantime I need somewhere to put regression tests for -fanalyzer > > with gfortran. > > > > This patch adds a gfortran.dg/analyzer subdirectory with an analyzer.exp, > > setting DEFAULT_FFLAGS on the tests run within it. > > I have seen no objections against this proposal, so please go ahead. > Perhaps, there are no objections because the people who contribute patches and provide reviews for gfortran have twindled to 1 or 2 people with sporadic available time. Did you actually review the proposed changes? If not, how can you rubber stamp this commit? You have a total of 12 ChangeLog entries over 18 years with the last occurring in 2011, and I do not recall you ever reviewing a patch. Finally, trunk is in stage 4 (regression fixes & docs only). This does not look like either. If I bootstrap gcc with fortran % mkdir obj % ../gcc/configure --prefix=$HOME/work --enable-languages=c,fortran \ --enable-bootstrap --enable-checking=yes % gmake bootstrap and then do % cd gcc % gmake check-fortran are these analyzer testcases built/executed? Do all architectures that support gfortran also support the analyzer infrastructure?
On Sun, 2020-02-09 at 12:55 -0800, Steve Kargl wrote: > On Sun, Feb 09, 2020 at 09:15:46PM +0100, Toon Moene wrote: > > On 2/6/20 9:01 PM, David Malcolm wrote: > > > > > PR analyzer/93405 reports an ICE when attempting to use > > > -fanalyzer on > > > certain gfortran code. The second patch in this kit fixes that, > > > but > > > in the meantime I need somewhere to put regression tests for > > > -fanalyzer > > > with gfortran. > > > > > > This patch adds a gfortran.dg/analyzer subdirectory with an > > > analyzer.exp, > > > setting DEFAULT_FFLAGS on the tests run within it. > > > > I have seen no objections against this proposal, so please go > > ahead. > > > > Perhaps, there are no objections because the people who contribute > patches and provide reviews for gfortran have twindled to 1 or 2 > people > with sporadic available time. Did you actually review the proposed > changes? If not, how can you rubber stamp this commit? You have a > total of 12 ChangeLog entries over 18 years with the last occurring > in > 2011, and I do not recall you ever reviewing a patch. FWIW Toon reported in BZ that patch 2 in the kit fixed the ICE he had reported, and I asked there if he was able to review this patch, which is what led to his email. I'm sorry if I overstepped the mark here. > Finally, trunk > is in stage 4 (regression fixes & docs only). This does not look > like > either. Indeed. The analyzer is a new feature in GCC 10. I'm hoping some latitude can be granted here given it's new (and hence all of its ICEs are, strictly speaking, not regressions), and this is about adding test coverage for fixing them. > If I bootstrap gcc with fortran > > % mkdir obj > % ../gcc/configure --prefix=$HOME/work --enable-languages=c,fortran \ > --enable-bootstrap --enable-checking=yes > % gmake bootstrap > > and then do > > % cd gcc > % gmake check-fortran > > are these analyzer testcases built/executed? That's the intent of the patch, yes (the cases are marked with dg-do compile, so "built", at least, if not "executed"). Note that it's possible to disable the analyzer at configure-time via -fdisable-analyzer; the analyzer.exp checks for this via check_effective_target_analyzer. > Do all architectures > that support gfortran also support the analyzer infrastructure? I believe so, yes; I've fixed bugs on e.g. powerpc-ibm-aix7.2.0.0 since merging (and if not, the check_effective_target_analyzer ought to immediately reject running the tests). That said, I've only verified these testcases on x86_64-pc-linux-gnu so far. An alternative would be to split patch 2, committing the ICE fix to the analyzer, and leaving the test coverage to next stage 1. Thoughts? Thanks Dave
On 2/9/20 9:55 PM, Steve Kargl wrote: > On Sun, Feb 09, 2020 at 09:15:46PM +0100, Toon Moene wrote: >> On 2/6/20 9:01 PM, David Malcolm wrote: >> >>> PR analyzer/93405 reports an ICE when attempting to use -fanalyzer on >>> certain gfortran code. The second patch in this kit fixes that, but >>> in the meantime I need somewhere to put regression tests for -fanalyzer >>> with gfortran. >>> >>> This patch adds a gfortran.dg/analyzer subdirectory with an analyzer.exp, >>> setting DEFAULT_FFLAGS on the tests run within it. >> >> I have seen no objections against this proposal, so please go ahead. >> > > Perhaps, there are no objections because the people who contribute > patches and provide reviews for gfortran have twindled to 1 or 2 people > with sporadic available time. Did you actually review the proposed > changes? You are right. I did test the fix for PR93405, and thought this update was included in that test, but it was not - my fault. I will be more careful in the future about what I test, and show the results (otherwise, why test ...). Suggestion withdrawn.
On Sun, Feb 09, 2020 at 04:19:13PM -0500, David Malcolm wrote: > On Sun, 2020-02-09 at 12:55 -0800, Steve Kargl wrote: > > On Sun, Feb 09, 2020 at 09:15:46PM +0100, Toon Moene wrote: > > > On 2/6/20 9:01 PM, David Malcolm wrote: > > > > > > > PR analyzer/93405 reports an ICE when attempting to use > > > > -fanalyzer on > > > > certain gfortran code. The second patch in this kit fixes that, > > > > but > > > > in the meantime I need somewhere to put regression tests for > > > > -fanalyzer > > > > with gfortran. > > > > > > > > This patch adds a gfortran.dg/analyzer subdirectory with an > > > > analyzer.exp, > > > > setting DEFAULT_FFLAGS on the tests run within it. > > > > > > I have seen no objections against this proposal, so please go > > > ahead. > > > > > > > Perhaps, there are no objections because the people who contribute > > patches and provide reviews for gfortran have twindled to 1 or 2 > > people > > with sporadic available time. Did you actually review the proposed > > changes? If not, how can you rubber stamp this commit? You have a > > total of 12 ChangeLog entries over 18 years with the last occurring > > in > > 2011, and I do not recall you ever reviewing a patch. > > FWIW Toon reported in BZ that patch 2 in the kit fixed the ICE he had > reported, and I asked there if he was able to review this patch, which > is what led to his email. > > I'm sorry if I overstepped the mark here. You didn't overstep the mark. I was questioning the manner in which approval seem to be rubber stamped. > > Finally, trunk > > is in stage 4 (regression fixes & docs only). This does not look > > like > > either. > > Indeed. The analyzer is a new feature in GCC 10. I'm hoping some > latitude can be granted here given it's new (and hence all of its ICEs > are, strictly speaking, not regressions), and this is about adding test > coverage for fixing them. Having now looked at the patch, I think it's okay to commit. As you note, it is new functionality in 10 so technically cannot be a regression. But, it does fix an issue before 10 is even released. OK to commit.
On Sun, 2020-02-09 at 16:38 -0800, Steve Kargl wrote: > On Sun, Feb 09, 2020 at 04:19:13PM -0500, David Malcolm wrote: > > On Sun, 2020-02-09 at 12:55 -0800, Steve Kargl wrote: > > > On Sun, Feb 09, 2020 at 09:15:46PM +0100, Toon Moene wrote: > > > > On 2/6/20 9:01 PM, David Malcolm wrote: > > > > > > > > > PR analyzer/93405 reports an ICE when attempting to use > > > > > -fanalyzer on > > > > > certain gfortran code. The second patch in this kit fixes > > > > > that, > > > > > but > > > > > in the meantime I need somewhere to put regression tests for > > > > > -fanalyzer > > > > > with gfortran. > > > > > > > > > > This patch adds a gfortran.dg/analyzer subdirectory with an > > > > > analyzer.exp, > > > > > setting DEFAULT_FFLAGS on the tests run within it. > > > > > > > > I have seen no objections against this proposal, so please go > > > > ahead. > > > > > > > > > > Perhaps, there are no objections because the people who > > > contribute > > > patches and provide reviews for gfortran have twindled to 1 or 2 > > > people > > > with sporadic available time. Did you actually review the > > > proposed > > > changes? If not, how can you rubber stamp this commit? You have > > > a > > > total of 12 ChangeLog entries over 18 years with the last > > > occurring > > > in > > > 2011, and I do not recall you ever reviewing a patch. > > > > FWIW Toon reported in BZ that patch 2 in the kit fixed the ICE he > > had > > reported, and I asked there if he was able to review this patch, > > which > > is what led to his email. > > > > I'm sorry if I overstepped the mark here. > > You didn't overstep the mark. I was questioning the manner in > which approval seem to be rubber stamped. > > > > Finally, trunk > > > is in stage 4 (regression fixes & docs only). This does not look > > > like > > > either. > > > > Indeed. The analyzer is a new feature in GCC 10. I'm hoping some > > latitude can be granted here given it's new (and hence all of its > > ICEs > > are, strictly speaking, not regressions), and this is about adding > > test > > coverage for fixing them. > > Having now looked at the patch, I think it's okay to commit. > As you note, it is new functionality in 10 so technically > cannot be a regression. But, it does fix an issue before > 10 is even released. > > OK to commit. Thanks. Is the Fortran testcase part of patch 2 OK as well? https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00394.html Dave
On Mon, Feb 10, 2020 at 03:52:13PM -0500, David Malcolm wrote: > On Sun, 2020-02-09 at 16:38 -0800, Steve Kargl wrote: > > > > Having now looked at the patch, I think it's okay to commit. > > As you note, it is new functionality in 10 so technically > > cannot be a regression. But, it does fix an issue before > > 10 is even released. > > > > OK to commit. > > Thanks. > > Is the Fortran testcase part of patch 2 OK as well? > https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00394.html > Yes.
diff --git a/gcc/testsuite/gfortran.dg/analyzer/analyzer.exp b/gcc/testsuite/gfortran.dg/analyzer/analyzer.exp new file mode 100644 index 00000000000..00edfa54dce --- /dev/null +++ b/gcc/testsuite/gfortran.dg/analyzer/analyzer.exp @@ -0,0 +1,55 @@ +# Copyright (C) 2020 Free Software Foundation, Inc. + +# This file is part of GCC. +# +# GCC is free software; you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation; either version 3, or (at your option) any later +# version. +# +# GCC is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +# for more details. +# +# You should have received a copy of the GNU General Public License +# along with GCC; see the file COPYING3. If not see +# <http://www.gnu.org/licenses/>. + +# GCC testsuite that uses the `dg.exp' driver. + +# Load support procs. +load_lib gfortran-dg.exp +load_lib gfortran.exp + +# If the analyzer has not been enabled, bail. +if { ![check_effective_target_analyzer] } { + return +} + +global DEFAULT_FFLAGS +if [info exists DEFAULT_FFLAGS] then { + set save_default_fflags $DEFAULT_FFLAGS +} + +# If a testcase doesn't have special options, use these. +set DEFAULT_FFLAGS "-fanalyzer -fdiagnostics-path-format=separate-events -Wanalyzer-too-complex -fanalyzer-call-summaries" + +# Initialize `dg'. +dg-init + +# Main loop. + +gfortran_init + +gfortran-dg-runtest [lsort \ + [glob -nocomplain $srcdir/$subdir/*.\[fF\]{,90,95,03,08} ] ] "" $DEFAULT_FFLAGS + +# All done. +dg-finish + +if [info exists save_default_fflags] { + set DEFAULT_FFLAGS $save_default_fflags +} else { + unset DEFAULT_FFLAGS +} diff --git a/gcc/testsuite/gfortran.dg/analyzer/malloc-example.f90 b/gcc/testsuite/gfortran.dg/analyzer/malloc-example.f90 new file mode 100644 index 00000000000..4c48d415e05 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/analyzer/malloc-example.f90 @@ -0,0 +1,21 @@ +! Example from GCC documentation +! { dg-do compile } +! { dg-additional-options "-fcray-pointer" } + +program test_malloc + implicit none + integer i + real*8 x(*), z + pointer(ptr_x,x) + + ptr_x = malloc(20*8) + do i = 1, 20 + x(i) = sqrt(1.0d0 / i) + end do + z = 0 + do i = 1, 20 + z = z + x(i) + print *, z + end do + call free(ptr_x) +end program test_malloc diff --git a/gcc/testsuite/gfortran.dg/analyzer/malloc.f90 b/gcc/testsuite/gfortran.dg/analyzer/malloc.f90 new file mode 100644 index 00000000000..05a0bcc3a53 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/analyzer/malloc.f90 @@ -0,0 +1,19 @@ +! { dg-do compile } +! { dg-additional-options "-fcray-pointer -O0" } + +subroutine test_ok + real*8 x(*) + pointer(ptr_x,x) + + ptr_x = malloc(20*8) + call free(ptr_x) +end subroutine test_ok ! { dg-bogus "leak" } + +subroutine test_double_free + real*8 x(*) + pointer(ptr_x,x) + + ptr_x = malloc(20*8) + call free(ptr_x) + call free(ptr_x) ! { dg-warning "double-'free'" } +end subroutine test_double_free ! { dg-bogus "leak" }
PR analyzer/93405 reports an ICE when attempting to use -fanalyzer on certain gfortran code. The second patch in this kit fixes that, but in the meantime I need somewhere to put regression tests for -fanalyzer with gfortran. This patch adds a gfortran.dg/analyzer subdirectory with an analyzer.exp, setting DEFAULT_FFLAGS on the tests run within it. It also adds a couple of simple proof-of-concept tests of e.g. detecting double-frees from gfortran. These work, though there are some issues with the output: (a) the double-free is reported as: Warning: double-‘free’ of ‘_1’ rather than: Warning: double-‘free’ of ‘ptr_x’ (b) the default output format for diagnostic paths is -fdiagnostics-path-format=inline-events but the various events in the path all have column == 0, and the path-printing doesn't do a good job of that (the event descriptions don't show up) With -fdiagnostics-path-format=separate-events, the output looks like: ../../src/gcc/testsuite/gfortran.dg/analyzer/malloc.f90:18:0: 18 | call free(ptr_x) ! { dg-warning "double-'free'" } | Warning: double-‘free’ of ‘_1’ [CWE-415] [-Wanalyzer-double-free] ../../src/gcc/testsuite/gfortran.dg/analyzer/malloc.f90:16:0: 16 | ptr_x = malloc(20*8) | note: (1) allocated here ../../src/gcc/testsuite/gfortran.dg/analyzer/malloc.f90:17:0: 17 | call free(ptr_x) | note: (2) first ‘free’ here ../../src/gcc/testsuite/gfortran.dg/analyzer/malloc.f90:18:0: 18 | call free(ptr_x) ! { dg-warning "double-'free'" } | note: (3) second ‘free’ here; first ‘free’ was at (2) In any case, is this OK for master? (as a place to put such tests, and do the tests look sane? I'm not an expert at Fortran, sorry). Successfully tested on x86_64-pc-linux-gnu; the combination of the two patches add 6 PASS results to gfortran.sum gcc/testsuite/ChangeLog: * gfortran.dg/analyzer/analyzer.exp: New subdirectory and .exp suite. * gfortran.dg/analyzer/malloc-example.f90: New test. * gfortran.dg/analyzer/malloc.f90: New test. --- .../gfortran.dg/analyzer/analyzer.exp | 55 +++++++++++++++++++ .../gfortran.dg/analyzer/malloc-example.f90 | 21 +++++++ gcc/testsuite/gfortran.dg/analyzer/malloc.f90 | 19 +++++++ 3 files changed, 95 insertions(+) create mode 100644 gcc/testsuite/gfortran.dg/analyzer/analyzer.exp create mode 100644 gcc/testsuite/gfortran.dg/analyzer/malloc-example.f90 create mode 100644 gcc/testsuite/gfortran.dg/analyzer/malloc.f90