diff mbox

squash spurious warnings in dominance.c

Message ID b79b6819-8e7d-3a88-c560-107865c35bb5@gmail.com
State New
Headers show

Commit Message

Martin Sebor April 22, 2017, 12:51 a.m. UTC 
Bug 80486 - spurious -Walloc-size-larger-than and
-Wstringop-overflow in dominance.c during profiledbootstrap
points out a number of warnings that show up in dominance.c
during a profiledbootstrap.  I'm pretty sure the warnings
are due to the size check the C++ new expression introduces
to avoid unsigned overflow before calling operator new, and
by some optimization like jump threading introducing a branch
with the call to the allocation function and memset with
the excessive constant size.

Two ways to avoid it come to mind: 1) use the libiberty
XCNEWVEC and XNEWVEC macros instead of C++ new expressions,
and 2) constraining the size variable to a valid range.

Either of these approaches should result in better code than
the new expression because they both eliminate the test for
the overflow.  Attached is a patch that implements (1). I
chose it mainly because it seems in line with GCC's memory
management policy and with avoiding exceptions.

An alternate patch should be straightforward.  Either add
an assert like the one below or change the type of
m_n_basic_blocks from size_t to unsigned.  This approach,
though less intrusive, will likely bring the warning back
in ILP32 builds; I'm not sure if it matters.

Martin

Comments

Richard Biener April 24, 2017, 7:32 a.m. UTC | #1 
On Sat, Apr 22, 2017 at 2:51 AM, Martin Sebor <msebor@gmail.com> wrote:
> Bug 80486 - spurious -Walloc-size-larger-than and
> -Wstringop-overflow in dominance.c during profiledbootstrap
> points out a number of warnings that show up in dominance.c
> during a profiledbootstrap.  I'm pretty sure the warnings
> are due to the size check the C++ new expression introduces
> to avoid unsigned overflow before calling operator new, and
> by some optimization like jump threading introducing a branch
> with the call to the allocation function and memset with
> the excessive constant size.
>
> Two ways to avoid it come to mind: 1) use the libiberty
> XCNEWVEC and XNEWVEC macros instead of C++ new expressions,
> and 2) constraining the size variable to a valid range.
>
> Either of these approaches should result in better code than
> the new expression because they both eliminate the test for
> the overflow.  Attached is a patch that implements (1). I
> chose it mainly because it seems in line with GCC's memory
> management policy and with avoiding exceptions.
>
> An alternate patch should be straightforward.  Either add
> an assert like the one below or change the type of
> m_n_basic_blocks from size_t to unsigned.  This approach,
> though less intrusive, will likely bring the warning back
> in ILP32 builds; I'm not sure if it matters.

Please change m_n_basic_blocks (and local copies) from size_t
to unsigned int.  This is an odd inconsistency that's worth fixing
in any case.

Richard.

