[llvm-commits] [PATCH] Avoid use after free in ScalarEvolution

[Dan Gohman]

On Jun 28, 2009, at 6:27 AM, Török Edwin wrote:


> On 2009-06-16 10:19, Török Edwin wrote:
>
>> On 2009-06-15 23:23, Dan Gohman wrote:
>>
>>
>>
>>> On Jun 15, 2009, at 11:43 AM, Török Edwin wrote:
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>> On 2009-06-15 21:15, Dan Gohman wrote:
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> Hi Edwin,
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> WritingAnLLVMPass.htm's description of releaseMemory says "This
>>>>>
>>>>> method
>>>>>
>>>>>
>>>>>
>>>>> is
>>>>>
>>>>>
>>>>>
>>>>> called after the run* method for the class, before the next call  
>>>>> of
>>>>>
>>>>> run*
>>>>>
>>>>>
>>>>>
>>>>> in your pass."  This suggests that it's a bug in the PassManager
>>>>>
>>>>> for not
>>>>>
>>>>>
>>>>>
>>>>> calling releaseMemory for on-the-fly analyses.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>> Would it be OK with you if I file a PR, and then try to provide a
>>>>
>>>> patch
>>>>
>>>> to do that?
>>>>
>>>> And also add an assertion before calling run() that checks that
>>>>
>>>> releaseMemory was called.
>>>>
>>>>
>>>>
>>>>
>>>>
>>> Sounds good to me.
>>>
>>>
>>>
>>>
>>>
>>
>>
>> I opened PR4398.
>>
>>
>>
>
> I attached 2 patches to that bugreport (bugfix + testcase), please  
> review.
>
> http://llvm.org/bugs/attachment.cgi?id=3131
> http://llvm.org/bugs/attachment.cgi?id=3132

+  if (!LQ.size()) // No loops, skip calling finalizers
+    return false;

Please use LQ.empty() here instead of !LQ.size().

 > +void FunctionPassManagerImpl::releaseMemoryOnTheFly()
 > +{

LLVM style has the brace on the same line as the function name.

 > +  if (!wasRun)
 > +    return;
 > +  for (unsigned Index = 0; Index < getNumContainedManagers(); + 
+Index) {
 > +    FPPassManager *FPPM = getContainedManager(Index);
 > +    for (unsigned Index = 0; Index < FPPM->getNumContainedPasses 
(); ++Index) {
 > +      FPPM->getContainedPass(Index)->releaseMemory();
 > +    }
 > +  }
 > +}

Should this set wasRun to true after freeing all the memory?

+  // Finalize on-the-fly passes
+  for (std::map<Pass *, FunctionPassManagerImpl *>::iterator
+       I = OnTheFlyManagers.begin(), E = OnTheFlyManagers.end();
+       I != E; ++I) {
+    FunctionPassManagerImpl *FPP = I->second;
+    // We don't know when is the last time an on-the-fly pass is run,
+    // so we need to releaseMemory / finalize here
+    FPP->releaseMemoryOnTheFly();
+    Changed |= FPP->doFinalization(M);
+  }

Is it correct to call releaseMemoryOnTheFly before calling
doInitialization? It seems like it should be the other way
around.

Dan