[PATCH] Fix PR85020

[PATCH] Fix PR85020

[Richard Biener]

The following fixes profiledbootstrap with LTO for Ada where we
end up with references to later optimized out decls in the early debug
parts.

The fix is to avoid running into

  /* Try harder to get a rtl.  If this symbol ends up not being emitted
     in the current CU, resolve_addr will remove the expression 
referencing
     it.  */
  if (rtl == NULL_RTX
...
      rtl = make_decl_rtl_for_debug (decl);

in rtl_for_decl_location as the comment already says the symbol can
end up not being emitted which is fatal for early debug.  For now
this is conditionalized on LTO emission.

This should then trigger the use of DW_OP_GNU_variable_value but
for the Ada ACATS testcase I was looking at to debug the issue
(c330001, resp. c330001_0-c330001_1.ads) this doesn't work
because the constraints are not met (we're in global context and
the upper bound decl is global as well).  Jakub, do you remember
a reason for having any constraints here?  I've reworked the
constraints to allow the globals (eventually using decl_function_context
on this may also allow other not-quite-local cases?  But it already
allowed local statics).  Maybe we can simply drop the restriction
alltogether?

Bootstrap and regtest running on x86_64-unknown-linux-gnu,
profiledbootstrap with LTO on x86_64-unknown-linux-gnu.

Eric, can you generate a gnat.dg LTO testcase from (reduced) c330001?

OK for trunk?

Thanks,
Richard.

2018-03-22  Richard Biener  <rguenther@suse.de>

	PR debug/85020
	* dwarf2out.c (loc_list_from_tree_1): Relax restriction on
	what decls we refer to via DW_OP_GNU_variable_value, respective
	allow globals from global context.
	(rtl_for_decl_location): Do not generate RTL early when
	we are going to emit early debug for LTO.

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 258720)
+++ gcc/dwarf2out.c	(working copy)
@@ -18204,14 +18204,15 @@ loc_list_from_tree_1 (tree loc, int want
 	rtl = rtl_for_decl_location (loc);
 	if (rtl == NULL_RTX)
 	  {
+	    tree fn_ctx;
 	    if (TREE_CODE (loc) != FUNCTION_DECL
 		&& early_dwarf
-		&& current_function_decl
 		&& want_address != 1
 		&& ! DECL_IGNORED_P (loc)
 		&& (INTEGRAL_TYPE_P (TREE_TYPE (loc))
 		    || POINTER_TYPE_P (TREE_TYPE (loc)))
-		&& DECL_CONTEXT (loc) == current_function_decl
+		&& (!(fn_ctx = decl_function_context (loc))
+		    || fn_ctx == current_function_decl)
 		&& (GET_MODE_SIZE (SCALAR_INT_TYPE_MODE (TREE_TYPE (loc)))
 		    <= DWARF2_ADDR_SIZE))
 	      {
@@ -19878,6 +19879,7 @@ rtl_for_decl_location (tree decl)
      in the current CU, resolve_addr will remove the expression referencing
      it.  */
   if (rtl == NULL_RTX
+      && !(early_dwarf && (flag_generate_lto || flag_generate_offload))
       && VAR_P (decl)
       && !DECL_EXTERNAL (decl)
       && TREE_STATIC (decl)

Re: [PATCH] Fix PR85020

[Jakub Jelinek]

On Thu, Mar 22, 2018 at 03:16:22PM +0100, Richard Biener wrote:
> the upper bound decl is global as well).  Jakub, do you remember
> a reason for having any constraints here?  I've reworked the

I don't, but it has been added in the
https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01499.html
patch.  It matches what we were using before:
-	      if (loc == NULL
-		  && early_dwarf
-		  && current_function_decl
-		  && DECL_CONTEXT (rszdecl) == current_function_decl)
when emitting the DW_OP_call4 stuff.

But more importantly, it seems that resolve_variable_value_in_expr
will not change anything if
      tree decl = loc->dw_loc_oprnd1.v.val_decl_ref;
      if (DECL_CONTEXT (decl) != current_function_decl)
        continue;
Which means resolve_variable_values would never process those
DW_OP_GNU_variable_value you've added, and I believe later on
note_variable_value_in_expr will just ICE on it:
    if (loc->dw_loc_opc == DW_OP_GNU_variable_value
        && loc->dw_loc_oprnd1.val_class == dw_val_class_decl_ref)
...
        if (! ref && (flag_generate_lto || flag_generate_offload))
          {
            /* ???  This is somewhat a hack because we do not create DIEs
               for variables not in BLOCK trees early but when generating
               early LTO output we need the dw_val_class_decl_ref to be
               fully resolved.  For fat LTO objects we'd also like to
               undo this after LTO dwarf output.  */
            gcc_assert (DECL_CONTEXT (decl));
Because DECL_CONTEXT (decl) is NULL, right?

Or is DECL_CONTEXT never NULL in LTO, just TRANSLATION_UNIT_DECL?
If so, perhaps note_variable_value_in_expr will handle those fine,
but for these we really don't want to force any DIEs, but rather just give
up if lookup_decl fails by the time we've processed the whole TU.

	Jakub

Re: [PATCH] Fix PR85020

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

> This should then trigger the use of DW_OP_GNU_variable_value but
> for the Ada ACATS testcase I was looking at to debug the issue
> (c330001, resp. c330001_0-c330001_1.ads) this doesn't work
> because the constraints are not met (we're in global context and
> the upper bound decl is global as well).  Jakub, do you remember
> a reason for having any constraints here?  I've reworked the
> constraints to allow the globals (eventually using decl_function_context
> on this may also allow other not-quite-local cases?  But it already
> allowed local statics).  Maybe we can simply drop the restriction
> alltogether?

Thanks for looking into this.

> Eric, can you generate a gnat.dg LTO testcase from (reduced) c330001?

Attached.


	* gnat.dg/lto22.adb: New test.
	* gnat.dg/lto22_pkg1.ad[sb]: New helper.
	* gnat.dg/lto22_pkg2.ads: Likewise.

-- 
Eric Botcazou

[-- Attachment #2: lto22_pkg1.ads --]
[-- Type: text/x-adasrc, Size: 100 bytes --]

with Lto22_Pkg2; use Lto22_Pkg2;

package Lto22_Pkg1 is  

   Public_1 : Rec := F;

end Lto22_Pkg1;

[-- Attachment #3: lto22_pkg2.adb --]
[-- Type: text/x-adasrc, Size: 131 bytes --]

package body Lto22_Pkg2 is  

   function F return Rec is
      Var_1 : Rec;
   begin
      return Var_1;
   end;

end Lto22_Pkg2;

[-- Attachment #4: lto22_pkg2.ads --]
[-- Type: text/x-adasrc, Size: 267 bytes --]

package Lto22_Pkg2 is  

   subtype Index_Type is Integer range 1 .. 20;

   type Rec (<>) is private;           

   function F return Rec;

private

   type Rec (D : Index_Type := 2) is record     
      S : String (1 .. D) := "Hi";
   end record;

end Lto22_Pkg2;

[-- Attachment #5: lto22.adb --]
[-- Type: text/x-adasrc, Size: 120 bytes --]

-- { dg-do run }
-- { dg-options "-g -flto" { target lto } }

with Lto22_Pkg1;

procedure Lto22 is  
begin
  null;
end;

Re: [PATCH] Fix PR85020

[Richard Biener]

On Thu, 22 Mar 2018, Jakub Jelinek wrote:

> On Thu, Mar 22, 2018 at 03:16:22PM +0100, Richard Biener wrote:
> > the upper bound decl is global as well).  Jakub, do you remember
> > a reason for having any constraints here?  I've reworked the
> 
> I don't, but it has been added in the
> https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01499.html
> patch.  It matches what we were using before:
> -	      if (loc == NULL
> -		  && early_dwarf
> -		  && current_function_decl
> -		  && DECL_CONTEXT (rszdecl) == current_function_decl)
> when emitting the DW_OP_call4 stuff.
> 
> But more importantly, it seems that resolve_variable_value_in_expr
> will not change anything if
>       tree decl = loc->dw_loc_oprnd1.v.val_decl_ref;
>       if (DECL_CONTEXT (decl) != current_function_decl)
>         continue;
> Which means resolve_variable_values would never process those
> DW_OP_GNU_variable_value you've added,

Ah, ok.  I guess this check can be simply removed.

> and I believe later on
> note_variable_value_in_expr will just ICE on it:
>     if (loc->dw_loc_opc == DW_OP_GNU_variable_value
>         && loc->dw_loc_oprnd1.val_class == dw_val_class_decl_ref)
> ...
>         if (! ref && (flag_generate_lto || flag_generate_offload))
>           {
>             /* ???  This is somewhat a hack because we do not create DIEs
>                for variables not in BLOCK trees early but when generating
>                early LTO output we need the dw_val_class_decl_ref to be
>                fully resolved.  For fat LTO objects we'd also like to
>                undo this after LTO dwarf output.  */
>             gcc_assert (DECL_CONTEXT (decl));
> Because DECL_CONTEXT (decl) is NULL, right?

It's TRANSLATION_UNIT_DECL for the Ada case (we do have FEs where
decls may have NULL context which then means file-scope).

> Or is DECL_CONTEXT never NULL in LTO, just TRANSLATION_UNIT_DECL?
> If so, perhaps note_variable_value_in_expr will handle those fine,
> but for these we really don't want to force any DIEs, but rather just give
> up if lookup_decl fails by the time we've processed the whole TU.

Yeah, I guess the whole ! ref case should go and instead drop the
location expression.  Is there some convenient "NULL" OP we can
use for <optimized out> or do we really have to prune the
attribute refering to it?  As the comment says for the FAT part
we'd like to keep the decl and defer for later fixup so if there's
a <optimized out> way to emit DWARF for a val_decl_ref we should
maybe simply fix it up at emission time?  I see there's
DW_OP_nop but I guess a nop in random places of a DWARF location
expression isn't a good way to say <optimized out> for that part.

That said, the second hunk of my patch fixes the bootstrap issue
with Ada (will double-check), the first hunk is only to try preserving
some debug info for the Ada case where the variable parts are not
necessarily function-local.  Let's delay that for now.

I suppose the 2nd hunk is ok.  For reference that was

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c     (revision 258684)
+++ gcc/dwarf2out.c     (working copy)
@@ -19878,6 +19878,7 @@ rtl_for_decl_location (tree decl)
      in the current CU, resolve_addr will remove the expression 
referencing
      it.  */
   if (rtl == NULL_RTX
+      && !(early_dwarf && (flag_generate_lto || flag_generate_offload))
       && VAR_P (decl)
       && !DECL_EXTERNAL (decl)
       && TREE_STATIC (decl)


Richard.

Re: [PATCH] Fix PR85020

[Jakub Jelinek]

On Fri, Mar 23, 2018 at 10:58:15AM +0100, Richard Biener wrote:
> On Thu, 22 Mar 2018, Jakub Jelinek wrote:
> 
> > On Thu, Mar 22, 2018 at 03:16:22PM +0100, Richard Biener wrote:
> > > the upper bound decl is global as well).  Jakub, do you remember
> > > a reason for having any constraints here?  I've reworked the
> > 
> > I don't, but it has been added in the
> > https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01499.html
> > patch.  It matches what we were using before:
> > -	      if (loc == NULL
> > -		  && early_dwarf
> > -		  && current_function_decl
> > -		  && DECL_CONTEXT (rszdecl) == current_function_decl)
> > when emitting the DW_OP_call4 stuff.
> > 
> > But more importantly, it seems that resolve_variable_value_in_expr
> > will not change anything if
> >       tree decl = loc->dw_loc_oprnd1.v.val_decl_ref;
> >       if (DECL_CONTEXT (decl) != current_function_decl)
> >         continue;
> > Which means resolve_variable_values would never process those
> > DW_OP_GNU_variable_value you've added,
> 
> Ah, ok.  I guess this check can be simply removed.

Actually it should be kept.
resolve_variable_values works on expressions that were added to the hash
table by the note_variable_value_in_expr function, and there a hash table
entry for each containing function and we want to process it only when
doing resolve_variable_values for the particular function e.g. if some
expression happens to be referenced from multiple hash tables.

> > and I believe later on
> > note_variable_value_in_expr will just ICE on it:
> >     if (loc->dw_loc_opc == DW_OP_GNU_variable_value
> >         && loc->dw_loc_oprnd1.val_class == dw_val_class_decl_ref)
> > ...
> >         if (! ref && (flag_generate_lto || flag_generate_offload))
> >           {
> >             /* ???  This is somewhat a hack because we do not create DIEs
> >                for variables not in BLOCK trees early but when generating
> >                early LTO output we need the dw_val_class_decl_ref to be
> >                fully resolved.  For fat LTO objects we'd also like to
> >                undo this after LTO dwarf output.  */
> >             gcc_assert (DECL_CONTEXT (decl));
> > Because DECL_CONTEXT (decl) is NULL, right?
> 
> It's TRANSLATION_UNIT_DECL for the Ada case (we do have FEs where
> decls may have NULL context which then means file-scope).

Anyway, the above code in note_variable_value_in_expr for -flto will ensure
nothing is ever added into variable_value_hash and thus resolve_variable_values
isn't reall invoked in that case.

> Yeah, I guess the whole ! ref case should go and instead drop the
> location expression.  Is there some convenient "NULL" OP we can

There is none, except newly DW_OP_GNU_variable_value referencing a DIE
(say DW_TAG_dwarf_procedure with no DW_AT_location/DW_AT_const_value)
- that is something that never has a usable value, so it is always optimized
out.  That said, resolve_addr_in_expr should handle the removal of
expressions containing DW_OP_GNU_variable_value with dw_val_class_decl_ref
if lookup_decl_die fails.

> I suppose the 2nd hunk is ok.  For reference that was
> 
> Index: gcc/dwarf2out.c
> ===================================================================
> --- gcc/dwarf2out.c     (revision 258684)
> +++ gcc/dwarf2out.c     (working copy)
> @@ -19878,6 +19878,7 @@ rtl_for_decl_location (tree decl)
>       in the current CU, resolve_addr will remove the expression 
> referencing
>       it.  */
>    if (rtl == NULL_RTX
> +      && !(early_dwarf && (flag_generate_lto || flag_generate_offload))
>        && VAR_P (decl)
>        && !DECL_EXTERNAL (decl)
>        && TREE_STATIC (decl)

Sure, this is obviously ok.

	Jakub

Re: [PATCH] Fix PR85020

[Richard Biener]

On Fri, 23 Mar 2018, Eric Botcazou wrote:

> > This should then trigger the use of DW_OP_GNU_variable_value but
> > for the Ada ACATS testcase I was looking at to debug the issue
> > (c330001, resp. c330001_0-c330001_1.ads) this doesn't work
> > because the constraints are not met (we're in global context and
> > the upper bound decl is global as well).  Jakub, do you remember
> > a reason for having any constraints here?  I've reworked the
> > constraints to allow the globals (eventually using decl_function_context
> > on this may also allow other not-quite-local cases?  But it already
> > allowed local statics).  Maybe we can simply drop the restriction
> > alltogether?
> 
> Thanks for looking into this.
> 
> > Eric, can you generate a gnat.dg LTO testcase from (reduced) c330001?
> 
> Attached.
> 
> 
> 	* gnat.dg/lto22.adb: New test.
> 	* gnat.dg/lto22_pkg1.ad[sb]: New helper.
> 	* gnat.dg/lto22_pkg2.ads: Likewise.

Thanks, committed.

Richard.