[PATCH] squash spurious warnings in dominance.c

[PATCH] squash spurious warnings in dominance.c

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

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

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);


[-- Attachment #2: gcc-80486.diff --]
[-- Type: text/x-patch, Size: 4178 bytes --]

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,

Re: [PATCH] squash spurious warnings in dominance.c

[Richard Biener]

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);
>

Re: [PATCH] squash spurious warnings in dominance.c

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On 04/24/2017 01:32 AM, Richard Biener wrote:
> 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.

Attached is this version of the patch.  It also eliminates
the warnings and passes profiledbootstrap/regression test
on x86_64.

Martin


[-- Attachment #2: gcc-80486.diff --]
[-- Type: text/x-patch, Size: 2122 bytes --]

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

gcc/ChangeLog:

	PR bootstrap/80486
	* dominance.c (dom_info::m_n_basic_blocks): Change type to unsigned.
	(new_zero_array): Adjust signature.
	(dom_info::dom_init): Used unsigned rather that size_t.
	(dom_info::dom_info): Same.

diff --git a/gcc/dominance.c b/gcc/dominance.c
index c76e62e..1d4bd54 100644
--- a/gcc/dominance.c
+++ b/gcc/dominance.c
@@ -125,7 +125,7 @@ private:
   bitmap m_fake_exit_edge;
 
   /* Number of basic blocks in the function being compiled.  */
-  size_t m_n_basic_blocks;
+  unsigned m_n_basic_blocks;
 
   /* True, if we are computing postdominators (rather than dominators).  */
   bool m_reverse;
@@ -148,7 +148,7 @@ void debug_dominance_tree (cdi_direction, basic_block);
    `x = new T[num] {};'.  */
 
 template<typename T>
-inline T *new_zero_array (size_t num)
+inline T *new_zero_array (unsigned num)
 {
   T *result = new T[num];
   memset (result, 0, sizeof (T) * num);
@@ -160,14 +160,15 @@ inline T *new_zero_array (size_t num)
 void
 dom_info::dom_init (void)
 {
-  size_t num = m_n_basic_blocks;
+  unsigned num = m_n_basic_blocks;
+
   m_dfs_parent = new_zero_array <TBB> (num);
   m_dom = new_zero_array <TBB> (num);
 
   m_path_min = new TBB[num];
   m_key = new TBB[num];
   m_set_size = new unsigned int[num];
-  for (size_t i = 0; i < num; i++)
+  for (unsigned i = 0; i < num; i++)
     {
       m_path_min[i] = m_key[i] = i;
       m_set_size[i] = 1;
@@ -221,13 +222,13 @@ dom_info::dom_info (function *fn, cdi_direction dir)
 dom_info::dom_info (vec<basic_block> region, cdi_direction dir)
 {
   m_n_basic_blocks = region.length ();
-  unsigned int nm1 = m_n_basic_blocks - 1;
+  unsigned nm1 = m_n_basic_blocks - 1;
 
   dom_init ();
 
   /* Determine max basic block index in region.  */
   int max_index = region[0]->index;
-  for (size_t i = 1; i <= nm1; i++)
+  for (unsigned i = 1; i <= nm1; i++)
     if (region[i]->index > max_index)
       max_index = region[i]->index;
   max_index += 1;  /* set index on the first bb out of region.  */

Re: [PATCH] squash spurious warnings in dominance.c

[Richard Biener]

On Mon, Apr 24, 2017 at 11:07 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 04/24/2017 01:32 AM, Richard Biener wrote:
>>
>> 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.
>
>
> Attached is this version of the patch.  It also eliminates
> the warnings and passes profiledbootstrap/regression test
> on x86_64.

Ok.

Thanks,
Richard.

> Martin
>