Re: [PATCH][PR84213] DWARF: no location for non-definition DECLs with non-trivial DECL_VALUE_EXPR

[Richard Biener <rguenther at suse dot de>]

On Fri, 9 Feb 2018, Pierre-Marie de Rodat wrote:

> This patch restricts the set of cases in which we allow the generation of
> location attributes for variables that are not defined in the current unit.
> For such variables with complex DECL_VALUE_EXPR trees, generating a location
> attribute can end up creating relocations to text symbols in the debug section
> of LTO object files, which is not valid.
> 
> Richard originally suggested to revert r248792 and then enhance
> tree_add_const_value_attribute_for_decl to generate a DW_AT_location attribute
> in the specific case of a DECL_VALUE_EXPR that is an
> INDIRECT_REF(NOP_EXPR(INTEGER_CST)). I ended up with this patch because
> changing a function called "tree_add_const_value_attribute*" to generate
> instead a location attribute whereas there already exists a function called
> "add_location_or_const_value_attribute" felt really wrong. ;-) Especially if
> this would either re-implement part of the latter function in the former one.
> 
> What I did instead was to make dwarf2out_late_global_decl call
> add_location_or_const_value_attribute only when we know it's safe to do so
> (i.e. when it won't generate relocations to text symbols). I have the  feeling
> that it is cleaner and from what I could see, it fixes the reported issue with
> no regression.
> 
> Bootstrapped and regtested on x86_64-linux. Ok to commit to trunk?

Ok with ...

> gcc/
> 	PR lto/84213
> 	* dwarf2out.c (is_trivial_indirect_ref): New function.
> 	(dwarf2out_late_global_decl): Do not generate a location attribute for
> 	variables that have a non-trivial DECL_VALUE_EXPR and that are not
> 	defined in the current unit.
> ---
>  gcc/dwarf2out.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 948b3cb..6538596 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -25716,6 +25716,23 @@ dwarf2out_early_global_decl (tree decl)
>    symtab->global_info_ready = save;
>  }
>  +/* Return whether EXPR is an expression with the following pattern:
> +   INDIRECT_REF (NOP_EXPR (INTEGER_CST)).  */
> +

whitespace fixed here - vertical space missing before the comment
and the first line of the comment (or cut&paste error with the patch?)

Thanks,
Richard.

> +static bool
> +is_trivial_indirect_ref (tree expr)
> +{
> +  if (expr == NULL_TREE || TREE_CODE (expr) != INDIRECT_REF)
> +    return false;
> +
> +  tree nop = TREE_OPERAND (expr, 0);
> +  if (nop == NULL_TREE || TREE_CODE (nop) != NOP_EXPR)
> +    return false;
> +
> +  tree int_cst = TREE_OPERAND (nop, 0);
> +  return int_cst != NULL_TREE && TREE_CODE (int_cst) == INTEGER_CST;
> +}
> +
>  /* Output debug information for global decl DECL.  Called from
>     toplev.c after compilation proper has finished.  */
>  @@ -25740,11 +25757,17 @@ dwarf2out_late_global_decl (tree decl)
>        if (die)
>  	{
>  	  /* We get called via the symtab code invoking late_global_decl
> -	     for symbols that are optimized out.  Do not add locations
> -	     for those, except if they have a DECL_VALUE_EXPR, in which case
> -	     they are relevant for debuggers.  */
> +	     for symbols that are optimized out.
> +
> +	     Do not add locations for those, except if they have a
> +	     DECL_VALUE_EXPR, in which case they are relevant for debuggers.
> +	     Still don't add a location if the DECL_VALUE_EXPR is not a
> trivial
> +	     INDIRECT_REF expression, as this could generate relocations to
> +	     text symbols in LTO object files, which is invalid.  */
>  	  varpool_node *node = varpool_node::get (decl);
> -	  if ((! node || ! node->definition) && ! DECL_HAS_VALUE_EXPR_P
> (decl))
> +	  if ((! node || ! node->definition)
> +	      && ! (DECL_HAS_VALUE_EXPR_P (decl)
> +		    && is_trivial_indirect_ref (DECL_VALUE_EXPR (decl))))
>  	    tree_add_const_value_attribute_for_decl (die, decl);
>  	  else
>  	    add_location_or_const_value_attribute (die, decl, false);
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)