| Message ID | 7331c16f-3725-c528-8cd6-7f38bc19f0c2@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] Fix minor bug in recently added sanity test in tree-ssa-dse.c | expand |
On Mon, Aug 27, 2018 at 10:31 PM Jeff Law <law@redhat.com> wrote: > > > We recently changes tree-ssa-dse.c to not trim stores outside the bounds > of the referenced object. This is generally a good thing. > > However, there are cases where the object doesn't have a usable size. > We see this during kernel builds, at least on the microblaze target. > > We've got... > > _1 = p_47(D)->stack; > childregs_48 = &MEM[(void *)_1 + 8040B]; > [ ... ] > memset (childregs_48, 0, 152); > [ ... ] > MEM[(struct pt_regs *)_1 + 8040B].pt_mode = 1; > > > We want to trim the memset call as the assignment to pt_mode is the last > word written by the memset call, thus making the store of that word via > memset dead. > > Our ref->base is: > > (gdb) p debug_tree ($66) > <mem_ref 0x7fffe8b946e0 > type <void_type 0x7fffe9e210a8 void VOID > align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type > 0x7fffe9e210a8 > pointer_to_this <pointer_type 0x7fffe9e21150>> > > arg:0 <ssa_name 0x7fffe8f8a2d0 > type <pointer_type 0x7fffe9e21150 type <void_type 0x7fffe9e210a8 > void> > public unsigned SI > size <integer_cst 0x7fffe9e0d678 constant 32> > unit-size <integer_cst 0x7fffe9e0d690 constant 4> > align:32 warn_if_not_align:0 symtab:0 alias-set 8 > canonical-type 0x7fffe9e21150 context <translation_unit_decl > 0x7fffe8f72438 /tmp/process.i> > pointer_to_this <pointer_type 0x7fffe9e28bd0> > reference_to_this <reference_type 0x7fffe9e282a0>> > visited > def_stmt _1 = p_47(D)->stack; > version:1 > ptr-info 0x7fffe8f65fa8> > arg:1 <integer_cst 0x7fffe8f65f60 type <pointer_type 0x7fffe9e21150> > constant 8040>> > > > Note the void type. Those do not have a suitable TYPE_SIZE_UNIT, thus > causing an ICE when we try to reference it within compute_trims: > /* But don't trim away out of bounds accesses, as this defeats > proper warnings. */ > if (*trim_tail > && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)), > last_orig) <= 0) > > > The fix is obvious enough. Don't do the check when the underlying type > has no usable TYPE_SIZE_UNIT. > > I pondered moving the tests into the code that determines whether or not > we do byte tracking DSE, but decided the current location was fine. > > Bootstrapped and regression tested on x86. Also verified the original > testfile will build with a microblaze cross compiler. > > Installing on the trunk momentarily. Hmm, you seem to get ref->base from an address but you know types on addresses do not have any meaning, so ... how does /* But don't trim away out of bounds accesses, as this defeats proper warnings. We could have a type with no TYPE_SIZE_UNIT or we could have a VLA where TYPE_SIZE_UNIT is not a constant. */ if (*trim_tail && TYPE_SIZE_UNIT (TREE_TYPE (ref->base)) && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (ref->base))) == INTEGER_CST && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)), last_orig) <= 0) *trim_tail = 0; make any sense in the above case where ref->base is even based on a pointer? I'd say the above should be at least if (*trim_tail && DECL_P (ref->base) && .... ? Not sure if we ever have decls with incomplete types so eventually the new check isn't needed. Richard. > jeff
On 8/28/18 3:37 AM, Richard Biener wrote: > On Mon, Aug 27, 2018 at 10:31 PM Jeff Law <law@redhat.com> wrote: >> >> >> We recently changes tree-ssa-dse.c to not trim stores outside the bounds >> of the referenced object. This is generally a good thing. >> >> However, there are cases where the object doesn't have a usable size. >> We see this during kernel builds, at least on the microblaze target. >> >> We've got... >> >> _1 = p_47(D)->stack; >> childregs_48 = &MEM[(void *)_1 + 8040B]; >> [ ... ] >> memset (childregs_48, 0, 152); >> [ ... ] >> MEM[(struct pt_regs *)_1 + 8040B].pt_mode = 1; >> >> >> We want to trim the memset call as the assignment to pt_mode is the last >> word written by the memset call, thus making the store of that word via >> memset dead. >> >> Our ref->base is: >> >> (gdb) p debug_tree ($66) >> <mem_ref 0x7fffe8b946e0 >> type <void_type 0x7fffe9e210a8 void VOID >> align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type >> 0x7fffe9e210a8 >> pointer_to_this <pointer_type 0x7fffe9e21150>> >> >> arg:0 <ssa_name 0x7fffe8f8a2d0 >> type <pointer_type 0x7fffe9e21150 type <void_type 0x7fffe9e210a8 >> void> >> public unsigned SI >> size <integer_cst 0x7fffe9e0d678 constant 32> >> unit-size <integer_cst 0x7fffe9e0d690 constant 4> >> align:32 warn_if_not_align:0 symtab:0 alias-set 8 >> canonical-type 0x7fffe9e21150 context <translation_unit_decl >> 0x7fffe8f72438 /tmp/process.i> >> pointer_to_this <pointer_type 0x7fffe9e28bd0> >> reference_to_this <reference_type 0x7fffe9e282a0>> >> visited >> def_stmt _1 = p_47(D)->stack; >> version:1 >> ptr-info 0x7fffe8f65fa8> >> arg:1 <integer_cst 0x7fffe8f65f60 type <pointer_type 0x7fffe9e21150> >> constant 8040>> >> >> >> Note the void type. Those do not have a suitable TYPE_SIZE_UNIT, thus >> causing an ICE when we try to reference it within compute_trims: >> /* But don't trim away out of bounds accesses, as this defeats >> proper warnings. */ >> if (*trim_tail >> && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)), >> last_orig) <= 0) >> >> >> The fix is obvious enough. Don't do the check when the underlying type >> has no usable TYPE_SIZE_UNIT. >> >> I pondered moving the tests into the code that determines whether or not >> we do byte tracking DSE, but decided the current location was fine. >> >> Bootstrapped and regression tested on x86. Also verified the original >> testfile will build with a microblaze cross compiler. >> >> Installing on the trunk momentarily. > > Hmm, you seem to get ref->base from an address but you know types > on addresses do not have any meaning, so ... how does It doesn't affect correctness. Essentially when we have information which indicates there might be an out of bounds write, we leave the statement as-is. So if the type specified a smaller size than reality, then we potentially might suppress an optimization. If the type specified a large size than reality, then nothing changes. > > /* But don't trim away out of bounds accesses, as this defeats > proper warnings. > > We could have a type with no TYPE_SIZE_UNIT or we could have a VLA > where TYPE_SIZE_UNIT is not a constant. */ > if (*trim_tail > && TYPE_SIZE_UNIT (TREE_TYPE (ref->base)) > && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (ref->base))) == INTEGER_CST > && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)), > last_orig) <= 0) > *trim_tail = 0; > > make any sense in the above case where ref->base is even based on a > pointer? I'd say the > above should be at least > > if (*trim_tail > && DECL_P (ref->base) > && .... > > ? Not sure if we ever have decls with incomplete types so eventually > the new check > isn't needed. We could have something casted to a void type. Can't remember where it happened, but it certainly came up. jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index ac46b7422bc..a8ab83580b5 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,8 @@ +2018-08-27 Jeff Law <law@redhat.com> + + * tree-ssa-dse.c (compute_trims): Handle case where the reference's + type does not have a TYPE_SIZE_UNIT. + 2018-08-27 Steve Ellcey <sellcey@cavium.com> * config/aarch64/aarch64-speculation.cc: Replace include of cfg.h diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 824372c346a..6410f4638cc 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2018-08-27 Jeff Law <law@redhat.com> + + * gcc.c-torture/compile/dse.c: New test. + 2018-08-27 Jakub Jelinek <jakub@redhat.com> PR c++/86993 diff --git a/gcc/testsuite/gcc.c-torture/compile/dse.c b/gcc/testsuite/gcc.c-torture/compile/dse.c new file mode 100644 index 00000000000..908e6503eb4 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/dse.c @@ -0,0 +1,19 @@ +typedef unsigned long microblaze_reg_t; +struct pt_regs +{ + microblaze_reg_t msr; + int pt_mode; +}; +struct task_struct +{ + void *stack; +}; +int +copy_thread (struct task_struct *p) +{ + struct pt_regs *childregs = + (((struct pt_regs *) ((1 << 13) + ((void *) (p)->stack))) - 1); + memset (childregs, 0, sizeof (struct pt_regs)); + childregs->pt_mode = 1; +} + diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c index 016aa6cc97c..bddbbe8377a 100644 --- a/gcc/tree-ssa-dse.c +++ b/gcc/tree-ssa-dse.c @@ -252,6 +252,7 @@ compute_trims (ao_ref *ref, sbitmap live, int *trim_head, int *trim_tail, /* But don't trim away out of bounds accesses, as this defeats proper warnings. */ if (*trim_tail + && TYPE_SIZE_UNIT (TREE_TYPE (ref->base)) && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)), last_orig) <= 0) *trim_tail = 0;
We recently changes tree-ssa-dse.c to not trim stores outside the bounds of the referenced object. This is generally a good thing. However, there are cases where the object doesn't have a usable size. We see this during kernel builds, at least on the microblaze target. We've got... _1 = p_47(D)->stack; childregs_48 = &MEM[(void *)_1 + 8040B]; [ ... ] memset (childregs_48, 0, 152); [ ... ] MEM[(struct pt_regs *)_1 + 8040B].pt_mode = 1; We want to trim the memset call as the assignment to pt_mode is the last word written by the memset call, thus making the store of that word via memset dead. Our ref->base is: (gdb) p debug_tree ($66) <mem_ref 0x7fffe8b946e0 type <void_type 0x7fffe9e210a8 void VOID align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffe9e210a8 pointer_to_this <pointer_type 0x7fffe9e21150>> arg:0 <ssa_name 0x7fffe8f8a2d0 type <pointer_type 0x7fffe9e21150 type <void_type 0x7fffe9e210a8 void> public unsigned SI size <integer_cst 0x7fffe9e0d678 constant 32> unit-size <integer_cst 0x7fffe9e0d690 constant 4> align:32 warn_if_not_align:0 symtab:0 alias-set 8 canonical-type 0x7fffe9e21150 context <translation_unit_decl 0x7fffe8f72438 /tmp/process.i> pointer_to_this <pointer_type 0x7fffe9e28bd0> reference_to_this <reference_type 0x7fffe9e282a0>> visited def_stmt _1 = p_47(D)->stack; version:1 ptr-info 0x7fffe8f65fa8> arg:1 <integer_cst 0x7fffe8f65f60 type <pointer_type 0x7fffe9e21150> constant 8040>> Note the void type. Those do not have a suitable TYPE_SIZE_UNIT, thus causing an ICE when we try to reference it within compute_trims: /* But don't trim away out of bounds accesses, as this defeats proper warnings. */ if (*trim_tail && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)), last_orig) <= 0) The fix is obvious enough. Don't do the check when the underlying type has no usable TYPE_SIZE_UNIT. I pondered moving the tests into the code that determines whether or not we do byte tracking DSE, but decided the current location was fine. Bootstrapped and regression tested on x86. Also verified the original testfile will build with a microblaze cross compiler. Installing on the trunk momentarily. jeff