[PATCH][C++] Avoid PCH dependent mangling

[PATCH][C++] Avoid PCH dependent mangling

[Richard Biener]

With the passes.c hunk in the patch below we FAIL assembly comparison
of g++.dg/pch/system-[12].C because with PCH we have computed
DECL_ASSEMBLER_NAME and thus appended DW_AT_linkage_name early during
PCH generation while without PCH we compute it lazily and end up
appending DW_AT_specification earlier.  Thus we have swapped dwarf
attribute order and assembly comparison fails.

Clearly this kind of "IL" changes dependent on whether we are writing
a PCH file is going to cause differences down the food chain.
(there is another case in instantiate_decl calling add_pending_template
dependent on pch_file)

Now a simple solution is to simply not do that (mangle decls).  Another
would be to always mangle decls where we now do so conditional on PCH.
Another soulution is to declare we don't care about assembly differences
with/without using PCH and remove assembly comparison from the
testsuite harness.

Bootstrapped on x86_64-unknown-linux-gnu, testing + gdb testing in 
progress.

Ok for trunk (with note_decl_for_pch completely removed)?

The passes.c hunk is needed because we otherwise miss to properly
create the early DIEs for those kind of globals (we'll be left
with a declaration DIE from the type DIE creation and miss the
definition part).

Thanks,
Richard.

2015-08-27  Richard Biener  <rguenther@suse.de>

	* passes.c (rest_of_decl_compilation): Also call early_global_decl
	on global definitions in type context.

	cp/
	* semantics.c (note_decl_for_pch): Do not mangle the decl.

Index: gcc/passes.c
===================================================================
--- gcc/passes.c	(revision 227258)
+++ gcc/passes.c	(working copy)
@@ -318,7 +318,15 @@ rest_of_decl_compilation (tree decl,
       && !decl_function_context (decl)
       && !current_function_decl
       && DECL_SOURCE_LOCATION (decl) != BUILTINS_LOCATION
-      && !decl_type_context (decl)
+      && (!decl_type_context (decl)
+	  /* If we created a varpool node for the decl make sure to
+	     call early_global_decl.  Otherwise we miss changes
+	     introduced by member definitions like
+	       struct A { static int staticdatamember; };
+	       int A::staticdatamember;
+	     and thus have incomplete early debug.  */
+	  || (TREE_CODE (decl) == VAR_DECL
+	      && TREE_STATIC (decl) && !DECL_EXTERNAL (decl)))
       /* Avoid confusing the debug information machinery when there are
 	 errors.  */
       && !seen_error ())
Index: gcc/cp/semantics.c
===================================================================
--- gcc/cp/semantics.c	(revision 227258)
+++ gcc/cp/semantics.c	(working copy)
@@ -2962,15 +2962,21 @@ finish_member_declaration (tree decl)
    translation units which include the PCH file.  */
 
 void
