Fix objsz ICE (PR tree-optimization/92056)
diff mbox series

Message ID 20191017070024.GV2116@tucnak
State New
Headers show
Series
  • Fix objsz ICE (PR tree-optimization/92056)
Related show

Commit Message

Jakub Jelinek Oct. 17, 2019, 7 a.m. UTC
Hi!

The following bug has been introduced when cond_expr_object_size has been
added in 2007.  We want to treat a COND_EXPR like a PHI with 2 arguments,
and PHI is handled in a loop that breaks if the lhs value is unknown, and
then does the if (TREE_CODE (arg) == SSA_NAME) merge_object_sizes else
expr_object_size which is used even in places that handle just a single
operand (with the lhs value initialized to the opposite value of unknown
first).  At least expr_object_size asserts that the lhs value is not
unknown at the start.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2019-10-17  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/92056
	* tree-object-size.c (cond_expr_object_size): Return early if then_
	processing resulted in unknown size.

	* gcc.c-torture/compile/pr92056.c: New test.


	Jakub

Comments

Richard Biener Oct. 17, 2019, 7:17 a.m. UTC | #1
On Thu, 17 Oct 2019, Jakub Jelinek wrote:

> Hi!
> 
> The following bug has been introduced when cond_expr_object_size has been
> added in 2007.  We want to treat a COND_EXPR like a PHI with 2 arguments,
> and PHI is handled in a loop that breaks if the lhs value is unknown, and
> then does the if (TREE_CODE (arg) == SSA_NAME) merge_object_sizes else
> expr_object_size which is used even in places that handle just a single
> operand (with the lhs value initialized to the opposite value of unknown
> first).  At least expr_object_size asserts that the lhs value is not
> unknown at the start.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?

OK.