> Martin
>
> diff --git a/gcc/dominance.c b/gcc/dominance.c
> index c76e62e..ebb0a8f 100644
> --- a/gcc/dominance.c
> +++ b/gcc/dominance.c
> @@ -161,6 +161,9 @@ void
>  dom_info::dom_init (void)
>  {
>    size_t num = m_n_basic_blocks;
> +
> +  gcc_assert (num < SIZE_MAX / sizeof (basic_block) / 2);
> +
>    m_dfs_parent = new_zero_array <TBB> (num);
>    m_dom = new_zero_array <TBB> (num);
>
diff mbox

Patch

PR bootstrap/80486 - spurious -Walloc-size-larger-than and -Wstringop-overflow in dominance.c during profiledbootstrap

gcc/ChangeLog:

	PR bootstrap/80486
	* dominance.c (new_zero_array): Remove.
	(dom_info::dom_init): Use XCNEWVEC and XNEWVEC instead of new_zero_array.
	(dom_info::dom_info): Same.
	(dom_info::calc_dfs_tree_nonrec): Same.
	(dom_info::~dom_info): Use XDELETEVEC instead of delete.

diff --git a/gcc/dominance.c b/gcc/dominance.c
index c76e62e..3d4b683 100644
--- a/gcc/dominance.c
+++ b/gcc/dominance.c
@@ -142,44 +142,31 @@  private:
 void debug_dominance_info (cdi_direction);
 void debug_dominance_tree (cdi_direction, basic_block);
 
-/* Allocate and zero-initialize NUM elements of type T (T must be a
-   POD-type).  Note: after transition to C++11 or later,
-   `x = new_zero_array <T> (num);' can be replaced with
-   `x = new T[num] {};'.  */
-
-template<typename T>
-inline T *new_zero_array (size_t num)
-{
-  T *result = new T[num];
-  memset (result, 0, sizeof (T) * num);
-  return result;
-}
-
 /* Helper function for constructors to initialize a part of class members.  */
 
 void
 dom_info::dom_init (void)
 {
   size_t num = m_n_basic_blocks;
-  m_dfs_parent = new_zero_array <TBB> (num);
-  m_dom = new_zero_array <TBB> (num);
+  m_dfs_parent = XCNEWVEC (TBB, num);
+  m_dom = XCNEWVEC (TBB, num);
 
-  m_path_min = new TBB[num];
-  m_key = new TBB[num];
-  m_set_size = new unsigned int[num];
+  m_path_min = XNEWVEC (TBB, num);
+  m_key = XNEWVEC (TBB, num);
+  m_set_size = XNEWVEC (unsigned int, num);
   for (size_t i = 0; i < num; i++)
     {
       m_path_min[i] = m_key[i] = i;
       m_set_size[i] = 1;
     }
 
-  m_bucket = new_zero_array <TBB> (num);
-  m_next_bucket = new_zero_array <TBB> (num);
+  m_bucket = XCNEWVEC (TBB, num);
+  m_next_bucket = XCNEWVEC (TBB, num);
 
-  m_set_chain = new_zero_array <TBB> (num);
-  m_set_child = new_zero_array <TBB> (num);
+  m_set_chain = XCNEWVEC (TBB, num);
+  m_set_child = XCNEWVEC (TBB, num);
 
-  m_dfs_to_bb = new_zero_array <basic_block> (num);
+  m_dfs_to_bb = XCNEWVEC (basic_block, num);
 
   m_dfsnum = 1;
   m_nodes = 0;
@@ -194,7 +181,7 @@  dom_info::dom_info (function *fn, cdi_direction dir)
   dom_init ();
 
   unsigned last_bb_index = last_basic_block_for_fn (fn);
-  m_dfs_order = new_zero_array <TBB> (last_bb_index + 1);
+  m_dfs_order = XCNEWVEC (TBB, last_bb_index + 1);
   m_dfs_last = &m_dfs_order[last_bb_index];
 
   switch (dir)
@@ -232,7 +219,7 @@  dom_info::dom_info (vec<basic_block> region, cdi_direction dir)
       max_index = region[i]->index;
   max_index += 1;  /* set index on the first bb out of region.  */
 
-  m_dfs_order = new_zero_array <TBB> (max_index + 1);
+  m_dfs_order = XCNEWVEC (TBB, max_index + 1);
   m_dfs_last = &m_dfs_order[max_index];
 
   m_fake_exit_edge = NULL; /* Assume that region is reducible.  */
@@ -277,17 +264,17 @@  dom_convert_dir_to_idx (cdi_direction dir)
 
 dom_info::~dom_info ()
 {
-  delete[] m_dfs_parent;
-  delete[] m_path_min;
-  delete[] m_key;
-  delete[] m_dom;
-  delete[] m_bucket;
-  delete[] m_next_bucket;
-  delete[] m_set_chain;
-  delete[] m_set_size;
-  delete[] m_set_child;
-  delete[] m_dfs_order;
-  delete[] m_dfs_to_bb;
+  XDELETEVEC (m_dfs_parent);
+  XDELETEVEC (m_path_min);
+  XDELETEVEC (m_key);
+  XDELETEVEC (m_dom);
+  XDELETEVEC (m_bucket);
+  XDELETEVEC (m_next_bucket);
+  XDELETEVEC (m_set_chain);
+  XDELETEVEC (m_set_size);
+  XDELETEVEC (m_set_child);
+  XDELETEVEC (m_dfs_order);
+  XDELETEVEC (m_dfs_to_bb);
   BITMAP_FREE (m_fake_exit_edge);
 }
 
@@ -300,7 +287,7 @@  dom_info::~dom_info ()
 void
 dom_info::calc_dfs_tree_nonrec (basic_block bb)
 {
-  edge_iterator *stack = new edge_iterator[m_n_basic_blocks + 1];
+  edge_iterator *stack = XNEWVEC (edge_iterator, m_n_basic_blocks + 1);
   int sp = 0;
   unsigned d_i = dom_convert_dir_to_idx (m_reverse ? CDI_POST_DOMINATORS
 					 : CDI_DOMINATORS);
@@ -384,7 +371,7 @@  dom_info::calc_dfs_tree_nonrec (basic_block bb)
          descendants or the tree depth.  */
       ei_next (&ei);
     }
-  delete[] stack;
+  XDELETEVEC (stack);
 }
 
 /* The main entry for calculating the DFS tree or forest.  m_reverse is true,