-note_decl_for_pch (tree decl)
+note_decl_for_pch (tree)
 {
   gcc_assert (pch_file);
 
+  /* ???  This changes debug info with/without PCH as DW_AT_linkage_name
+     attributes are added at different times (early when with PCH
+     or late, via pending assembler names, when without PCH).
+     See g++.dg/pch/system-[12].C.  */
+#if 0
   /* There's a good chance that we'll have to mangle names at some
      point, even if only for emission in debugging information.  */
   if (VAR_OR_FUNCTION_DECL_P (decl)
       && !processing_template_decl)
     mangle_decl (decl);
+#endif
 }
 
 /* Finish processing a complete template declaration.  The PARMS are

Re: [PATCH][C++] Avoid PCH dependent mangling

[Richard Biener]

On Thu, 27 Aug 2015, Richard Biener wrote:

> 
> With the passes.c hunk in the patch below we FAIL assembly comparison
> of g++.dg/pch/system-[12].C because with PCH we have computed
> DECL_ASSEMBLER_NAME and thus appended DW_AT_linkage_name early during
> PCH generation while without PCH we compute it lazily and end up
> appending DW_AT_specification earlier.  Thus we have swapped dwarf
> attribute order and assembly comparison fails.
> 
> Clearly this kind of "IL" changes dependent on whether we are writing
> a PCH file is going to cause differences down the food chain.
> (there is another case in instantiate_decl calling add_pending_template
> dependent on pch_file)
> 
> Now a simple solution is to simply not do that (mangle decls).  Another
> would be to always mangle decls where we now do so conditional on PCH.
> Another soulution is to declare we don't care about assembly differences
> with/without using PCH and remove assembly comparison from the
> testsuite harness.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing + gdb testing in 
> progress.
> 
> Ok for trunk (with note_decl_for_pch completely removed)?

Ping.

The C++ issue prevents me from refactoring late global decl dwarf
and I'd rather not munge that together with this C++ specific issue.

Thanks,
Richard.

> The passes.c hunk is needed because we otherwise miss to properly
> create the early DIEs for those kind of globals (we'll be left
> with a declaration DIE from the type DIE creation and miss the
> definition part).
> 
> Thanks,
> Richard.
> 
> 2015-08-27  Richard Biener  <rguenther@suse.de>
> 
> 	* passes.c (rest_of_decl_compilation): Also call early_global_decl
> 	on global definitions in type context.
> 
> 	cp/
> 	* semantics.c (note_decl_for_pch): Do not mangle the decl.
> 
> Index: gcc/passes.c
> ===================================================================
> --- gcc/passes.c	(revision 227258)
> +++ gcc/passes.c	(working copy)
> @@ -318,7 +318,15 @@ rest_of_decl_compilation (tree decl,
>        && !decl_function_context (decl)
>        && !current_function_decl
>        && DECL_SOURCE_LOCATION (decl) != BUILTINS_LOCATION
> -      && !decl_type_context (decl)
> +      && (!decl_type_context (decl)
> +	  /* If we created a varpool node for the decl make sure to
> +	     call early_global_decl.  Otherwise we miss changes
> +	     introduced by member definitions like
> +	       struct A { static int staticdatamember; };
> +	       int A::staticdatamember;
> +	     and thus have incomplete early debug.  */
> +	  || (TREE_CODE (decl) == VAR_DECL
> +	      && TREE_STATIC (decl) && !DECL_EXTERNAL (decl)))
>        /* Avoid confusing the debug information machinery when there are
>  	 errors.  */
>        && !seen_error ())
> Index: gcc/cp/semantics.c
> ===================================================================
> --- gcc/cp/semantics.c	(revision 227258)
> +++ gcc/cp/semantics.c	(working copy)
> @@ -2962,15 +2962,21 @@ finish_member_declaration (tree decl)
>     translation units which include the PCH file.  */
>  
>  void
> -note_decl_for_pch (tree decl)
> +note_decl_for_pch (tree)
>  {
>    gcc_assert (pch_file);
>  
> +  /* ???  This changes debug info with/without PCH as DW_AT_linkage_name
> +     attributes are added at different times (early when with PCH
> +     or late, via pending assembler names, when without PCH).
> +     See g++.dg/pch/system-[12].C.  */
> +#if 0
>    /* There's a good chance that we'll have to mangle names at some
>       point, even if only for emission in debugging information.  */
>    if (VAR_OR_FUNCTION_DECL_P (decl)
>        && !processing_template_decl)
>      mangle_decl (decl);
> +#endif
>  }
>  
>  /* Finish processing a complete template declaration.  The PARMS are
> 

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

Re: [PATCH][C++] Avoid PCH dependent mangling

[Jason Merrill]

On 08/27/2015 09:36 AM, Richard Biener wrote:
>
> With the passes.c hunk in the patch below we FAIL assembly comparison
> of g++.dg/pch/system-[12].C because with PCH we have computed
> DECL_ASSEMBLER_NAME and thus appended DW_AT_linkage_name early during
> PCH generation while without PCH we compute it lazily and end up
> appending DW_AT_specification earlier.  Thus we have swapped dwarf
> attribute order and assembly comparison fails.
>
> Clearly this kind of "IL" changes dependent on whether we are writing
> a PCH file is going to cause differences down the food chain.
> (there is another case in instantiate_decl calling add_pending_template
> dependent on pch_file)
>
> Now a simple solution is to simply not do that (mangle decls).  Another
> would be to always mangle decls where we now do so conditional on PCH.
> Another soulution is to declare we don't care about assembly differences
> with/without using PCH and remove assembly comparison from the
> testsuite harness.

Hmm, what if we walk through the symtab and mangle everything later, 
when we're about to write the PCH?  That should still get the benefit of 
doing the mangling work only once, without changing the order of the 
attributes.

Jason

Re: [PATCH][C++] Avoid PCH dependent mangling

[Richard Biener]

On September 15, 2015 4:25:37 PM GMT+02:00, Jason Merrill <jason@redhat.com> wrote:
>On 08/27/2015 09:36 AM, Richard Biener wrote:
>>
>> With the passes.c hunk in the patch below we FAIL assembly comparison
>> of g++.dg/pch/system-[12].C because with PCH we have computed
>> DECL_ASSEMBLER_NAME and thus appended DW_AT_linkage_name early during
>> PCH generation while without PCH we compute it lazily and end up
>> appending DW_AT_specification earlier.  Thus we have swapped dwarf
>> attribute order and assembly comparison fails.
>>
>> Clearly this kind of "IL" changes dependent on whether we are writing
>> a PCH file is going to cause differences down the food chain.
>> (there is another case in instantiate_decl calling
>add_pending_template
>> dependent on pch_file)
>>
>> Now a simple solution is to simply not do that (mangle decls). 
>Another
>> would be to always mangle decls where we now do so conditional on
>PCH.
>> Another soulution is to declare we don't care about assembly
>differences
>> with/without using PCH and remove assembly comparison from the
>> testsuite harness.
>
>Hmm, what if we walk through the symtab and mangle everything later, 
>when we're about to write the PCH?  That should still get the benefit
>of 
>doing the mangling work only once, without changing the order of the 
>attributes.

That might work if we can get at all relevant decls that way.  If not we can populate a pointer-set from the function and walk that before writing the PCH. I can do that if you prefer, I just didn't know if we care about PCH performance enough to worry.

Thanks,
Richard.

>
>Jason

Re: [PATCH][C++] Avoid PCH dependent mangling

[Richard Biener]

On Tue, 15 Sep 2015, Richard Biener wrote:

> On September 15, 2015 4:25:37 PM GMT+02:00, Jason Merrill <jason@redhat.com> wrote:
> >On 08/27/2015 09:36 AM, Richard Biener wrote:
> >>
> >> With the passes.c hunk in the patch below we FAIL assembly comparison
> >> of g++.dg/pch/system-[12].C because with PCH we have computed
> >> DECL_ASSEMBLER_NAME and thus appended DW_AT_linkage_name early during
> >> PCH generation while without PCH we compute it lazily and end up
> >> appending DW_AT_specification earlier.  Thus we have swapped dwarf
> >> attribute order and assembly comparison fails.
> >>
> >> Clearly this kind of "IL" changes dependent on whether we are writing
> >> a PCH file is going to cause differences down the food chain.
> >> (there is another case in instantiate_decl calling
> >add_pending_template
> >> dependent on pch_file)
> >>
> >> Now a simple solution is to simply not do that (mangle decls). 
> >Another
> >> would be to always mangle decls where we now do so conditional on
> >PCH.
> >> Another soulution is to declare we don't care about assembly
> >differences
> >> with/without using PCH and remove assembly comparison from the
> >> testsuite harness.
> >
> >Hmm, what if we walk through the symtab and mangle everything later, 
> >when we're about to write the PCH?  That should still get the benefit
> >of 
> >doing the mangling work only once, without changing the order of the 
> >attributes.
> 
> That might work if we can get at all relevant decls that way.  If not we 
> can populate a pointer-set from the function and walk that before 
> writing the PCH. I can do that if you prefer, I just didn't know if we 
> care about PCH performance enough to worry.

Ok, so the following fixes my pch.exp issues.  It drops note_decl_for_pch
in favor of mangling all globals before PCH write.

Bootstrap & regtest on x86_64-unknown-linux-gnu in progress, ok for trunk?

Thanks,
Richard.

2015-09-16  Richard Biener  <rguenther@suse.de>

	cp/
	* cp-tree.h (note_decl_for_pch): Remove.
	* class.c (build_clone): Do not call note_decl_for_pch.
	* semantics.c (finish_member_declaration): Likewise.
	(note_decl_for_pch): Remove.
	* decl2.c (c_parse_final_cleanups): Mangle all globals before
	writing the PCH.

Index: gcc/cp/class.c
===================================================================
*** gcc/cp/class.c	(revision 227779)
--- gcc/cp/class.c	(working copy)
*************** build_clone (tree fn, tree name)
*** 4691,4699 ****
    SET_DECL_RTL (clone, NULL);
    rest_of_decl_compilation (clone, /*top_level=*/1, at_eof);
  
