| Message ID | 076d995d-533a-8648-e9c8-5527907329da@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFA,PR,tree-optimization/90949] Don't propagate context sensitive non-nullness when copy-propagating pointers | expand |
On 6/20/19 4:23 PM, Jeff Law wrote: > As outlined in the BZ, our alias analysis code is context insensitive. > So when we copy-propagate pointers, we can can and do copy PTA > information from members to the representative pointer in the copy-of > chain (we do this when the representative pointer has no associated PTA > information). > > However, [E]VRP can set the non-nullness of a pointer using context > sensitive information. So we have to be more careful when copying PTA > information. > > We already have similar issues with alignment information as well. This > patch just extends the hack to avoid copying alignment information in > some circumstances to also avoid copying the non-nullness property. > > Bootstrapped and regression tested on x86_64-linux-gnu. OK for the trunk? > > Jeff > Just a question/comment about the test: --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr90949.c @@ -0,0 +1,33 @@ +void __attribute__((noipa,noinline)) my_puts (const char *str) { } +void __attribute__((noipa,noinline)) my_free (void *p) { } + + +struct Node +{ + struct Node* child; +}; + +char *space[sizeof (struct Node) * 2] = { }; + +void * __attribute__((noipa,noinline)) my_malloc (int bytes) { return &space;} Shouldn't space be declared as an array of char rather than char*? (As it is, strictly speaking, accessing it via an lvalue of type Node is undefined, even if it's carefully hidden from the compiler by the attributes.) Martin
On Fri, Jun 21, 2019 at 12:24 AM Jeff Law <law@redhat.com> wrote: > > As outlined in the BZ, our alias analysis code is context insensitive. > So when we copy-propagate pointers, we can can and do copy PTA > information from members to the representative pointer in the copy-of > chain (we do this when the representative pointer has no associated PTA > information). > > However, [E]VRP can set the non-nullness of a pointer using context > sensitive information. So we have to be more careful when copying PTA > information. > > We already have similar issues with alignment information as well. This > patch just extends the hack to avoid copying alignment information in > some circumstances to also avoid copying the non-nullness property. > > Bootstrapped and regression tested on x86_64-linux-gnu. OK for the trunk? I think this should be done in reset_flow_sensitive_info () as well and tree-ssa-copy.c should use that function instead of calling mark_ptr_info_alignment_unknown. I'd probably avoid the new clear_ptr_nonnull function since it might make people miss the other half (alignment). Otherwise looks OK. Richard. > Jeff
On 6/20/19 8:09 PM, Martin Sebor wrote: > On 6/20/19 4:23 PM, Jeff Law wrote: >> As outlined in the BZ, our alias analysis code is context insensitive. >> So when we copy-propagate pointers, we can can and do copy PTA >> information from members to the representative pointer in the copy-of >> chain (we do this when the representative pointer has no associated PTA >> information). >> >> However, [E]VRP can set the non-nullness of a pointer using context >> sensitive information. So we have to be more careful when copying PTA >> information. >> >> We already have similar issues with alignment information as well. This >> patch just extends the hack to avoid copying alignment information in >> some circumstances to also avoid copying the non-nullness property. >> >> Bootstrapped and regression tested on x86_64-linux-gnu. OK for the >> trunk? >> >> Jeff >> > > Just a question/comment about the test: > > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/pr90949.c > @@ -0,0 +1,33 @@ > +void __attribute__((noipa,noinline)) my_puts (const char *str) { } > +void __attribute__((noipa,noinline)) my_free (void *p) { } > + > + > +struct Node > +{ > + struct Node* child; > +}; > + > +char *space[sizeof (struct Node) * 2] = { }; > + > +void * __attribute__((noipa,noinline)) my_malloc (int bytes) { return > &space;} > > Shouldn't space be declared as an array of char rather than char*? Yup. I'll fix it. > (As it is, strictly speaking, accessing it via an lvalue of type > Node is undefined, even if it's carefully hidden from the compiler > by the attributes.) Doesn't really matter in the end. In fact there's really not a reason why this has to return char * or void * since it's really not malloc. We could have struct Nodes. I'll fix that too. jeff > > Martin
On 6/21/19 6:28 AM, Richard Biener wrote: > On Fri, Jun 21, 2019 at 12:24 AM Jeff Law <law@redhat.com> wrote: >> >> As outlined in the BZ, our alias analysis code is context insensitive. >> So when we copy-propagate pointers, we can can and do copy PTA >> information from members to the representative pointer in the copy-of >> chain (we do this when the representative pointer has no associated PTA >> information). >> >> However, [E]VRP can set the non-nullness of a pointer using context >> sensitive information. So we have to be more careful when copying PTA >> information. >> >> We already have similar issues with alignment information as well. This >> patch just extends the hack to avoid copying alignment information in >> some circumstances to also avoid copying the non-nullness property. >> >> Bootstrapped and regression tested on x86_64-linux-gnu. OK for the trunk? > > I think this should be done in reset_flow_sensitive_info () as well > and tree-ssa-copy.c should use that function instead of calling > mark_ptr_info_alignment_unknown. OK. Agreed that's better -- one centralized routine to reset this stuff if we find more over time. > > I'd probably avoid the new clear_ptr_nonnull function since it might make > people miss the other half (alignment). OK. Presumably you just want to twiddle the field directly? THat's easy 'nuff. Jeff
On June 21, 2019 4:43:10 PM GMT+02:00, Jeff Law <law@redhat.com> wrote: >On 6/21/19 6:28 AM, Richard Biener wrote: >> On Fri, Jun 21, 2019 at 12:24 AM Jeff Law <law@redhat.com> wrote: >>> >>> As outlined in the BZ, our alias analysis code is context >insensitive. >>> So when we copy-propagate pointers, we can can and do copy PTA >>> information from members to the representative pointer in the >copy-of >>> chain (we do this when the representative pointer has no associated >PTA >>> information). >>> >>> However, [E]VRP can set the non-nullness of a pointer using context >>> sensitive information. So we have to be more careful when copying >PTA >>> information. >>> >>> We already have similar issues with alignment information as well. >This >>> patch just extends the hack to avoid copying alignment information >in >>> some circumstances to also avoid copying the non-nullness property. >>> >>> Bootstrapped and regression tested on x86_64-linux-gnu. OK for the >trunk? >> >> I think this should be done in reset_flow_sensitive_info () as well >> and tree-ssa-copy.c should use that function instead of calling >> mark_ptr_info_alignment_unknown. >OK. Agreed that's better -- one centralized routine to reset this >stuff >if we find more over time. > >> >> I'd probably avoid the new clear_ptr_nonnull function since it might >make >> people miss the other half (alignment). >OK. Presumably you just want to twiddle the field directly? THat's >easy 'nuff. Yes indeed. Richard. > >Jeff
On 6/21/19 6:28 AM, Richard Biener wrote: > On Fri, Jun 21, 2019 at 12:24 AM Jeff Law <law@redhat.com> wrote: >> >> As outlined in the BZ, our alias analysis code is context insensitive. >> So when we copy-propagate pointers, we can can and do copy PTA >> information from members to the representative pointer in the copy-of >> chain (we do this when the representative pointer has no associated PTA >> information). >> >> However, [E]VRP can set the non-nullness of a pointer using context >> sensitive information. So we have to be more careful when copying PTA >> information. >> >> We already have similar issues with alignment information as well. This >> patch just extends the hack to avoid copying alignment information in >> some circumstances to also avoid copying the non-nullness property. >> >> Bootstrapped and regression tested on x86_64-linux-gnu. OK for the trunk? > > I think this should be done in reset_flow_sensitive_info () as well > and tree-ssa-copy.c should use that function instead of calling > mark_ptr_info_alignment_unknown. > > I'd probably avoid the new clear_ptr_nonnull function since it might make > people miss the other half (alignment). > > Otherwise looks OK. Given that reset_flow_sensitive_info is used in a variety of other places, the requested changes seemed well past the line that required another bootstrap & regression test, which went fine. Attached is what I actually installed. I'll backport to gcc-9 after a few days. I'll need to look at a gcc-8 to see if it's potentially needed there as well. jeff * gcc.c-torture/execute/pr90949.c: New test. * tree-ssa-copy.c (fini_copy_prop): Use reset_flow_sensitive_info. * tree-ssanames.c (reset_flow_sensitive_info): Reset non-null state. diff --git a/gcc/testsuite/gcc.c-torture/execute/pr90949.c b/gcc/testsuite/gcc.c-torture/execute/pr90949.c new file mode 100644 index 00000000000..8c2ae3972d4 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr90949.c @@ -0,0 +1,42 @@ +void __attribute__ ((noipa, noinline)) my_puts (const char *str) { } + +void __attribute__ ((noipa, noinline)) my_free (void *p) { } + + +struct Node +{ + struct Node *child; +}; + +struct Node space[2] = { }; + +struct Node * __attribute__ ((noipa, noinline)) my_malloc (int bytes) +{ + return &space[0]; +} + +void +walk (struct Node *module, int cleanup) +{ + if (module == 0) + { + return; + } + if (!cleanup) + { + my_puts ("No cleanup"); + } + walk (module->child, cleanup); + if (cleanup) + { + my_free (module); + } +} + +int +main () +{ + struct Node *node = my_malloc (sizeof (struct Node)); + node->child = 0; + walk (node, 1); +} diff --git a/gcc/tree-ssa-copy.c b/gcc/tree-ssa-copy.c index 89532633e42..28ff8d3fbe2 100644 --- a/gcc/tree-ssa-copy.c +++ b/gcc/tree-ssa-copy.c @@ -545,13 +545,12 @@ fini_copy_prop (void) duplicate_ssa_name_ptr_info (copy_of[i].value, SSA_NAME_PTR_INFO (var)); /* Points-to information is cfg insensitive, - but alignment info might be cfg sensitive, if it - e.g. is derived from VRP derived non-zero bits. - So, do not copy alignment info if the two SSA_NAMEs - aren't defined in the same basic block. */ + but [E]VRP might record context sensitive alignment + info, non-nullness, etc. So reset context sensitive + info if the two SSA_NAMEs aren't defined in the same + basic block. */ if (var_bb != copy_of_bb) - mark_ptr_info_alignment_unknown - (SSA_NAME_PTR_INFO (copy_of[i].value)); + reset_flow_sensitive_info (copy_of[i].value); } else if (!POINTER_TYPE_P (TREE_TYPE (var)) && SSA_NAME_RANGE_INFO (var) diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c index 5bac799e9a3..8b80bce8945 100644 --- a/gcc/tree-ssanames.c +++ b/gcc/tree-ssanames.c @@ -820,7 +820,12 @@ reset_flow_sensitive_info (tree name) { /* points-to info is not flow-sensitive. */ if (SSA_NAME_PTR_INFO (name)) - mark_ptr_info_alignment_unknown (SSA_NAME_PTR_INFO (name)); + { + /* [E]VRP can derive context sensitive alignment info and + non-nullness properties. We must reset both. */ + mark_ptr_info_alignment_unknown (SSA_NAME_PTR_INFO (name)); + SSA_NAME_PTR_INFO (name)->pt.null = 1; + } } else SSA_NAME_RANGE_INFO (name) = NULL;
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr90949.c b/gcc/testsuite/gcc.c-torture/execute/pr90949.c new file mode 100644 index 00000000000..12ae31d97a5 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr90949.c @@ -0,0 +1,33 @@ +void __attribute__((noipa,noinline)) my_puts (const char *str) { } +void __attribute__((noipa,noinline)) my_free (void *p) { } + + +struct Node +{ + struct Node* child; +}; + +char *space[sizeof (struct Node) * 2] = { }; + +void * __attribute__((noipa,noinline)) my_malloc (int bytes) { return &space;} + +void walk(struct Node* module, int cleanup) +{ + if (module == 0) { + return; + } + if (!cleanup) { + my_puts("No cleanup"); + } + walk(module->child, cleanup); + if (cleanup) { + my_free(module); + } +} + +int main() +{ + struct Node* node = my_malloc(sizeof(struct Node)); + node->child = 0; + walk(node, 1); +} diff --git a/gcc/tree-ssa-copy.c b/gcc/tree-ssa-copy.c index 89532633e42..ccb95bf18b7 100644 --- a/gcc/tree-ssa-copy.c +++ b/gcc/tree-ssa-copy.c @@ -548,10 +548,28 @@ fini_copy_prop (void) but alignment info might be cfg sensitive, if it e.g. is derived from VRP derived non-zero bits. So, do not copy alignment info if the two SSA_NAMEs - aren't defined in the same basic block. */ + aren't defined in the same basic block. + + Similarly, we may have a context sensitive non-NULL + state for an SSA_NAME (call it A), which in turn is + used to derive a global non-NULL state for a different + SSA_NAME (call it B) via a PHI node. + + That PHI node also represents a copy which we will try + to eliminate here. We will copy the alias info to the + representative element in the copy-of chains. If A is + the representative element, then we just made A globally + non-NULL which is incorrect. + + Arguably one might claim this is too fragile and that we + should never dupicate the points-to information if the + objects are in different blocks. */ if (var_bb != copy_of_bb) - mark_ptr_info_alignment_unknown - (SSA_NAME_PTR_INFO (copy_of[i].value)); + { + mark_ptr_info_alignment_unknown + (SSA_NAME_PTR_INFO (copy_of[i].value)); + clear_ptr_nonnull (copy_of[i].value); + } } else if (!POINTER_TYPE_P (TREE_TYPE (var)) && SSA_NAME_RANGE_INFO (var) diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c index 5bac799e9a3..2114a9584ba 100644 --- a/gcc/tree-ssanames.c +++ b/gcc/tree-ssanames.c @@ -451,6 +451,16 @@ get_range_info (const_tree name, value_range_base &vr) return kind; } +/* Clear nonnull attribute to pointer NAME. */ + +void +clear_ptr_nonnull (tree name) +{ + gcc_assert (POINTER_TYPE_P (TREE_TYPE (name))); + struct ptr_info_def *pi = get_ptr_info (name); + pi->pt.null = 1; +} + /* Set nonnull attribute to pointer NAME. */ void diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h index 6e6cffbce6a..6470d491deb 100644 --- a/gcc/tree-ssanames.h +++ b/gcc/tree-ssanames.h @@ -91,6 +91,7 @@ extern void set_ptr_info_alignment (struct ptr_info_def *, unsigned int, extern void adjust_ptr_info_misalignment (struct ptr_info_def *, poly_uint64); extern struct ptr_info_def *get_ptr_info (tree); extern void set_ptr_nonnull (tree); +extern void clear_ptr_nonnull (tree); extern bool get_ptr_nonnull (const_tree); extern tree copy_ssa_name_fn (struct function *, tree, gimple *);