avoid early reference to debug-only symbol

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(-)

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>

On Jul 13, 2021, Richard Biener <richard.guenther@gmail.com> wrote:

> 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?

Possibly.

The call to debug_hooks->late_global_decl in symtab::remove_unused_nodes
is only for varpool nodes, and only variables have initializers that are
expanded as if for non-debug uses, but the initializers *might*
reference function symbols, and that might in turn lead to analogous
codegen differences.  I don't have a testcase for that, but it might not
be hard to come up with an analogous one.

> Maybe reference_to_unused is the wrong tool here and we need a
> reference_to_discardable or so?

Yeah, back when we expanded debug info very late in compilation,
reference_to_unused was good enough, but now that we process and discard
stuff early, what we really need is some way to avoid issuing a hard
reference to something that might end up not be referenced at all if it
weren't for debug info.

> 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?

That seems doable and desirable.  decContext.c doesn't generate location
or const_value for neither mfctop nor mfcone, even after removing
rtl_for_decl_location from add_location_or_const_value_attribute,
despite lookup_decl_loc's finding a location expression.

I'm afraid I can't really commit to pursuing this any further.

I'd be glad if someone more familiar with this were to pick this up, but
I haven't managed to come up with a testcase to trigger the problem
without the patchset that I'm not ready to contribute.

The kicker, in case someone wants to force it, is that at
materialize_all_clones, the reference to mfctop in a constprop-ed
decContextTestEndian gets substituted by its constant initializer,
&mfcone, so mfctop becomes unreachable and goes through late_global_decl
in remove_unreachable_nodes.  Eventually, ccp2 resolves the mfcone
dereference to a constant, and no reason remains for mfcone to be
output.  However, when outputting debug info, the expand_expr of
mfctop's initializer will have already generated RTL for mfcone,
committing it to be output, wehreas without debug info, mfcone is not
output at all.

What enables these transformations during IPA is the introduction of a
wrapper for decContextTestEndian, moving its body to a clone that is
materialized early, at the end of all_small_ipa_passes.  This clone,
being a local function, seems to be what enables the substitution of the
mfctop constant initializer.  I haven't found a way to cause such a
difference without my pass, even getting a constprop (local) clone for
the function, and preventing its inlining into an exported function.

Hopefully this is enough information to enable the issue to be
investigated.

On Wed, Jul 14, 2021 at 7:10 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Jul 13, 2021, Richard Biener <richard.guenther@gmail.com> wrote:
>
> > 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?
>
> Possibly.
>
> The call to debug_hooks->late_global_decl in symtab::remove_unused_nodes
> is only for varpool nodes, and only variables have initializers that are
> expanded as if for non-debug uses, but the initializers *might*
> reference function symbols, and that might in turn lead to analogous
> codegen differences.  I don't have a testcase for that, but it might not
> be hard to come up with an analogous one.
>
> > Maybe reference_to_unused is the wrong tool here and we need a
> > reference_to_discardable or so?
>
> Yeah, back when we expanded debug info very late in compilation,
> reference_to_unused was good enough, but now that we process and discard
> stuff early, what we really need is some way to avoid issuing a hard
> reference to something that might end up not be referenced at all if it
> weren't for debug info.
>
> > 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?
>
> That seems doable and desirable.  decContext.c doesn't generate location
> or const_value for neither mfctop nor mfcone, even after removing
> rtl_for_decl_location from add_location_or_const_value_attribute,
> despite lookup_decl_loc's finding a location expression.
>
> I'm afraid I can't really commit to pursuing this any further.
>
> I'd be glad if someone more familiar with this were to pick this up, but
> I haven't managed to come up with a testcase to trigger the problem
> without the patchset that I'm not ready to contribute.
>
> The kicker, in case someone wants to force it, is that at
> materialize_all_clones, the reference to mfctop in a constprop-ed
> decContextTestEndian gets substituted by its constant initializer,
> &mfcone, so mfctop becomes unreachable and goes through late_global_decl
> in remove_unreachable_nodes.  Eventually, ccp2 resolves the mfcone
> dereference to a constant, and no reason remains for mfcone to be
> output.  However, when outputting debug info, the expand_expr of
> mfctop's initializer will have already generated RTL for mfcone,
> committing it to be output, wehreas without debug info, mfcone is not
> output at all.
>
> What enables these transformations during IPA is the introduction of a
> wrapper for decContextTestEndian, moving its body to a clone that is
> materialized early, at the end of all_small_ipa_passes.  This clone,
> being a local function, seems to be what enables the substitution of the
> mfctop constant initializer.  I haven't found a way to cause such a
> difference without my pass, even getting a constprop (local) clone for
> the function, and preventing its inlining into an exported function.
>
> Hopefully this is enough information to enable the issue to be
> investigated.

Can you put the above (and maybe your patch) into a PR so it doesn't
get lost?

Thanks,
Richard.

>
> --
> 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>

On Jul 14, 2021, Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> Can you put the above (and maybe your patch) into a PR so it doesn't
> get lost?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101454