Re: [patch] move array bounds checking into its own file

[Aldy Hernandez via Gcc-patches]

On Sun, May 31, 2020 at 7:31 PM Jeff Law  wrote:
>
> On Mon, 2020-05-18 at 20:11 +0200, Aldy Hernandez wrote:
> > As a follow-up to the patch moving array bounds checking into its own
> > class, this moves the class into its own files.  As I've mentioned
> > previously, having it in tree-vrp just pollutes the file with unrelated
> > stuff.
> >
> > Jeff, I know you've mentioned you'd like to move the array bounds
> > checking to the path isolation pass at some point.  Would the current
> > approach work for you?  I'm ok, inasmuch as it's gone from tree-vrp.c,
> > the file is bloated enough as it is.
> Yes, this is fine.  There's nothing conceptually about the array bounds 
> checking
> that requires it to live in VRP -- it was a historical decision because 
> access to
> the IL after ASSERT_EXPR insertion gave better results.
>
> As you probably remember, it was your investigative work on moving the array
> bounds bits out of VRP with an eye towards isolating them and turning them 
> into
> trap that was a significant factor in kicking off the Ranger project years 
> ago.
>
> This might need minor edits to make sure you don't lose Martin's work from 
> May 18
> in check_mem_ref().

Thanks, and good catch.  I will re-test and push.

Aldy

Re: [patch] move array bounds checking into its own file

[Jeff Law via Gcc-patches]

On Mon, 2020-05-18 at 20:11 +0200, Aldy Hernandez wrote:
> As a follow-up to the patch moving array bounds checking into its own 
> class, this moves the class into its own files.  As I've mentioned 
> previously, having it in tree-vrp just pollutes the file with unrelated 
> stuff.
> 
> Jeff, I know you've mentioned you'd like to move the array bounds 
> checking to the path isolation pass at some point.  Would the current 
> approach work for you?  I'm ok, inasmuch as it's gone from tree-vrp.c, 
> the file is bloated enough as it is.
Yes, this is fine.  There's nothing conceptually about the array bounds checking
that requires it to live in VRP -- it was a historical decision because access 
to
the IL after ASSERT_EXPR insertion gave better results.

As you probably remember, it was your investigative work on moving the array
bounds bits out of VRP with an eye towards isolating them and turning them into
trap that was a significant factor in kicking off the Ranger project years ago.

This might need minor edits to make sure you don't lose Martin's work from May 
18
in check_mem_ref().
Jeff

Re: [patch] move array bounds checking into its own file

[Jeff Law via Gcc-patches]

On Tue, 2020-05-19 at 09:10 +0200, Richard Biener wrote:
> On Mon, May 18, 2020 at 8:21 PM Aldy Hernandez via Gcc-patches
>  wrote:
> > As a follow-up to the patch moving array bounds checking into its own
> > class, this moves the class into its own files.  As I've mentioned
> > previously, having it in tree-vrp just pollutes the file with unrelated
> > stuff.
> > 
> > Jeff, I know you've mentioned you'd like to move the array bounds
> > checking to the path isolation pass at some point.  Would the current
> > approach work for you?  I'm ok, inasmuch as it's gone from tree-vrp.c,
> > the file is bloated enough as it is.
> > wc -l gcc/tree-vrp.c
> 5586 gcc/tree-vrp.c
> > wc -l gcc/vr-values.c
> 4361 gcc/vr-values.c
> > wc -l gcc/range-op.cc
> 3113 gcc/range-op.cc
> 
> I wouldn't call this bloated.  Moving to separate files is hardly
> a good thing on its own.
But there's no good reason to have these bits in VRP to begin with.  The only
reason they're there is they get more accurate data because of the ASSERT_EXPRs
and PHI nodes due to additional definitions.

Jeff

Re: [patch] move array bounds checking into its own file

[Richard Biener via Gcc-patches]

On Mon, May 18, 2020 at 8:21 PM Aldy Hernandez via Gcc-patches
 wrote:
>
> As a follow-up to the patch moving array bounds checking into its own
> class, this moves the class into its own files.  As I've mentioned
> previously, having it in tree-vrp just pollutes the file with unrelated
> stuff.
>
> Jeff, I know you've mentioned you'd like to move the array bounds
> checking to the path isolation pass at some point.  Would the current
> approach work for you?  I'm ok, inasmuch as it's gone from tree-vrp.c,
> the file is bloated enough as it is.

> wc -l gcc/tree-vrp.c
5586 gcc/tree-vrp.c
> wc -l gcc/vr-values.c
4361 gcc/vr-values.c
> wc -l gcc/range-op.cc
3113 gcc/range-op.cc

I wouldn't call this bloated.  Moving to separate files is hardly
a good thing on its own.

>
> Aldy

[patch] move array bounds checking into its own file

[Aldy Hernandez via Gcc-patches]

