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 <mjam...@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 <mjam...@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); }
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
