[PATCH] Avoid Valgrind's still-reachable leak warnings

[Ryo Onodera]

Hi, Duncan

Thanks for paying attention to this patch!

Here is the patch.

If you have any questions, please let me know.

For the record, here is the explanation for this patch:



On Fri, May 31, 2013 at 8:26 AM, Ryo Onodera <ryoqun at gmail.com> wrote:
Hi,

Sorry

I lacked the supporting rationale for this change.

As a background, in this instantiation of the cl::list, the Parser
becomes llvm::PassNameParser.
And llvm::PassNameParser's destructor uses one of static
variables of ManagedStatic (to be specific,
llvm::ManagedStatic<llvm::sys::SmartMutex<true> >).

Previously, when static cl::list variables are destructed at the
very end of process termination, all ManagedStatic variables
have already been destructed via llvm::llvm_shutdown().

Nevertheless, cl::list destructs its Parser member variable,
llvm::PassNameParser accesses the ManagedStatic and the
ManagedStatic constructs its value as accessed. Finally, the
process actually terminates at this point. Then, the
ManagedStatic's value is never delete-d. Hence, still-reachable
memory leaks happen.

With my change, Parser is destructed with the correct timing of
llvm::llvm_shutdown(). And when cl::list is destructed, the
ManagedStatic<Parser> does no-op.



On Thu, May 30, 2013 at 3:37 AM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:
I am not sure I follow. Why do you have to wrap Parser, but the
Positions std::vector just above it gets freed correctly without it?

Using a ManagedStatic member looks really odd. The member will be
destroyed, but containing object will not, no? What was the
initialization order problem you were having before?


On 28 May 2013 06:23, Ryo Onodera <ryoqun at gmail.com> wrote:
> Hi,
>
> Sorry for a late reply...
>
> I changed the approach to fix those reachable memory issues, because my
> previous approach had problems.
>
> My previous approach wrapped cl::list with ManagedStatic. But, this had an
> initialization problem because cl::list strictly depends on purely being
> static variable, ie, it must be initialized at the start of runtime. But
> ManagedStatic delays the initialization by its nature.
>
> My new approach wraps Parser with ManagedStatic inside cl::list. This way,
> cl::list is correctly initialized at the start of runtime and the
> PassNamePaser is destroyed correctly at the same time. PassNameParser is
the
> actual culprit for the memory issues.
>
> In my opinion, this solution looks simpler and more straight fix than the
> previous one.
>
> I've checked all tests pass.
>
> And I've checked the memory issues are gone by running this:
>
>   valgrind --leak-check=full --show-reachable=yes ./Debug+Asserts/bin/opt
> -instnamer hello.ll -o hello.bc
>
> I think it's hard to test it correctly, because this is a memory leak
issue,
> which happens at the very end of process termination.
>
> Is there any similar test in the repository?


Regards


On Tue, Jul 2, 2013 at 10:45 PM, Duncan Sands <duncan.sands at gmail.com>wrote:

> On 25/06/13 16:21, Ryo Onodera wrote:
>
>> Hi,
>>
>> What's the status of this patch? Is there any blocker for merging?
>>
>
> I can't comment because I seem to have deleted the email with your patch.
>  Want
> to send it again?
>
> Ciao, Duncan.
>
> ______________________________**_________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/**mailman/listinfo/llvm-commits<http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>



-- 
 ryo   | 小野寺 諒   | ryoqun at gmail.com
   qun | Ryo Onodera | http://ryoqun.github.com/
       |             | 090-1264-2619
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130704/24f942fb/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Avoid-Valgrind-s-still-reachable-leak-warnings(1).patch
Type: application/octet-stream
Size: 4115 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130704/24f942fb/attachment.obj>