[llvm-commits] [llvm] r168581 - /llvm/trunk/lib/VMCore/PassManager.cpp

[Sheng Zhou]

Hi Chandler,

Thanks for the feedback...

On Sat, Dec 1, 2012 at 5:14 PM, Chandler Carruth <chandlerc at google.com>wrote:

> Zhou, this patch was never removed. Please do not commit patches without
> review first. Please revert and ping the review thread.
>

Done. Will never check in before approval.


>
> While it might seem like this patch is "obvious" two things make it clear
> that it is not:
>
> On Sun, Nov 25, 2012 at 9:45 PM, Zhou Sheng <zhousheng00 at gmail.com> wrote:
>
>> Author: sheng
>> Date: Sun Nov 25 23:45:53 2012
>> New Revision: 168581
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=168581&view=rev
>> Log:
>> Fix a PassManager pointer use-after-free bug.
>> The bug can be triggered when we require LoopInfo analysis ahead of
>> DominatorTree construction in a Module Pass.
>
>
> Then you should include a test case which demonstrates the bug under
> valgrind, or some other use-after-free detection system.
>

I already have a case to repeat the issue, and attached it with the patch
in the email I sent to llvm-commit for code review a week ago. But I'm not
sure how to add this case to llvm test suit as the case itself is a
ModulePass sample which needs to compile with llvm. Any suggestion?


>
>
>> The cause is that the LoopInfo analysis itself also invokes DominatorTree
>> construction, therefore, when PassManager schedules LoopInfo, it will add
>> DominatorTree first. Then after that, when the PassManger turns to schedule
>> DominatorTree invoked by the above ModulePass, it finds there is already a
>> DominatorTree, so it delete the redundant one. However, somehow it still
>> try to access that pass pointer after free as code pasted below, which
>> results in segment fault.
>>
>
> "somehow" isn't a good enough explanation for this fix being the correct
> fix. You need to actually understand the complete issue. Also, the comments
> below don't really seem to match your description here.
>

Okay, how about this:  " However, the freed pointer is still passed to
FunctionPassManagerImpl::setLastUser() for accessing, and then results in
crash"


>
>
> Anyways, we should take the rest of this to the code review thread, and
> continue there until the review is complete before committing.
>

Okay, got it.

-Sheng
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121201/30df1682/attachment.html>