As a follow-up to the patch moving array bounds checking into its own 
class, this moves the class into its own files.  As I've mentioned 
previously, having it in tree-vrp just pollutes the file with unrelated 
stuff.


Jeff, I know you've mentioned you'd like to move the array bounds 
checking to the path isolation pass at some point.  Would the current 
approach work for you?  I'm ok, inasmuch as it's gone from tree-vrp.c, 
the file is bloated enough as it is.


Aldy
commit 6bc1b4c62b2704e18f5c8564e7fad144b63314b7
Author: Aldy Hernandez 
Date:   Sun May 17 15:03:20 2020 +0200

Move array bounds checking into its own file.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 60ff1ffceef..7e42099e03d 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2020-05-17  Aldy Hernandez  
+
+	* Makefile.in (gimple-array-bounds.o): New.
+	* tree-vrp.h/cc: Move array bounds code...
+	* gimple-array-bounds.h/cc: ...here.
+
 2020-05-17  Aldy Hernandez  
 
 	Revert:
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 9ba21f735f6..fcf6db8afb4 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1349,6 +1349,7 @@ OBJS = \
 	ggc-common.o \
 	ggc-tests.o \
 	gimple.o \
+	gimple-array-bounds.o \
 	gimple-builder.o \
 	gimple-expr.o \
 	gimple-iterator.o \
diff --git a/gcc/gimple-array-bounds.cc b/gcc/gimple-array-bounds.cc
new file mode 100644
index 000..68ca1c5ee7a
--- /dev/null
+++ b/gcc/gimple-array-bounds.cc
@@ -0,0 +1,714 @@
+/* Array bounds checking.
+   Copyright (C) 2005-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
+.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "backend.h"
+#include "tree.h"
+#include "gimple.h"
+#include "ssa.h"
+#include "gimple-array-bounds.h"
+#include "gimple-iterator.h"
+#include "gimple-walk.h"
+#include "tree-dfa.h"
+#include "fold-const.h"
+#include "diagnostic-core.h"
+#include "intl.h"
+#include "tree-vrp.h"
+#include "alloc-pool.h"
+#include "vr-values.h"
+#include "domwalk.h"
+#include "tree-cfg.h"
+
+// This purposely returns a value_range, not a value_range_equiv, to
+// break the dependency on equivalences for this pass.
+
+const value_range *
+array_bounds_checker::get_value_range (const_tree op)
+{
+  return ranges->get_value_range (op);
+}
+
+/* Checks one ARRAY_REF in REF, located at LOCUS. Ignores flexible
+   arrays and "struct" hacks. If VRP can determine that the array
+   subscript is a constant, check if it is outside valid range.  If
+   the array subscript is a RANGE, warn if it is non-overlapping with
+   valid range.  IGNORE_OFF_BY_ONE is true if the ARRAY_REF is inside
+   a ADDR_EXPR.  Returns true if a warning has been issued.  */
+
+bool
+array_bounds_checker::check_array_ref (location_t location, tree ref,
+   bool ignore_off_by_one)
+{
+  if (TREE_NO_WARNING (ref))
+return false;
+
+  tree low_sub = TREE_OPERAND (ref, 1);
+  tree up_sub = low_sub;
+  tree up_bound = array_ref_up_bound (ref);
+
+  /* Referenced decl if one can be determined.  */
+  tree decl = NULL_TREE;
+
+  /* Set for accesses to interior zero-length arrays.  */
+  bool interior_zero_len = false;
+
+  tree up_bound_p1;
+
+  if (!up_bound
+  || TREE_CODE (up_bound) != INTEGER_CST
+  || (warn_array_bounds < 2
+	  && array_at_struct_end_p (ref)))
+{
+  /* Accesses to trailing arrays via pointers may access storage
+	 beyond the types array bounds.  For such arrays, or for flexible
+	 array members, as well as for other arrays of an unknown size,
+	 replace the upper bound with a more permissive one that assumes
+	 the size of the largest object is PTRDIFF_MAX.  */
+  tree eltsize = array_ref_element_size (ref);
+
+  if (TREE_CODE (eltsize) != INTEGER_CST
+	  || integer_zerop (eltsize))
+	{
+	  up_bound = NULL_TREE;
+	  up_bound_p1 = NULL_TREE;
+	}
+  else
+	{
+	  tree ptrdiff_max = TYPE_MAX_VALUE (ptrdiff_type_node);
+	  tree maxbound = ptrdiff_max;
+	  tree arg = TREE_OPERAND (ref, 0);
+
+	  const bool compref = TREE_CODE (arg) == COMPONENT_REF;
+	  if (compref)
+	{
+	  /* Try to determine the size of the trailing array from
+		 its initializer (if it has one).  */
+	  if (tree refsize = component_ref_size (arg, _zero_len))
+		if (TREE_CODE (refsize) == INTEGER_CST)
+		  maxbound = refsize;
+	}
+
+	  if (maxbound == ptrdiff_max)
+	{
+	  /* Try to determine the