Re: [PATCH][3/n] dwarf2out refactoring for early (LTO) debug

[Jason Merrill]

Looks good.

On Wed, Sep 21, 2016 at 4:37 AM, Richard Biener
 wrote:
> On Wed, Aug 26, 2015 at 1:34 PM, Richard Biener  wrote:
>>
>> The following fixes a GC issue I run into when doing
>> prune_unused_types_prune early.  The issue is that the DIE struct
>> has a chain_circular marked field (die_sib) which cannot tolerate
>> spurious extra entries from old removed entries into the circular
>> chain.  Otherwise we fail to properly mark parts of the chain.
>> Those stray entries are kept live referenced from TYPE_SYMTAB_DIE.
>>
>> So the following patch that makes sure to clear ->die_sib for
>> nodes we remove.  (these DIEs remaining in TYPE_SYMTAB_DIE also
>> means we may end up re-using them which is probably not what we
>> want ... in the original LTO experiment I had a ->removed flag
>> in the DIE struct and removed DIEs from the cache at cache lookup
>> time if I hit a removed DIE)
>>
>> Bootstrapped and tested on x86_64-unknown-linux-gnu, gdb tested there
>> as well.
>>
>> Ok for trunk?
>
> I am re-testing this now and will commit it as part of the piecewise early LTO
> merge.
>
> Richard.
>
>> Thanks,
>> Richard.
>>
>> 2015-08-26  Richard Biener  
>>
>> * dwarf2out.c (remove_child_with_prev): Clear child->die_sib.
>> (replace_child): Likewise.
>> (remove_child_TAG): Adjust.
>> (move_marked_base_types): Likewise.
>> (prune_unused_types_prune): Clear die_sib of removed children.
>>
>> Index: trunk/gcc/dwarf2out.c
>> ===
>> --- trunk.orig/gcc/dwarf2out.c  2015-08-26 09:30:54.679185817 +0200
>> +++ trunk/gcc/dwarf2out.c   2015-08-25 16:54:09.150506037 +0200
>> @@ -4827,6 +4827,7 @@ remove_child_with_prev (dw_die_ref child
>>  prev->die_sib = child->die_sib;
>>if (child->die_parent->die_child == child)
>>  child->die_parent->die_child = prev;
>> +  child->die_sib = NULL;
>>  }
>>
>>  /* Replace OLD_CHILD with NEW_CHILD.  PREV must have the property that
>> @@ -4853,6 +4854,7 @@ replace_child (dw_die_ref old_child, dw_
>>  }
>>if (old_child->die_parent->die_child == old_child)
>>  old_child->die_parent->die_child = new_child;
>> +  old_child->die_sib = NULL;
>>  }
>>
>>  /* Move all children from OLD_PARENT to NEW_PARENT.  */
>> @@ -4883,9 +4885,9 @@ remove_child_TAG (dw_die_ref die, enum d
>> remove_child_with_prev (c, prev);
>> c->die_parent = NULL;
>> /* Might have removed every child.  */
>> -   if (c == c->die_sib)
>> +   if (die->die_child == NULL)
>>   return;
>> -   c = c->die_sib;
>> +   c = prev->die_sib;
>>}
>>} while (c != die->die_child);
>>  }
>> @@ -24565,8 +24590,8 @@ prune_unused_types_prune (dw_die_ref die
>>
>>c = die->die_child;
>>do {
>> -dw_die_ref prev = c;
>> -for (c = c->die_sib; ! c->die_mark; c = c->die_sib)
>> +dw_die_ref prev = c, next;
>> +for (c = c->die_sib; ! c->die_mark; c = next)
>>if (c == die->die_child)
>> {
>>   /* No marked children between 'prev' and the end of the list.  */
>> @@ -24578,8 +24603,14 @@ prune_unused_types_prune (dw_die_ref die
>>   prev->die_sib = c->die_sib;
>>   die->die_child = prev;
>> }
>> + c->die_sib = NULL;
>>   return;
>> }
>> +  else
>> +   {
>> + next = c->die_sib;
>> + c->die_sib = NULL;
>> +   }
>>
>>  if (c != prev->die_sib)
>>prev->die_sib = c;
>> @@ -24824,8 +24855,8 @@ move_marked_base_types (void)
>>   remove_child_with_prev (c, prev);
>>   /* As base types got marked, there must be at least
>>  one node other than DW_TAG_base_type.  */
>> - gcc_assert (c != c->die_sib);
>> - c = c->die_sib;
>> + gcc_assert (die->die_child != NULL);
>> + c = prev->die_sib;
>> }
>>  }
>>while (c != die->die_child);

