[PATCH 2/2] analyzer: fix ICE with fortran constant arguments (PR 93405)

[PATCH 2/2] analyzer: fix ICE with fortran constant arguments (PR 93405)

[David Malcolm]

PR analyzer/93405 reports an ICE with -fanalyzer when passing
a constant "by reference" in gfortran.

The issue is that the constant is passed as an ADDR_EXPR
of a CONST_DECL, and region_model::get_lvalue_1 doesn't
know how to handle CONST_DECL.

This patch implements it for CONST_DECL by providing
a placeholder region, holding the CONST_DECL's value,
fixing the ICE.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

Is the Fortran testcase OK for master?  (this relies on patch 1 in
the kit, obviously)

gcc/analyzer/ChangeLog:
	PR analyzer/93405
	* region-model.cc (region_model::get_lvalue_1): Implement
	CONST_DECL.

gcc/testsuite/ChangeLog:
	PR analyzer/93405
	* gfortran.dg/analyzer/pr93405.f90: New test.
---
 gcc/analyzer/region-model.cc                   | 13 +++++++++++++
 gcc/testsuite/gfortran.dg/analyzer/pr93405.f90 | 14 ++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 gcc/testsuite/gfortran.dg/analyzer/pr93405.f90

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 8b57a623084..61390aa4cd1 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -4717,6 +4717,19 @@ region_model::get_lvalue_1 (path_var pv, region_model_context *ctxt)
       }
       break;
 
+    case CONST_DECL:
+      {
+	tree cst_type = TREE_TYPE (expr);
+	region_id cst_rid = add_region_for_type (m_root_rid, cst_type);
+	if (tree value = DECL_INITIAL (expr))
+	  {
+	    svalue_id sid = get_rvalue (value, ctxt);
+	    get_region (cst_rid)->set_value (*this, cst_rid, sid, ctxt);
+	  }
+	return cst_rid;
+      }
+      break;
+
     case STRING_CST:
       {
 	tree cst_type = TREE_TYPE (expr);
diff --git a/gcc/testsuite/gfortran.dg/analyzer/pr93405.f90 b/gcc/testsuite/gfortran.dg/analyzer/pr93405.f90
new file mode 100644
index 00000000000..e2c23753015
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/analyzer/pr93405.f90
@@ -0,0 +1,14 @@
+! { dg-do compile }
+real a(10), b(10), c(10)
+a = 0.
+b = 1.
+call sum(a, b, c, 10)
+print *, c(5)
+end
+subroutine sum(a, b, c, n)
+integer i, n
+real a(n), b(n), c(n)
+do i = 1, n
+   c(i) = a(i) + b(i)
+enddo
+end
-- 
2.21.0

[PATCH 1/2] analyzer: gfortran testsuite support

[David Malcolm]

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

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" }
-- 
2.21.0

Re: [PATCH 1/2] analyzer: gfortran testsuite support

[Toon Moene]

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,

-- 
Toon Moene - e-mail: toon@moene.org - phone: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/
Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news

Re: [PATCH 1/2] analyzer: gfortran testsuite support

[Steve Kargl]

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?

-- 
Steve

Re: [PATCH 1/2] analyzer: gfortran testsuite support

[David Malcolm]

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

Re: [PATCH 1/2] analyzer: gfortran testsuite support

[Toon Moene]

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.

-- 
Toon Moene - e-mail: toon@moene.org - phone: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/
Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news

Re: [PATCH 1/2] analyzer: gfortran testsuite support

[Steve Kargl]

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.

-- 
Steve

Re: [PATCH 1/2] analyzer: gfortran testsuite support

[David Malcolm]

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

Re: [PATCH 1/2] analyzer: gfortran testsuite support

[Steve Kargl]

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.

-- 
Steve