| Message ID | 20191017070024.GV2116@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Fix objsz ICE (PR tree-optimization/92056) | expand |
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 >
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 >
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
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
--- 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); + } +}