Re: [PATCH][3/n] dwarf2out refactoring for early (LTO) debug

[Richard Biener]

On Wed, Aug 26, 2015 at 1:34 PM, Richard Biener  wrote:
>
> The following fixes a GC issue I run into when doing
> prune_unused_types_prune early.  The issue is that the DIE struct
> has a chain_circular marked field (die_sib) which cannot tolerate
> spurious extra entries from old removed entries into the circular
> chain.  Otherwise we fail to properly mark parts of the chain.
> Those stray entries are kept live referenced from TYPE_SYMTAB_DIE.
>
> So the following patch that makes sure to clear ->die_sib for
> nodes we remove.  (these DIEs remaining in TYPE_SYMTAB_DIE also
> means we may end up re-using them which is probably not what we
> want ... in the original LTO experiment I had a ->removed flag
> in the DIE struct and removed DIEs from the cache at cache lookup
> time if I hit a removed DIE)
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, gdb tested there
> as well.
>
> Ok for trunk?

I am re-testing this now and will commit it as part of the piecewise early LTO
merge.

Richard.

> Thanks,
> Richard.
>
> 2015-08-26  Richard Biener  
>
> * dwarf2out.c (remove_child_with_prev): Clear child->die_sib.
> (replace_child): Likewise.
> (remove_child_TAG): Adjust.
> (move_marked_base_types): Likewise.
> (prune_unused_types_prune): Clear die_sib of removed children.
>
> Index: trunk/gcc/dwarf2out.c
> ===
> --- trunk.orig/gcc/dwarf2out.c  2015-08-26 09:30:54.679185817 +0200
> +++ trunk/gcc/dwarf2out.c   2015-08-25 16:54:09.150506037 +0200
> @@ -4827,6 +4827,7 @@ remove_child_with_prev (dw_die_ref child
>  prev->die_sib = child->die_sib;
>if (child->die_parent->die_child == child)
>  child->die_parent->die_child = prev;
> +  child->die_sib = NULL;
>  }
>
>  /* Replace OLD_CHILD with NEW_CHILD.  PREV must have the property that
> @@ -4853,6 +4854,7 @@ replace_child (dw_die_ref old_child, dw_
>  }
>if (old_child->die_parent->die_child == old_child)
>  old_child->die_parent->die_child = new_child;
> +  old_child->die_sib = NULL;
>  }
>
>  /* Move all children from OLD_PARENT to NEW_PARENT.  */
> @@ -4883,9 +4885,9 @@ remove_child_TAG (dw_die_ref die, enum d
> remove_child_with_prev (c, prev);
> c->die_parent = NULL;
> /* Might have removed every child.  */
> -   if (c == c->die_sib)
> +   if (die->die_child == NULL)
>   return;
> -   c = c->die_sib;
> +   c = prev->die_sib;
>}
>} while (c != die->die_child);
>  }
> @@ -24565,8 +24590,8 @@ prune_unused_types_prune (dw_die_ref die
>
>c = die->die_child;
>do {
> -dw_die_ref prev = c;
> -for (c = c->die_sib; ! c->die_mark; c = c->die_sib)
> +dw_die_ref prev = c, next;
> +for (c = c->die_sib; ! c->die_mark; c = next)
>if (c == die->die_child)
> {
>   /* No marked children between 'prev' and the end of the list.  */
> @@ -24578,8 +24603,14 @@ prune_unused_types_prune (dw_die_ref die
>   prev->die_sib = c->die_sib;
>   die->die_child = prev;
> }
> + c->die_sib = NULL;
>   return;
> }
> +  else
> +   {
> + next = c->die_sib;
> + c->die_sib = NULL;
> +   }
>
>  if (c != prev->die_sib)
>prev->die_sib = c;
> @@ -24824,8 +24855,8 @@ move_marked_base_types (void)
>   remove_child_with_prev (c, prev);
>   /* As base types got marked, there must be at least
>  one node other than DW_TAG_base_type.  */
> - gcc_assert (c != c->die_sib);
> - c = c->die_sib;
> + gcc_assert (die->die_child != NULL);
> + c = prev->die_sib;
> }
>  }
>while (c != die->die_child);

