[PATCH] dwarf2out: Don't call expand_expr during early_dwarf [PR104407]

[PATCH] dwarf2out: Don't call expand_expr during early_dwarf [PR104407]

[Jakub Jelinek]

Hi!

As mentioned in the PR, since PR96690 r11-2834 we call rtl_for_decl_init
which can call expand_expr already during early_dwarf.  The comment and PR
explains it that the intent is to ensure the referenced vars and functions
are properly mangled because free_lang_data doesn't cover everything, like
template parameters etc.  It doesn't work well though, because expand_expr
can set DECL_RTLs e.g. on referenced vars and keep them there, and they can
be created e.g. with different MEM_ALIGN compared to what they would be
created with if they were emitted later.
So, the following patch stops calling rtl_for_decl_init and instead
for cases for which rtl_for_decl_init does anything at all walks the
initializer and ensures referenced vars or functions are mangled.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2022-02-08  Jakub Jelinek  <jakub@redhat.com>

	PR debug/104407
	* dwarf2out.cc (mangle_referenced_decls): New function.
	(tree_add_const_value_attribute): Don't call rtl_for_decl_init if
	early_dwarf.  Instead walk the initializer and try to mangle vars or
	functions referenced from it.

	* g++.dg/debug/dwarf2/pr104407.C: New test.

--- gcc/dwarf2out.cc.jj	2022-02-04 14:36:54.966605844 +0100
+++ gcc/dwarf2out.cc	2022-02-07 12:28:13.000520666 +0100
@@ -20881,6 +20881,19 @@ add_location_or_const_value_attribute (d
   return tree_add_const_value_attribute_for_decl (die, decl);
 }
 
+/* Mangle referenced decls.  */
+static tree
+mangle_referenced_decls (tree *tp, int *walk_subtrees, void *)
+{
+  if (! EXPR_P (*tp) && ! CONSTANT_CLASS_P (*tp))
+    *walk_subtrees = 0;
+
+  if (VAR_OR_FUNCTION_DECL_P (*tp))
+    assign_assembler_name_if_needed (*tp);
+
+  return NULL_TREE;
+}
+
 /* Attach a DW_AT_const_value attribute to DIE. The value of the
    attribute is the const value T.  */
 
