Re: [PATCH] Fix debuginfo in -fopenmp code (PR debug/87039)

[Richard Biener <rguenther at suse dot de>]

On Sat, 17 Nov 2018, Jakub Jelinek wrote:

> On Sat, Nov 17, 2018 at 05:51:05PM +0100, Richard Biener wrote:
> > On November 17, 2018 4:14:58 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
> > >On Sat, Nov 17, 2018 at 08:31:32AM +0100, Richard Biener wrote:
> > >> On November 16, 2018 10:10:00 PM GMT+01:00, Jakub Jelinek
> > >> <jakub@redhat.com> wrote: Can you add a comment why doing it
> > >differently
> > >> for this case is necessary or do it the same way in all cases?
> > >
> > >Do you mean in omp-expand.c or dwarf2out.c?  I believe I'm doing it the
> > >same
> > 
> > I meant in dwarf2out.c.  IIRC we have multiple calls into dwarf2out_decl and you adjust only one? 
> 
> We don't need to change the case where decl isn't a FUNCTION_DECL,
> and must not change the case where we call dwarf2out_decl (decl), that would
> lead to infinite recursion.
> 
> The only case might be replace
>               current_function_decl = origin;
>               dwarf2out_decl (origin);
> with the dwarf2out_early_global_decl (origin); call, not really sure if that
> is needed though.  Do we ever have functions where
> decl_function_context (decl) != decl_function_context (DECL_ABSTRACT_ORIGIN (decl))
> ?
> 
> The other possibility is to move this decl_function_context handling code
> from dwarf2out_early_global_decl to dwarf2out_decl, guarded with 
>   if (early_dwarf
>       && decl_function_context (decl)))
>    ...
> 
> Something like (completely untested):
> 
> --- gcc/dwarf2out.c.jj	2018-11-16 17:33:42.899215778 +0100
> +++ gcc/dwarf2out.c	2018-11-17 20:11:26.847301806 +0100
> @@ -26390,22 +26390,6 @@ dwarf2out_early_global_decl (tree decl)
>  	{
>  	  tree save_fndecl = current_function_decl;
>  
> -	  /* For nested functions, make sure we have DIEs for the parents first
> -	     so that all nested DIEs are generated at the proper scope in the
> -	     first shot.  */
> -	  tree context = decl_function_context (decl);
> -	  if (context != NULL)
> -	    {
> -	      dw_die_ref context_die = lookup_decl_die (context);
> -	      current_function_decl = context;
> -
> -	      /* Avoid emitting DIEs multiple times, but still process CONTEXT
> -		 enough so that it lands in its own context.  This avoids type
> -		 pruning issues later on.  */
> -	      if (context_die == NULL || is_declaration_die (context_die))
> -		dwarf2out_decl (context);
> -	    }
> -
>  	  /* Emit an abstract origin of a function first.  This happens
>  	     with C++ constructor clones for example and makes
>  	     dwarf2out_abstract_function happy which requires the early
> @@ -26716,11 +26700,31 @@ dwarf2out_decl (tree decl)
>  	 we're a method, it will be ignored, since we already have a DIE.
>  	 Avoid doing this late though since clones of class methods may
>  	 otherwise end up in limbo and create type DIEs late.  */
> -      if (early_dwarf
> -	  && decl_function_context (decl)
> -	  /* But if we're in terse mode, we don't care about scope.  */
> -	  && debug_info_level > DINFO_LEVEL_TERSE)
> -	context_die = NULL;
> +      if (early_dwarf)
> +	{
> +	  /* For nested functions, make sure we have DIEs for the parents first
> +	     so that all nested DIEs are generated at the proper scope in the
> +	     first shot.  */
> +	  tree context = decl_function_context (decl);
> +	  if (context != NULL)
> +	    {
> +	      dw_die_ref ctx_die = lookup_decl_die (context);
> +	      tree save_fndecl = current_function_decl;
> +	      current_function_decl = context;
> +
> +	      /* Avoid emitting DIEs multiple times, but still process CONTEXT
> +		 enough so that it lands in its own context.  This avoids type
> +		 pruning issues later on.  */
> +	      if (ctx_die == NULL || is_declaration_die (ctx_die))
> +		dwarf2out_decl (context);
> +
> +	      current_function_decl = save_fndecl;
> +
> +	      /* But if we're in terse mode, we don't care about scope.  */
> +	      if (debug_info_level > DINFO_LEVEL_TERSE)
> +		context_die = NULL;
> +	    }
> +	}

Ugh, I'd like to keep the ordering "hacks" in dwarf2out_early_global_decl.

Reviewing the original code now the only case we need the recursion
is that of your original change (though the recursion shouldn't
be necessary for the is_declaration_die case).

Thus the original dwarf2out.c hunk is OK.

Thanks and sorry for the confusion coming from reviewing patches
from a mobile phone w/o SVN access...

Richard.


>        break;
>  
>      case VAR_DECL:
> 
> 
> 	Jakub
> 
> 

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