[PATCH] Add verifier for leaked SSA names

[PATCH] Add verifier for leaked SSA names

[Richard Biener]

The following patch doesn't pass bootstrap & regtest.  It did at some
point though and its comment hints that fixing leaks after inlining
was too interesting a problem to solve ;)

Thus patch is FYI.

Richard.

Index: tree-ssa.c
===================================================================
--- tree-ssa.c  (revision 228320)
+++ tree-ssa.c  (working copy)
@@ -693,6 +693,16 @@ verify_def (basic_block bb, basic_block
       goto err;
     }
 
+  if (bb == NULL
+      /* ???  Too many latent cases in the main opt pipeline.  But it's
+         worth to fix all cases before inlining as that reduces the
+        amount of garbage kept live.  */
+      && !cfun->after_inlining)
+    {
+      error ("removed STMT failed to release SSA name");
+      goto err;
+    }
+
   if (definition_block[SSA_NAME_VERSION (ssa_name)])
     {
       error ("SSA_NAME created in two different blocks %i and %i",

Re: [PATCH] Add verifier for leaked SSA names

[Jeff Law]

On 10/02/2015 01:37 AM, Richard Biener wrote:
>
> The following patch doesn't pass bootstrap & regtest.  It did at some
> point though and its comment hints that fixing leaks after inlining
> was too interesting a problem to solve ;)
>
> Thus patch is FYI.
>
> Richard.
>
> Index: tree-ssa.c
> ===================================================================
> --- tree-ssa.c  (revision 228320)
> +++ tree-ssa.c  (working copy)
> @@ -693,6 +693,16 @@ verify_def (basic_block bb, basic_block
>         goto err;
>       }
>
> +  if (bb == NULL
> +      /* ???  Too many latent cases in the main opt pipeline.  But it's
> +         worth to fix all cases before inlining as that reduces the
> +        amount of garbage kept live.  */
> +      && !cfun->after_inlining)
> +    {
> +      error ("removed STMT failed to release SSA name");
> +      goto err;
> +    }
> +
I was building the verification step into the ssa name manager. 
Essentially at the point where we flush from the pending to the free 
list, we should have a consistent state.

Thus we ought to be able to walk the IL marking everything we can see, 
combine that with the contents of the freelist and the result ought to 
be every SSA_NAME ever created.

Reality is somewhat different, of course.

Yours takes a slightly different approach.  Ultimately if we get the 
leaks plugged, we might even consider using both.

jeff

Re: [PATCH] Add verifier for leaked SSA names

[Richard Biener]

On Fri, Oct 2, 2015 at 5:24 PM, Jeff Law <law@redhat.com> wrote:
> On 10/02/2015 01:37 AM, Richard Biener wrote:
>>
>>
>> The following patch doesn't pass bootstrap & regtest.  It did at some
>> point though and its comment hints that fixing leaks after inlining
>> was too interesting a problem to solve ;)
>>
>> Thus patch is FYI.
>>
>> Richard.
>>
>> Index: tree-ssa.c
>> ===================================================================
>> --- tree-ssa.c  (revision 228320)
>> +++ tree-ssa.c  (working copy)
>> @@ -693,6 +693,16 @@ verify_def (basic_block bb, basic_block
>>         goto err;
>>       }
>>
>> +  if (bb == NULL
>> +      /* ???  Too many latent cases in the main opt pipeline.  But it's
>> +         worth to fix all cases before inlining as that reduces the
>> +        amount of garbage kept live.  */
>> +      && !cfun->after_inlining)
>> +    {
>> +      error ("removed STMT failed to release SSA name");
>> +      goto err;
>> +    }
>> +
>
> I was building the verification step into the ssa name manager. Essentially
> at the point where we flush from the pending to the free list, we should
> have a consistent state.

Yeah, though when SSA verifiers run the state should also be consistent
and we'd get to pinpoint the offending pass easier.

> Thus we ought to be able to walk the IL marking everything we can see,
> combine that with the contents of the freelist and the result ought to be
> every SSA_NAME ever created.
>
> Reality is somewhat different, of course.
>
> Yours takes a slightly different approach.  Ultimately if we get the leaks
> plugged, we might even consider using both.

Sure.  Note that the above is from simply walking all SSA names.

Richard.

> jeff
>
>
>

Re: [PATCH] Add verifier for leaked SSA names

[Jeff Law]

On 10/05/2015 02:56 AM, Richard Biener wrote:
>> I was building the verification step into the ssa name manager. Essentially
>> at the point where we flush from the pending to the free list, we should
>> have a consistent state.
>
> Yeah, though when SSA verifiers run the state should also be consistent
> and we'd get to pinpoint the offending pass easier.
Agreed.

>
>> Thus we ought to be able to walk the IL marking everything we can see,
>> combine that with the contents of the freelist and the result ought to be
>> every SSA_NAME ever created.
>>
>> Reality is somewhat different, of course.
>>
>> Yours takes a slightly different approach.  Ultimately if we get the leaks
>> plugged, we might even consider using both.
>
> Sure.  Note that the above is from simply walking all SSA names.
Right.  That's precisely what I was referring to.  Yours walks the 
SSA_NAMEs and declares those with an empty block for the defining 
statement as leaks.

Mine walks the IL and declares any name that was allocated, but not 
found in the IL as a leak.

Mine is obviously more expensive, but possibly catches more leaks.

The one conclusion I did come to over the weekend is that without a 
recycle immediate policy, there's no value in explicitly releasing the 
names.  ie, we could just drop all the explicit management and rely on 
garbage collection at safe points.

If we added either a mode to allow immediate recycling or an adaptive 
behaviour in the manager to recycle immediately until it wasn't safe to 
do so, then there's obviously value in the explicit releases and 
plugging  the leaks.

jeff