> 2019-10-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/92056
> 	* tree-object-size.c (cond_expr_object_size): Return early if then_
> 	processing resulted in unknown size.
> 
> 	* gcc.c-torture/compile/pr92056.c: New test.
> 
> --- gcc/tree-object-size.c.jj	2019-10-05 09:35:14.895967464 +0200
> +++ gcc/tree-object-size.c	2019-10-16 15:34:11.414769994 +0200
> @@ -903,6 +903,9 @@ cond_expr_object_size (struct object_siz
>    else
>      expr_object_size (osi, var, then_);
>  
> +  if (object_sizes[object_size_type][varno] == unknown[object_size_type])
> +    return reexamine;
> +
>    if (TREE_CODE (else_) == SSA_NAME)
>      reexamine |= merge_object_sizes (osi, var, else_, 0);
>    else
> --- gcc/testsuite/gcc.c-torture/compile/pr92056.c.jj	2019-10-16 15:42:56.042848440 +0200
> +++ gcc/testsuite/gcc.c-torture/compile/pr92056.c	2019-10-16 15:42:41.595066602 +0200
> @@ -0,0 +1,18 @@
> +/* PR tree-optimization/92056 */
> +
> +const char *d;
> +
> +void
> +foo (int c, char *e, const char *a, const char *b)
> +{
> +  switch (c)
> +    {
> +    case 33:
> +      for (;; d++)
> +        if (__builtin_strcmp (b ? : "", d))
> +          return;
> +      break;
> +    case 4:
> +      __builtin_sprintf (e, a);
> +    }
> +}
> 
> 	Jakub
>
Martin Sebor Oct. 18, 2019, 12:07 a.m. UTC | #2
On 10/17/19 1:00 AM, Jakub Jelinek wrote:
> Hi!
> 
> The following bug has been introduced when cond_expr_object_size has been
> added in 2007.  We want to treat a COND_EXPR like a PHI with 2 arguments,
> and PHI is handled in a loop that breaks if the lhs value is unknown, and
> then does the if (TREE_CODE (arg) == SSA_NAME) merge_object_sizes else
> expr_object_size which is used even in places that handle just a single
> operand (with the lhs value initialized to the opposite value of unknown
> first).  At least expr_object_size asserts that the lhs value is not
> unknown at the start.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?

I'm not sure the other change (r277134) it the right way to fix
the problem with the missing initialization.  It was introduced
with the merger of the sprintf pass.  The latter still calls
init_object_sizes in get_destination_size.  I think the call
should be moved from there into the new combined sprintf/strlen
printf_strlen_execute function that also calls fini_object_sizes,
and the one from determine_min_objsize should be removed.  I can
take care of it unless you think it needs to stay the way it is
now for some reason.

Martin

> 
> 2019-10-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/92056
> 	* tree-object-size.c (cond_expr_object_size): Return early if then_
> 	processing resulted in unknown size.
> 
> 	* gcc.c-torture/compile/pr92056.c: New test.
> 
> --- gcc/tree-object-size.c.jj	2019-10-05 09:35:14.895967464 +0200
> +++ gcc/tree-object-size.c	2019-10-16 15:34:11.414769994 +0200
> @@ -903,6 +903,9 @@ cond_expr_object_size (struct object_siz
>     else
>       expr_object_size (osi, var, then_);
>   
> +  if (object_sizes[object_size_type][varno] == unknown[object_size_type])
> +    return reexamine;
> +
>     if (TREE_CODE (else_) == SSA_NAME)
>       reexamine |= merge_object_sizes (osi, var, else_, 0);
>     else
> --- gcc/testsuite/gcc.c-torture/compile/pr92056.c.jj	2019-10-16 15:42:56.042848440 +0200
> +++ gcc/testsuite/gcc.c-torture/compile/pr92056.c	2019-10-16 15:42:41.595066602 +0200
> @@ -0,0 +1,18 @@
> +/* PR tree-optimization/92056 */
> +
> +const char *d;
> +
> +void
> +foo (int c, char *e, const char *a, const char *b)
> +{
> +  switch (c)
> +    {
> +    case 33:
> +      for (;; d++)
> +        if (__builtin_strcmp (b ? : "", d))
> +          return;
> +      break;
> +    case 4:
> +      __builtin_sprintf (e, a);
> +    }
> +}
> 
> 	Jakub
>
Jakub Jelinek Oct. 18, 2019, 6:52 a.m. UTC | #3
On Thu, Oct 17, 2019 at 06:07:37PM -0600, Martin Sebor wrote:
> On 10/17/19 1:00 AM, Jakub Jelinek wrote:
> > Hi!
> > 
> > The following bug has been introduced when cond_expr_object_size has been
> > added in 2007.  We want to treat a COND_EXPR like a PHI with 2 arguments,
> > and PHI is handled in a loop that breaks if the lhs value is unknown, and
> > then does the if (TREE_CODE (arg) == SSA_NAME) merge_object_sizes else
> > expr_object_size which is used even in places that handle just a single
> > operand (with the lhs value initialized to the opposite value of unknown
> > first).  At least expr_object_size asserts that the lhs value is not
> > unknown at the start.
> > 
> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > trunk?
> 
> I'm not sure the other change (r277134) it the right way to fix
> the problem with the missing initialization.  It was introduced
> with the merger of the sprintf pass.  The latter still calls
> init_object_sizes in get_destination_size.  I think the call
> should be moved from there into the new combined sprintf/strlen
> printf_strlen_execute function that also calls fini_object_sizes,
> and the one from determine_min_objsize should be removed.  I can
> take care of it unless you think it needs to stay the way it is
> now for some reason.

Why?  As I said, init_object_sizes is designed to be called multiple times
and is cheap (load + comparison + early return) if it has been already
called, so is meant to be called only when needed, rather than at the
beginning of a pass just in case something appears.  The objsz pass does the
same.  No need to allocate bitmaps/vectors if nothing will need them.

	Jakub
Martin Sebor Oct. 18, 2019, 3:01 p.m. UTC | #4
On 10/18/19 12:52 AM, Jakub Jelinek wrote:
> On Thu, Oct 17, 2019 at 06:07:37PM -0600, Martin Sebor wrote:
>> On 10/17/19 1:00 AM, Jakub Jelinek wrote:
>>> Hi!
>>>
>>> The following bug has been introduced when cond_expr_object_size has been
>>> added in 2007.  We want to treat a COND_EXPR like a PHI with 2 arguments,
>>> and PHI is handled in a loop that breaks if the lhs value is unknown, and
>>> then does the if (TREE_CODE (arg) == SSA_NAME) merge_object_sizes else
>>> expr_object_size which is used even in places that handle just a single
>>> operand (with the lhs value initialized to the opposite value of unknown
>>> first).  At least expr_object_size asserts that the lhs value is not
>>> unknown at the start.
>>>
>>> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>>> trunk?
>>
>> I'm not sure the other change (r277134) it the right way to fix
>> the problem with the missing initialization.  It was introduced
>> with the merger of the sprintf pass.  The latter still calls
>> init_object_sizes in get_destination_size.  I think the call
>> should be moved from there into the new combined sprintf/strlen
>> printf_strlen_execute function that also calls fini_object_sizes,
>> and the one from determine_min_objsize should be removed.  I can
>> take care of it unless you think it needs to stay the way it is
>> now for some reason.
> 
> Why?  As I said, init_object_sizes is designed to be called multiple times
> and is cheap (load + comparison + early return) if it has been already
> called, so is meant to be called only when needed, rather than at the
> beginning of a pass just in case something appears.  The objsz pass does the
> same.  No need to allocate bitmaps/vectors if nothing will need them.

Why?  Because as the bug you just fixed illustrates it's obviously
error prone to initialize the pass on demand in a utility function.
There are two now, and the next time someone adds a new one they
could easily reintroduce the same problem.

Martin

Patch
diff mbox series

--- gcc/tree-object-size.c.jj	2019-10-05 09:35:14.895967464 +0200
+++ gcc/tree-object-size.c	2019-10-16 15:34:11.414769994 +0200
@@ -903,6 +903,9 @@  cond_expr_object_size (struct object_siz
   else
     expr_object_size (osi, var, then_);
 
+  if (object_sizes[object_size_type][varno] == unknown[object_size_type])
+    return reexamine;
+
   if (TREE_CODE (else_) == SSA_NAME)
     reexamine |= merge_object_sizes (osi, var, else_, 0);
   else
--- gcc/testsuite/gcc.c-torture/compile/pr92056.c.jj	2019-10-16 15:42:56.042848440 +0200
+++ gcc/testsuite/gcc.c-torture/compile/pr92056.c	2019-10-16 15:42:41.595066602 +0200
@@ -0,0 +1,18 @@ 
+/* PR tree-optimization/92056 */
+
+const char *d;
+
+void
+foo (int c, char *e, const char *a, const char *b)
+{
+  switch (c)
+    {
+    case 33:
+      for (;; d++)
+        if (__builtin_strcmp (b ? : "", d))
+          return;
+      break;
+    case 4:
+      __builtin_sprintf (e, a);
+    }
+}