-   if (pch_file)
-     note_decl_for_pch (clone);
- 
    return clone;
  }
  
--- 4691,4696 ----
Index: gcc/cp/semantics.c
===================================================================
*** gcc/cp/semantics.c	(revision 227779)
--- gcc/cp/semantics.c	(working copy)
*************** finish_member_declaration (tree decl)
*** 2951,2976 ****
        maybe_add_class_template_decl_list (current_class_type, decl,
  					  /*friend_p=*/0);
      }
- 
-   if (pch_file)
-     note_decl_for_pch (decl);
- }
- 
- /* DECL has been declared while we are building a PCH file.  Perform
-    actions that we might normally undertake lazily, but which can be
-    performed now so that they do not have to be performed in
-    translation units which include the PCH file.  */
- 
- void
- note_decl_for_pch (tree decl)
- {
-   gcc_assert (pch_file);
- 
-   /* There's a good chance that we'll have to mangle names at some
-      point, even if only for emission in debugging information.  */
-   if (VAR_OR_FUNCTION_DECL_P (decl)
-       && !processing_template_decl)
-     mangle_decl (decl);
  }
  
  /* Finish processing a complete template declaration.  The PARMS are
--- 2951,2956 ----
Index: gcc/cp/decl2.c
===================================================================
*** gcc/cp/decl2.c	(revision 227779)
--- gcc/cp/decl2.c	(working copy)
*************** c_parse_final_cleanups (void)
*** 4511,4516 ****
--- 4511,4522 ----
       In that case we do not want to do anything else.  */
    if (pch_file)
      {
+       /* Mangle all symbols at PCH creation time.  */
+       symtab_node *node;
+       FOR_EACH_SYMBOL (node)
+ 	if (! is_a <varpool_node *> (node)
+ 	    || ! DECL_HARD_REGISTER (node->decl))
+ 	  DECL_ASSEMBLER_NAME (node->decl);
        c_common_write_pch ();
        dump_tu ();
        return;
Index: gcc/cp/cp-tree.h
===================================================================
*** gcc/cp/cp-tree.h	(revision 227779)
--- gcc/cp/cp-tree.h	(working copy)
*************** extern tree finish_qualified_id_expr		(t
*** 6253,6259 ****
  						 bool, bool, tsubst_flags_t);
  extern void simplify_aggr_init_expr		(tree *);
  extern void finalize_nrv			(tree *, tree, tree);
- extern void note_decl_for_pch			(tree);
  extern tree omp_reduction_id			(enum tree_code, tree, tree);
  extern tree cp_remove_omp_priv_cleanup_stmt	(tree *, int *, void *);
  extern void cp_check_omp_declare_reduction	(tree);
--- 6253,6258 ----

Re: [PATCH][C++] Avoid PCH dependent mangling

[Jason Merrill]

OK.

Jason