[PATCH] ipa: Fix segfault when remapping debug_binds with expressions (PR 103132)

Richard Biener rguenther@suse.de
Tue Nov 9 07:30:39 GMT 2021


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); }
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)


More information about the Gcc-patches mailing list