avoid early reference to debug-only symbol

Tue Jul 13 08:14:14 GMT 2021

On Tue, Jul 13, 2021 at 5:13 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
>
> If some IPA pass replaces the only reference to a constant non-public
> non-automatic variable with its initializer, namely the address of
> another such variable, the former becomes unreferenced and it's
> discarded by symbol_table::remove_unreachable_nodes.  It calls
> debug_hooks->late_global_decl while at that, and this expands the
> initializer, which assigs RTL to the latter variable and forces it to
> be retained by remove_unreferenced_decls, and eventually be output
> despite not being referenced.  Without debug information, it's not
> output.
>
> This has caused a bootstrap-debug compare failure in
> libdecnumber/decContext.o, while developing a transformation that ends
> up enabling the above substitution in constprop.
>
> This patch makes reference_to_unused slightly more conservative about
> such variables at the end of IPA passes, falling back onto
> expand_debug_expr for expressions referencing symbols that might or
> might not be output, avoiding the loss of debug information when the
> symbol is output, while avoiding a symbol output only because of debug
> info.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?
>
>
> for  gcc/ChangeLog
>
>         * dwarf2out.c (add_const_value_attribute): Return false if
>         resolve_one_addr fails.
>         (reference_to_unused): Don't assume local symbol presence
>         while it can still be optimized out.
>         (rtl_for_decl_init): Fallback to expand_debug_expr.
>         * cfgexpand.c (expand_debug_expr): Export.
>         * expr.h (expand_debug_expr): Declare.
> ---
>  gcc/cfgexpand.c |   12 +++++++++---
>  gcc/dwarf2out.c |   15 +++++++++++++--
>  gcc/expr.h      |    2 ++
>  3 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index 3edd53c37dcb3..b731a5598230c 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -91,8 +91,6 @@ struct ssaexpand SA;
>     of comminucating the profile info to the builtin expanders.  */
>  gimple *currently_expanding_gimple_stmt;
>
> -static rtx expand_debug_expr (tree);
> -
>  static bool defer_stack_allocation (tree, bool);
>
>  static void record_alignment_for_reg_var (unsigned int);
> @@ -4413,7 +4411,7 @@ expand_debug_parm_decl (tree decl)
>
>  /* Return an RTX equivalent to the value of the tree expression EXP.  */
>
> -static rtx
> +rtx
>  expand_debug_expr (tree exp)
>  {
>    rtx op0 = NULL_RTX, op1 = NULL_RTX, op2 = NULL_RTX;
> @@ -5285,6 +5283,14 @@ expand_debug_expr (tree exp)
>        else
>         goto flag_unsupported;
>
> +    case COMPOUND_LITERAL_EXPR:
> +      exp = COMPOUND_LITERAL_EXPR_DECL_EXPR (exp);
> +      /* DECL_EXPR is a tcc_statement, which expand_debug_expr does
> +        not expect, so instead of recursing we take care of it right
> +        away.  */
> +      exp = DECL_EXPR_DECL (exp);
> +      return expand_debug_expr (exp);
> +
>      case CALL_EXPR:
>        /* ??? Maybe handle some builtins?  */
>        return NULL;
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 82783c4968b85..bb7e2b8dc4e2c 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -20170,7 +20170,8 @@ add_const_value_attribute (dw_die_ref die, rtx rtl)
>        if (dwarf_version >= 4 || !dwarf_strict)
>         {
>           dw_loc_descr_ref loc_result;
> -         resolve_one_addr (&rtl);
> +         if (!resolve_one_addr (&rtl))
> +           return false;
>         rtl_addr:
>            loc_result = new_addr_loc_descr (rtl, dtprel_false);
>           add_loc_descr (&loc_result, new_loc_descr (DW_OP_stack_value, 0, 0));
> @@ -20255,6 +20256,12 @@ reference_to_unused (tree * tp, int * walk_subtrees,
>        varpool_node *node = varpool_node::get (*tp);
>        if (!node || !node->definition)
>         return *tp;
> +      /* If it's local, it might still be optimized out, unless we've
> +        already committed to outputting it by assigning RTL to it.  */
> +      if (! TREE_PUBLIC (*tp) && ! TREE_ASM_WRITTEN (*tp)
> +         && symtab->state <= IPA_SSA_AFTER_INLINING

Hmm, elsewhere in this function we're not anticipating future removal but
instead use ->global_info_ready which IIRC is when the unit was
initially analyzed.  So don't the other uses have the same issue?  Maybe
reference_to_unused is the wrong tool here and we need a
reference_to_discardable or so?

In other places we manage to use symbolic DIE references later resolved
by note_variable_values, can we maybe do this unconditionally for the
initializers of removed decls somehow?

> +         && ! DECL_RTL_SET_P (*tp))
> +       return *tp;
>      }
>    else if (TREE_CODE (*tp) == FUNCTION_DECL
>            && (!DECL_EXTERNAL (*tp) || DECL_DECLARED_INLINE_P (*tp)))
> @@ -20279,6 +20286,7 @@ static rtx
>  rtl_for_decl_init (tree init, tree type)
>  {
>    rtx rtl = NULL_RTX;
> +  bool valid_p = false;
>
>    STRIP_NOPS (init);
>
> @@ -20322,7 +20330,7 @@ rtl_for_decl_init (tree init, tree type)
>    /* If the initializer is something that we know will expand into an
>       immediate RTL constant, expand it now.  We must be careful not to
>       reference variables which won't be output.  */
> -  else if (initializer_constant_valid_p (init, type)
> +  else if ((valid_p = initializer_constant_valid_p (init, type))
>            && ! walk_tree (&init, reference_to_unused, NULL, NULL))
>      {
>        /* Convert vector CONSTRUCTOR initializers to VECTOR_CST if
> @@ -20367,6 +20375,9 @@ rtl_for_decl_init (tree init, tree type)
>        /* If expand_expr returns a MEM, it wasn't immediate.  */
>        gcc_assert (!rtl || !MEM_P (rtl));
>      }
> +  else if (valid_p)
> +    /* Perhaps we could just use this and skip all of the above?  */
> +    rtl = expand_debug_expr (init);
>
>    return rtl;
>  }
> diff --git a/gcc/expr.h b/gcc/expr.h
> index a4f44265759ce..7b060462020be 100644
> --- a/gcc/expr.h
> +++ b/gcc/expr.h
> @@ -307,6 +307,8 @@ expand_normal (tree exp)
>    return expand_expr_real (exp, NULL_RTX, VOIDmode, EXPAND_NORMAL, NULL, false);
>  }
>
> +/* This one is defined in cfgexpand.c.  */
> +extern rtx expand_debug_expr (tree);
>
>  /* Return STRING_CST and set offset, size and decl, if the first
>     argument corresponds to a string constant.  */
>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>