[PATCH][3/n] dwarf2out refactoring for early (LTO) debug

[Richard Biener]

The following fixes a GC issue I run into when doing 
prune_unused_types_prune early.  The issue is that the DIE struct
has a chain_circular marked field (die_sib) which cannot tolerate
spurious extra entries from old removed entries into the circular
chain.  Otherwise we fail to properly mark parts of the chain.
Those stray entries are kept live referenced from TYPE_SYMTAB_DIE.

So the following patch that makes sure to clear -die_sib for
nodes we remove.  (these DIEs remaining in TYPE_SYMTAB_DIE also
means we may end up re-using them which is probably not what we
want ... in the original LTO experiment I had a -removed flag
in the DIE struct and removed DIEs from the cache at cache lookup
time if I hit a removed DIE)

Bootstrapped and tested on x86_64-unknown-linux-gnu, gdb tested there
as well.

Ok for trunk?

Thanks,
Richard.

2015-08-26  Richard Biener  rguent...@suse.de

* dwarf2out.c (remove_child_with_prev): Clear child-die_sib.
(replace_child): Likewise.
(remove_child_TAG): Adjust.
(move_marked_base_types): Likewise.
(prune_unused_types_prune): Clear die_sib of removed children.

Index: trunk/gcc/dwarf2out.c
===
--- trunk.orig/gcc/dwarf2out.c  2015-08-26 09:30:54.679185817 +0200
+++ trunk/gcc/dwarf2out.c   2015-08-25 16:54:09.150506037 +0200
@@ -4827,6 +4827,7 @@ remove_child_with_prev (dw_die_ref child
 prev-die_sib = child-die_sib;
   if (child-die_parent-die_child == child)
 child-die_parent-die_child = prev;
+  child-die_sib = NULL;
 }
 
 /* Replace OLD_CHILD with NEW_CHILD.  PREV must have the property that
@@ -4853,6 +4854,7 @@ replace_child (dw_die_ref old_child, dw_
 }
   if (old_child-die_parent-die_child == old_child)
 old_child-die_parent-die_child = new_child;
+  old_child-die_sib = NULL;
 }
 
 /* Move all children from OLD_PARENT to NEW_PARENT.  */
@@ -4883,9 +4885,9 @@ remove_child_TAG (dw_die_ref die, enum d
remove_child_with_prev (c, prev);
c-die_parent = NULL;
/* Might have removed every child.  */
-   if (c == c-die_sib)
+   if (die-die_child == NULL)
  return;
-   c = c-die_sib;
+   c = prev-die_sib;
   }
   } while (c != die-die_child);
 }
@@ -24565,8 +24590,8 @@ prune_unused_types_prune (dw_die_ref die
 
   c = die-die_child;
   do {
-dw_die_ref prev = c;
-for (c = c-die_sib; ! c-die_mark; c = c-die_sib)
+dw_die_ref prev = c, next;
+for (c = c-die_sib; ! c-die_mark; c = next)
   if (c == die-die_child)
{
  /* No marked children between 'prev' and the end of the list.  */
@@ -24578,8 +24603,14 @@ prune_unused_types_prune (dw_die_ref die
  prev-die_sib = c-die_sib;
  die-die_child = prev;
}
+ c-die_sib = NULL;
  return;
}
+  else
+   {
+ next = c-die_sib;
+ c-die_sib = NULL;
+   }
 
 if (c != prev-die_sib)
   prev-die_sib = c;
@@ -24824,8 +24855,8 @@ move_marked_base_types (void)
  remove_child_with_prev (c, prev);
  /* As base types got marked, there must be at least
 one node other than DW_TAG_base_type.  */
- gcc_assert (c != c-die_sib);
- c = c-die_sib;
+ gcc_assert (die-die_child != NULL);
+ c = prev-die_sib;
}
 }
   while (c != die-die_child);