| Message ID | ri6y25ya8mb.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | ipa: Fix segfault when remapping debug_binds with expressions (PR 103132) | expand |
On Mon, 8 Nov 2021, Martin Jambor wrote: > Hi, > > my initial implementation of the method > ipa_param_body_adjustments::remap_with_debug_expressions was based on > the assumption that if it was asked to remap an expression (as opposed > to a simple SSA_NAME), the expression would not contain an SSA_NAME > operand which is to be debug-reset. While that is true for when > called from ipa_param_body_adjustments::prepare_debug_expressions, it > turns out it is not true when invoked from remap_gimple_stmt in > tree-inline.c. This patch adds a simple logic to handle such cases > and simply map the entire value to NULL_TREE in those cases. > > I have bootstrapped and tested the patch on x86_64-linux. OK for trunk? OK. Thanks, Richard. > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2021-11-08 Martin Jambor <mjambor@suse.cz> > > PR ipa/103132 > * ipa-param-manipulation.c (replace_with_mapped_expr): Early > return with error_mark_mode when part of expression is mapped to > NULL. > (ipa_param_body_adjustments::remap_with_debug_expressions): Set > mapped value to NULL if walk_tree returns error_mark_mode. > > gcc/testsuite/ChangeLog: > > 2021-11-08 Martin Jambor <mjambor@suse.cz> > > PR ipa/103132 > * gcc.dg/ipa/pr103132.c: New test. > --- > gcc/ipa-param-manipulation.c | 27 +++++++++++++++++++-------- > gcc/testsuite/gcc.dg/ipa/pr103132.c | 19 +++++++++++++++++++ > 2 files changed, 38 insertions(+), 8 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103132.c > > diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c > index 44f3bed2640..4610fc4ac03 100644 > --- a/gcc/ipa-param-manipulation.c > +++ b/gcc/ipa-param-manipulation.c > @@ -1071,8 +1071,9 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param, > } > > /* Callback to walk_tree. If REMAP is an SSA_NAME that is present in hash_map > - passed in DATA, replace it with unshared version of what it was mapped > - to. */ > + passed in DATA, replace it with unshared version of what it was mapped to. > + If an SSA argument would be remapped to NULL, the whole operation needs to > + abort which is signaled by returning error_mark_node. */ > > static tree > replace_with_mapped_expr (tree *remap, int *walk_subtrees, void *data) > @@ -1089,7 +1090,11 @@ replace_with_mapped_expr (tree *remap, int *walk_subtrees, void *data) > > hash_map<tree, tree> *equivs = (hash_map<tree, tree> *) data; > if (tree *p = equivs->get (*remap)) > - *remap = unshare_expr (*p); > + { > + if (!*p) > + return error_mark_node; > + *remap = unshare_expr (*p); > + } > return 0; > } > > @@ -1100,16 +1105,22 @@ void > ipa_param_body_adjustments::remap_with_debug_expressions (tree *t) > { > /* If *t is an SSA_NAME which should have its debug statements reset, it is > - mapped to NULL in the hash_map. We need to handle that case separately or > - otherwise the walker would segfault. No expression that is more > - complicated than that can have its operands mapped to NULL. */ > + mapped to NULL in the hash_map. > + > + It is perhaps simpler to handle the SSA_NAME cases directly and only > + invoke walk_tree on more complex expressions. When > + remap_with_debug_expressions is called from tree-inline.c, a to-be-reset > + SSA_NAME can be an operand to such expressions and the entire debug > + variable we are remapping should be reset. This is signaled by walk_tree > + returning error_mark_node and done by setting *t to NULL. */ > if (TREE_CODE (*t) == SSA_NAME) > { > if (tree *p = m_dead_ssa_debug_equiv.get (*t)) > *t = *p; > } > - else > - walk_tree (t, replace_with_mapped_expr, &m_dead_ssa_debug_equiv, NULL); > + else if (walk_tree (t, replace_with_mapped_expr, > + &m_dead_ssa_debug_equiv, NULL) == error_mark_node) > + *t = NULL_TREE; > } > > /* For an SSA_NAME DEAD_SSA which is about to be DCEd because it is based on a > diff --git a/gcc/testsuite/gcc.dg/ipa/pr103132.c b/gcc/testsuite/gcc.dg/ipa/pr103132.c > new file mode 100644 > index 00000000000..bef56494c03 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr103132.c > @@ -0,0 +1,19 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -g" } */ > + > +int globus_i_GLOBUS_GRIDFTP_SERVER_debug_handle_1; > +int globus_l_gfs_ipc_unpack_data__sz; > +void globus_i_GLOBUS_GRIDFTP_SERVER_debug_printf(const char *, ...); > +static void globus_l_gfs_ipc_unpack_cred(int len) { > + if (globus_i_GLOBUS_GRIDFTP_SERVER_debug_handle_1) > + globus_i_GLOBUS_GRIDFTP_SERVER_debug_printf("", __func__); > +} > +static void globus_l_gfs_ipc_unpack_data(int len) { > + for (; globus_l_gfs_ipc_unpack_data__sz;) > + len--; > + len -= 4; > + len -= 4; > + globus_l_gfs_ipc_unpack_cred(len); > +} > +void globus_l_gfs_ipc_reply_read_body_cb(int len) > +{ globus_l_gfs_ipc_unpack_data(len); } >
diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c index 44f3bed2640..4610fc4ac03 100644 --- a/gcc/ipa-param-manipulation.c +++ b/gcc/ipa-param-manipulation.c @@ -1071,8 +1071,9 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param, } /* Callback to walk_tree. If REMAP is an SSA_NAME that is present in hash_map - passed in DATA, replace it with unshared version of what it was mapped - to. */ + passed in DATA, replace it with unshared version of what it was mapped to. + If an SSA argument would be remapped to NULL, the whole operation needs to + abort which is signaled by returning error_mark_node. */ static tree replace_with_mapped_expr (tree *remap, int *walk_subtrees, void *data) @@ -1089,7 +1090,11 @@ replace_with_mapped_expr (tree *remap, int *walk_subtrees, void *data) hash_map<tree, tree> *equivs = (hash_map<tree, tree> *) data; if (tree *p = equivs->get (*remap)) - *remap = unshare_expr (*p); + { + if (!*p) + return error_mark_node; + *remap = unshare_expr (*p); + } return 0; } @@ -1100,16 +1105,22 @@ void ipa_param_body_adjustments::remap_with_debug_expressions (tree *t) { /* If *t is an SSA_NAME which should have its debug statements reset, it is - mapped to NULL in the hash_map. We need to handle that case separately or - otherwise the walker would segfault. No expression that is more - complicated than that can have its operands mapped to NULL. */ + mapped to NULL in the hash_map. + + It is perhaps simpler to handle the SSA_NAME cases directly and only + invoke walk_tree on more complex expressions. When + remap_with_debug_expressions is called from tree-inline.c, a to-be-reset + SSA_NAME can be an operand to such expressions and the entire debug + variable we are remapping should be reset. This is signaled by walk_tree + returning error_mark_node and done by setting *t to NULL. */ if (TREE_CODE (*t) == SSA_NAME) { if (tree *p = m_dead_ssa_debug_equiv.get (*t)) *t = *p; } - else - walk_tree (t, replace_with_mapped_expr, &m_dead_ssa_debug_equiv, NULL); + else if (walk_tree (t, replace_with_mapped_expr, + &m_dead_ssa_debug_equiv, NULL) == error_mark_node) + *t = NULL_TREE; } /* For an SSA_NAME DEAD_SSA which is about to be DCEd because it is based on a diff --git a/gcc/testsuite/gcc.dg/ipa/pr103132.c b/gcc/testsuite/gcc.dg/ipa/pr103132.c new file mode 100644 index 00000000000..bef56494c03 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr103132.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -g" } */ + +int globus_i_GLOBUS_GRIDFTP_SERVER_debug_handle_1; +int globus_l_gfs_ipc_unpack_data__sz; +void globus_i_GLOBUS_GRIDFTP_SERVER_debug_printf(const char *, ...); +static void globus_l_gfs_ipc_unpack_cred(int len) { + if (globus_i_GLOBUS_GRIDFTP_SERVER_debug_handle_1) + globus_i_GLOBUS_GRIDFTP_SERVER_debug_printf("", __func__); +} +static void globus_l_gfs_ipc_unpack_data(int len) { + for (; globus_l_gfs_ipc_unpack_data__sz;) + len--; + len -= 4; + len -= 4; + globus_l_gfs_ipc_unpack_cred(len); +} +void globus_l_gfs_ipc_reply_read_body_cb(int len) +{ globus_l_gfs_ipc_unpack_data(len); }