analyzer: fix ICE on pointer arithmetic with incomplete types [PR 93774]

Message ID 20200218012913.11549-1-dmalcolm@redhat.com
State New
Headers show
Series
  • analyzer: fix ICE on pointer arithmetic with incomplete types [PR 93774]
Related show

Commit Message

David Malcolm Feb. 18, 2020, 1:29 a.m.
PR analyzer/93774 reports an ICE in gfortran with -fanalyzer within
region_model::convert_byte_offset_to_array_index on a pointer of
incomplete type ("character(kind=1)[0:][1:0] * restrict").

This patch bulletproofs the routine against incomplete types, fixing
the ICE.

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

I think I have permission to self-approve the analyzer part.

Is the Fortran testcase OK for master?

gcc/analyzer/ChangeLog:
	PR analyzer/93774
	* region-model.cc
	(region_model::convert_byte_offset_to_array_index): Use
	int_size_in_bytes before calling size_in_bytes, to gracefully fail
	on incomplete types.

gcc/testsuite/ChangeLog:
	PR analyzer/93774
	* gfortran.dg/analyzer/deferred_character_25.f90: New test.
---
 gcc/analyzer/region-model.cc                  | 33 ++++++++++---------
 .../analyzer/deferred_character_25.f90        |  3 ++
 2 files changed, 21 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/analyzer/deferred_character_25.f90

-- 
2.21.0

Comments

Thomas König Feb. 18, 2020, 7:57 a.m. | #1
Hi David

in principle, any valid test case (especially for an ICE) should count as obvious and simple, so no approval should be needed.

Having said that, I think I would prefer a copy of the original test case rather than an include statement. Although we usually do not change or remove test cases, sometimes it is done for one reason or anotjer, and in this case I would prefer that the derived test case does not change automatically.

So, all four of your test cases are OK if you change those which use include to a plain test case, maybe with a comment pointing to the original one. In the future, you can just commit this kind of test case as simple and obvious; we would appreciate a note to fortran@ if you do so.

Regards

Thomas
David Malcolm Feb. 18, 2020, 10:32 a.m. | #2
On Tue, 2020-02-18 at 08:57 +0100, Thomas König wrote:
> Hi David

> 

> in principle, any valid test case (especially for an ICE) should

> count as obvious and simple, so no approval should be needed.

> 

> Having said that, I think I would prefer a copy of the original test

> case rather than an include statement. Although we usually do not

> change or remove test cases, sometimes it is done for one reason or

> anotjer, and in this case I would prefer that the derived test case

> does not change automatically.


Thanks - I wasn't sure which approach was preferable here.

> So, all four of your test cases are OK if you change those which use

> include to a plain test case, maybe with a comment pointing to the

> original one. 


I'll go ahead and rework those tests accordingly.

> In the future, you can just commit this kind of test case as simple

> and obvious; we would appreciate a note to fortran@ if you do so.


Thanks - will do.

Dave

Patch

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index deb201546f3..659255a8db4 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -6514,24 +6514,27 @@  region_model::convert_byte_offset_to_array_index (tree ptr_type,
 
       /* Arithmetic on void-pointers is a GNU C extension, treating the size
 	 of a void as 1.
-	 https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
-
-	 Returning early for this case avoids a diagnostic from within the
-	 call to size_in_bytes.  */
+	 https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html  */
       if (TREE_CODE (elem_type) == VOID_TYPE)
 	return offset_sid;
 
-      /* This might not be a constant.  */
-      tree byte_size = size_in_bytes (elem_type);
-
-      /* Try to get a constant by dividing, ensuring that we're in a
-	 signed representation first.  */
-      tree index
-	= fold_binary (TRUNC_DIV_EXPR, ssizetype,
-		       fold_convert (ssizetype, offset_cst),
-		       fold_convert (ssizetype, byte_size));
-      if (index && TREE_CODE (index) == INTEGER_CST)
-	return get_or_create_constant_svalue (index);
+      /* First, use int_size_in_bytes, to reject the case where we have an
+	 incomplete type, or a non-constant value.  */
+      HOST_WIDE_INT hwi_byte_size = int_size_in_bytes (elem_type);
+      if (hwi_byte_size > 0)
+	{
+	  /* Now call size_in_bytes to get the answer in tree form.  */
+	  tree byte_size = size_in_bytes (elem_type);
+	  gcc_assert (byte_size);
+	  /* Try to get a constant by dividing, ensuring that we're in a
+	     signed representation first.  */
+	  tree index
+	    = fold_binary (TRUNC_DIV_EXPR, ssizetype,
+			   fold_convert (ssizetype, offset_cst),
+			   fold_convert (ssizetype, byte_size));
+	  if (index && TREE_CODE (index) == INTEGER_CST)
+	    return get_or_create_constant_svalue (index);
+	}
     }
 
   /* Otherwise, we don't know the array index; generate a new unknown value.
diff --git a/gcc/testsuite/gfortran.dg/analyzer/deferred_character_25.f90 b/gcc/testsuite/gfortran.dg/analyzer/deferred_character_25.f90
new file mode 100644
index 00000000000..09303630d98
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/analyzer/deferred_character_25.f90
@@ -0,0 +1,3 @@ 
+! { dg-do compile }
+! { dg-additional-options "-Wno-analyzer-too-complex" }
+include "../deferred_character_25.f90"