@@ -20889,7 +20902,6 @@ tree_add_const_value_attribute (dw_die_r
 {
   tree init;
   tree type = TREE_TYPE (t);
-  rtx rtl;
 
   if (!t || !TREE_TYPE (t) || TREE_TYPE (t) == error_mark_node)
     return false;
@@ -20910,11 +20922,26 @@ tree_add_const_value_attribute (dw_die_r
 	  return true;
 	}
     }
-  /* Generate the RTL even if early_dwarf to force mangling of all refered to
-     symbols.  */
-  rtl = rtl_for_decl_init (init, type);
-  if (rtl && !early_dwarf)
-    return add_const_value_attribute (die, TYPE_MODE (type), rtl);
+  if (!early_dwarf)
+    {
+      rtx rtl = rtl_for_decl_init (init, type);
+      if (rtl && !early_dwarf)
+	return add_const_value_attribute (die, TYPE_MODE (type), rtl);
+    }
+  else
+    {
+      /* For early_dwarf force mangling of all referenced symbols.  */
+      tree initializer = init;
+      STRIP_NOPS (initializer);
+      /* rtl_for_decl_init punts on other aggregates, and complex values.  */
+      if (AGGREGATE_TYPE_P (type)
+	  || (TREE_CODE (initializer) == VIEW_CONVERT_EXPR
+	      && AGGREGATE_TYPE_P (TREE_TYPE (TREE_OPERAND (initializer, 0))))
+	  || TREE_CODE (type) == COMPLEX_TYPE)
+	;
+      else if (initializer_constant_valid_p (initializer, type))
+	walk_tree (&initializer, mangle_referenced_decls, NULL, NULL);
+    }
   /* If the host and target are sane, try harder.  */
   if (CHAR_BIT == 8 && BITS_PER_UNIT == 8
       && initializer_constant_valid_p (init, type))
--- gcc/testsuite/g++.dg/debug/dwarf2/pr104407.C.jj	2022-02-07 10:45:51.945041031 +0100
+++ gcc/testsuite/g++.dg/debug/dwarf2/pr104407.C	2022-02-07 10:45:09.834635340 +0100
@@ -0,0 +1,12 @@
+// PR debug/104407
+// { dg-do compile { target c++17 } }
+// { dg-options "-O1 -fcompare-debug" }
+
+struct A { int i; long j; int k : 2; char l; } a;
+
+auto [ aa, bb, cc, dd ] = a;
+
+namespace N
+{
+  auto & [ m, n, o, ppp ] = a;
+}

	Jakub

Re: [PATCH] dwarf2out: Don't call expand_expr during early_dwarf [PR104407]

[Richard Biener]

On Tue, 8 Feb 2022, Jakub Jelinek wrote:

> Hi!
> 
> As mentioned in the PR, since PR96690 r11-2834 we call rtl_for_decl_init
> which can call expand_expr already during early_dwarf.  The comment and PR
> explains it that the intent is to ensure the referenced vars and functions
> are properly mangled because free_lang_data doesn't cover everything, like
> template parameters etc.  It doesn't work well though, because expand_expr

free_lang_data mangling also only runs when generating LTO bytecode...

> can set DECL_RTLs e.g. on referenced vars and keep them there, and they can
> be created e.g. with different MEM_ALIGN compared to what they would be
> created with if they were emitted later.
> So, the following patch stops calling rtl_for_decl_init and instead
> for cases for which rtl_for_decl_init does anything at all walks the
> initializer and ensures referenced vars or functions are mangled.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2022-02-08  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/104407
> 	* dwarf2out.cc (mangle_referenced_decls): New function.
> 	(tree_add_const_value_attribute): Don't call rtl_for_decl_init if
> 	early_dwarf.  Instead walk the initializer and try to mangle vars or
> 	functions referenced from it.
> 
> 	* g++.dg/debug/dwarf2/pr104407.C: New test.
> 
> --- gcc/dwarf2out.cc.jj	2022-02-04 14:36:54.966605844 +0100
> +++ gcc/dwarf2out.cc	2022-02-07 12:28:13.000520666 +0100
> @@ -20881,6 +20881,19 @@ add_location_or_const_value_attribute (d
>    return tree_add_const_value_attribute_for_decl (die, decl);
>  }
>  
> +/* Mangle referenced decls.  */
> +static tree
> +mangle_referenced_decls (tree *tp, int *walk_subtrees, void *)
> +{
> +  if (! EXPR_P (*tp) && ! CONSTANT_CLASS_P (*tp))
> +    *walk_subtrees = 0;
> +
> +  if (VAR_OR_FUNCTION_DECL_P (*tp))
> +    assign_assembler_name_if_needed (*tp);
> +
> +  return NULL_TREE;
> +}
> +
>  /* Attach a DW_AT_const_value attribute to DIE. The value of the
>     attribute is the const value T.  */
>  
> @@ -20889,7 +20902,6 @@ tree_add_const_value_attribute (dw_die_r
>  {
>    tree init;
>    tree type = TREE_TYPE (t);
> -  rtx rtl;
>  
>    if (!t || !TREE_TYPE (t) || TREE_TYPE (t) == error_mark_node)
>      return false;
> @@ -20910,11 +20922,26 @@ tree_add_const_value_attribute (dw_die_r
>  	  return true;
>  	}
>      }
> -  /* Generate the RTL even if early_dwarf to force mangling of all refered to
> -     symbols.  */
> -  rtl = rtl_for_decl_init (init, type);
> -  if (rtl && !early_dwarf)
> -    return add_const_value_attribute (die, TYPE_MODE (type), rtl);
> +  if (!early_dwarf)
> +    {
> +      rtx rtl = rtl_for_decl_init (init, type);
> +      if (rtl && !early_dwarf)

the !early_dwarf is redundant

Otherwise looks good to me, please give Jason the opportunity to comment
though.

Thanks,
Richard.

> +	return add_const_value_attribute (die, TYPE_MODE (type), rtl);
> +    }
> +  else
> +    {
> +      /* For early_dwarf force mangling of all referenced symbols.  */
> +      tree initializer = init;
> +      STRIP_NOPS (initializer);
> +      /* rtl_for_decl_init punts on other aggregates, and complex values.  */
> +      if (AGGREGATE_TYPE_P (type)
> +	  || (TREE_CODE (initializer) == VIEW_CONVERT_EXPR
> +	      && AGGREGATE_TYPE_P (TREE_TYPE (TREE_OPERAND (initializer, 0))))
> +	  || TREE_CODE (type) == COMPLEX_TYPE)
> +	;
> +      else if (initializer_constant_valid_p (initializer, type))
> +	walk_tree (&initializer, mangle_referenced_decls, NULL, NULL);
> +    }
>    /* If the host and target are sane, try harder.  */
>    if (CHAR_BIT == 8 && BITS_PER_UNIT == 8
>        && initializer_constant_valid_p (init, type))
> --- gcc/testsuite/g++.dg/debug/dwarf2/pr104407.C.jj	2022-02-07 10:45:51.945041031 +0100
> +++ gcc/testsuite/g++.dg/debug/dwarf2/pr104407.C	2022-02-07 10:45:09.834635340 +0100
> @@ -0,0 +1,12 @@
> +// PR debug/104407
> +// { dg-do compile { target c++17 } }
> +// { dg-options "-O1 -fcompare-debug" }
> +
> +struct A { int i; long j; int k : 2; char l; } a;
> +
> +auto [ aa, bb, cc, dd ] = a;
> +
> +namespace N
> +{
> +  auto & [ m, n, o, ppp ] = a;
> +}
> 
> 	Jakub
> 
> 

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

Re: [PATCH] dwarf2out: Don't call expand_expr during early_dwarf [PR104407]

[Jason Merrill]

On 2/8/22 03:35, Richard Biener wrote:
> On Tue, 8 Feb 2022, Jakub Jelinek wrote:
> 
>> Hi!
>>
>> As mentioned in the PR, since PR96690 r11-2834 we call rtl_for_decl_init
>> which can call expand_expr already during early_dwarf.  The comment and PR
>> explains it that the intent is to ensure the referenced vars and functions
>> are properly mangled because free_lang_data doesn't cover everything, like
>> template parameters etc.  It doesn't work well though, because expand_expr
> 
> free_lang_data mangling also only runs when generating LTO bytecode...
> 
>> can set DECL_RTLs e.g. on referenced vars and keep them there, and they can
>> be created e.g. with different MEM_ALIGN compared to what they would be
>> created with if they were emitted later.
>> So, the following patch stops calling rtl_for_decl_init and instead
>> for cases for which rtl_for_decl_init does anything at all walks the
>> initializer and ensures referenced vars or functions are mangled.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> 2022-02-08  Jakub Jelinek  <jakub@redhat.com>
>>
>> 	PR debug/104407
>> 	* dwarf2out.cc (mangle_referenced_decls): New function.
>> 	(tree_add_const_value_attribute): Don't call rtl_for_decl_init if
>> 	early_dwarf.  Instead walk the initializer and try to mangle vars or
>> 	functions referenced from it.
>>
>> 	* g++.dg/debug/dwarf2/pr104407.C: New test.
>>
>> --- gcc/dwarf2out.cc.jj	2022-02-04 14:36:54.966605844 +0100
>> +++ gcc/dwarf2out.cc	2022-02-07 12:28:13.000520666 +0100
>> @@ -20881,6 +20881,19 @@ add_location_or_const_value_attribute (d
>>     return tree_add_const_value_attribute_for_decl (die, decl);
>>   }
>>   
>> +/* Mangle referenced decls.  */
>> +static tree
>> +mangle_referenced_decls (tree *tp, int *walk_subtrees, void *)
>> +{
>> +  if (! EXPR_P (*tp) && ! CONSTANT_CLASS_P (*tp))
>> +    *walk_subtrees = 0;
>> +
>> +  if (VAR_OR_FUNCTION_DECL_P (*tp))
>> +    assign_assembler_name_if_needed (*tp);
>> +
>> +  return NULL_TREE;
>> +}
>> +
>>   /* Attach a DW_AT_const_value attribute to DIE. The value of the
>>      attribute is the const value T.  */
>>   
>> @@ -20889,7 +20902,6 @@ tree_add_const_value_attribute (dw_die_r
>>   {
>>     tree init;
>>     tree type = TREE_TYPE (t);
>> -  rtx rtl;
>>   
>>     if (!t || !TREE_TYPE (t) || TREE_TYPE (t) == error_mark_node)
>>       return false;
>> @@ -20910,11 +20922,26 @@ tree_add_const_value_attribute (dw_die_r
>>   	  return true;
>>   	}
>>       }
>> -  /* Generate the RTL even if early_dwarf to force mangling of all refered to
>> -     symbols.  */
>> -  rtl = rtl_for_decl_init (init, type);
>> -  if (rtl && !early_dwarf)
>> -    return add_const_value_attribute (die, TYPE_MODE (type), rtl);
>> +  if (!early_dwarf)
>> +    {
>> +      rtx rtl = rtl_for_decl_init (init, type);
>> +      if (rtl && !early_dwarf)
> 
> the !early_dwarf is redundant
> 
> Otherwise looks good to me, please give Jason the opportunity to comment
> though.
> 
> Thanks,
> Richard.
> 
>> +	return add_const_value_attribute (die, TYPE_MODE (type), rtl);
>> +    }
>> +  else
>> +    {
>> +      /* For early_dwarf force mangling of all referenced symbols.  */
>> +      tree initializer = init;
>> +      STRIP_NOPS (initializer);
>> +      /* rtl_for_decl_init punts on other aggregates, and complex values.  */
>> +      if (AGGREGATE_TYPE_P (type)
>> +	  || (TREE_CODE (initializer) == VIEW_CONVERT_EXPR
>> +	      && AGGREGATE_TYPE_P (TREE_TYPE (TREE_OPERAND (initializer, 0))))
>> +	  || TREE_CODE (type) == COMPLEX_TYPE)
>> +	;

Hmm, I don't think we need to mirror the limitations of 
rtl_for_decl_init here; IMO we might as well go ahead mangle everything 
so we don't need to change this place as well when the FIXME is 
resolved.  OK either way, but if you decide to keep this condition 
please update the FIXME in rtl_for_decl_init to point here.

>> +      else if (initializer_constant_valid_p (initializer, type))
>> +	walk_tree (&initializer, mangle_referenced_decls, NULL, NULL);
>> +    }
>>     /* If the host and target are sane, try harder.  */
>>     if (CHAR_BIT == 8 && BITS_PER_UNIT == 8
>>         && initializer_constant_valid_p (init, type))
>> --- gcc/testsuite/g++.dg/debug/dwarf2/pr104407.C.jj	2022-02-07 10:45:51.945041031 +0100
>> +++ gcc/testsuite/g++.dg/debug/dwarf2/pr104407.C	2022-02-07 10:45:09.834635340 +0100
>> @@ -0,0 +1,12 @@
>> +// PR debug/104407
>> +// { dg-do compile { target c++17 } }
>> +// { dg-options "-O1 -fcompare-debug" }
>> +
>> +struct A { int i; long j; int k : 2; char l; } a;
>> +
>> +auto [ aa, bb, cc, dd ] = a;
>> +
>> +namespace N
>> +{
>> +  auto & [ m, n, o, ppp ] = a;
>> +}
>>
>> 	Jakub
>>
>>
> 

Re: [PATCH] dwarf2out: Don't call expand_expr during early_dwarf [PR104407]

[Jakub Jelinek]

On Tue, Feb 08, 2022 at 02:20:28PM -0500, Jason Merrill wrote:
> > > +	return add_const_value_attribute (die, TYPE_MODE (type), rtl);
> > > +    }
> > > +  else
> > > +    {
> > > +      /* For early_dwarf force mangling of all referenced symbols.  */
> > > +      tree initializer = init;
> > > +      STRIP_NOPS (initializer);
> > > +      /* rtl_for_decl_init punts on other aggregates, and complex values.  */
> > > +      if (AGGREGATE_TYPE_P (type)
> > > +	  || (TREE_CODE (initializer) == VIEW_CONVERT_EXPR
> > > +	      && AGGREGATE_TYPE_P (TREE_TYPE (TREE_OPERAND (initializer, 0))))
> > > +	  || TREE_CODE (type) == COMPLEX_TYPE)
> > > +	;
> 
> Hmm, I don't think we need to mirror the limitations of rtl_for_decl_init
> here; IMO we might as well go ahead mangle everything so we don't need to
> change this place as well when the FIXME is resolved.  OK either way, but if
> you decide to keep this condition please update the FIXME in
> rtl_for_decl_init to point here.

I did that because the aggregate case can mean very large initializers that
would need mangling a lot of symbols even when we actually don't need that
because we'd punt on it, and because I'm a little bit afraid how safe the
mangling is for -fcompare-debug if done only for the -g case (I vaguely
remember it can try to instantiate some templates etc.).
Can certainly adjust the FIXME comment.

	Jakub