<div dir="ltr">Hi,<br><br>Sorry <br><br>I lacked the supporting rationale for this change.<br><br>As a background, in this instantiation of the cl::list, the Parser<br>becomes llvm::PassNameParser.<br>And llvm::PassNameParser's destructor uses one of static<br>
variables of ManagedStatic (to be specific, <br>llvm::ManagedStatic<llvm::sys::SmartMutex<true> >).<br><br>Previously, when static cl::list variables are destructed at the<br>very end of process termination, all ManagedStatic variables<br>
have already been destructed via llvm::llvm_shutdown().<br><br>Nevertheless, cl::list destructs its Parser member variable,<br>llvm::PassNameParser accesses the ManagedStatic and the<br>ManagedStatic constructs its value as accessed. Finally, the<br>
process actually terminates at this point. Then, the<br>ManagedStatic's value is never delete-d. Hence, still-reachable<br>memory leaks happen.<br><br>With my change, Parser is destructed with the correct timing of<br>
llvm::llvm_shutdown(). And when cl::list is destructed, the<br>ManagedStatic<Parser> does no-op.<br><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, May 30, 2013 at 3:37 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I am not sure I follow. Why do you have to wrap Parser, but the<br>
Positions std::vector just above it gets freed correctly without it?<br>
<br>
Using a ManagedStatic member looks really odd. The member will be<br>
destroyed, but containing object will not, no? What was the<br>
initialization order problem you were having before?<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On 28 May 2013 06:23, Ryo Onodera <<a href="mailto:ryoqun@gmail.com">ryoqun@gmail.com</a>> wrote:<br>
> Hi,<br>
><br>
> Sorry for a late reply...<br>
><br>
> I changed the approach to fix those reachable memory issues, because my<br>
> previous approach had problems.<br>
><br>
> My previous approach wrapped cl::list with ManagedStatic. But, this had an<br>
> initialization problem because cl::list strictly depends on purely being<br>
> static variable, ie, it must be initialized at the start of runtime. But<br>
> ManagedStatic delays the initialization by its nature.<br>
><br>
> My new approach wraps Parser with ManagedStatic inside cl::list. This way,<br>
> cl::list is correctly initialized at the start of runtime and the<br>
> PassNamePaser is destroyed correctly at the same time. PassNameParser is the<br>
> actual culprit for the memory issues.<br>
><br>
> In my opinion, this solution looks simpler and more straight fix than the<br>
> previous one.<br>
><br>
> I've checked all tests pass.<br>
><br>
> And I've checked the memory issues are gone by running this:<br>
><br>
> valgrind --leak-check=full --show-reachable=yes ./Debug+Asserts/bin/opt<br>
> -instnamer hello.ll -o hello.bc<br>
><br>
> I think it's hard to test it correctly, because this is a memory leak issue,<br>
> which happens at the very end of process termination.<br>
><br>
> Is there any similar test in the repository?<br>
><br>
><br>
> On Tue, May 14, 2013 at 11:07 PM, Rafael Espíndola<br>
> <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>><br>
>> Any idea why opt is still leaking? Can you turn toy.cpp into a unit<br>
>> test and include it in the patch?<br>
>><br>
>> On 13 May 2013 07:21, Ryo Onodera <<a href="mailto:ryoqun@gmail.com">ryoqun@gmail.com</a>> wrote:<br>
>> > Hi,<br>
>> ><br>
>> > Thanks for trying my patch!<br>
>> ><br>
>> > I've prepared a minimal test case. Could you give it a try?<br>
>> ><br>
>> > Build the attached source file and run like this:<br>
>> ><br>
>> > $ clang++ -g -O3 toy.cpp `/path/to//llvm-config --cppflags --libs`<br>
>> > `/path/to/llvm-config --ldflags` -o toy && valgrind --show-reachable=yes<br>
>> > --leak-check=full ./toy<br>
>> ><br>
>> > Regards,<br>
>> ><br>
>> > On Wed, May 8, 2013 at 11:02 PM, Rafael Espíndola<br>
>> > <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>> >><br>
>> >> How are you getting these warning? I tried running<br>
>> >><br>
>> >> valgrind --leak-check=full --show-reachable=yes .opt -instnamer<br>
>> >> test.ll -o test.bc<br>
>> >><br>
>> >> And I get the same 96 bytes with or without your patch.<br>
>> >><br>
>> >> On 1 April 2013 19:50, Ryo Onodera <<a href="mailto:ryoqun@gmail.com">ryoqun@gmail.com</a>> wrote:<br>
>> >> > (This is repost of my previous post to llvm-dev<br>
>> >> > <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-March/060691.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-March/060691.html</a><br>
>> >> > I sent my patch to a wrong list, sorry)<br>
>> >> ><br>
>> >> > Hi, I'm Ryo Onodera.<br>
>> >> ><br>
>> >> > This is my first post to this mailing list. Fist, thanks for creating<br>
>> >> > a great library!<br>
>> >> ><br>
>> >> > We're using LLVM to support JIT in Rubinius (an alternative<br>
>> >> > implementation of Ruby, a dynamic programming language).<br>
>> >> ><br>
>> >> > I'm submitting a small patch for a minor issue.<br>
>> >> ><br>
>> >> > It solves still-reachable leak warnings from Valgrind. Example<br>
>> >> > warnings are shown at the end of this mail.<br>
>> >> ><br>
>> >> > As these warnings weren't considered problematic in the past(*1), I<br>
>> >> > know this is really minor issue. But, I want to fix it to reduce<br>
>> >> > noises when using Valgrind, not resorting to Valgrind's suppression<br>
>> >> > mechanism.<br>
>> >> ><br>
>> >> > This patch contains no functional change. And it should cause no<br>
>> >> > problem.<br>
>> >> ><br>
>> >> > Since this is my first patch to LLVM, please let me know any<br>
>> >> > corrections. I'll greatly appreciate them and happily update my<br>
>> >> > patch.<br>
>> >> ><br>
>> >> > Also, LLVM's codebase is really clean. It took little time to make<br>
>> >> > this<br>
>> >> > patch.<br>
>> >> ><br>
>> >> > The actual warnings are like this:<br>
>> >> ><br>
>> >> > ==26332== 40 bytes in 1 blocks are still reachable in loss record 5<br>
>> >> > of<br>
>> >> > 10<br>
>> >> > ==26332== at 0x4C2B3F8: malloc (in<br>
>> >> > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)<br>
>> >> > ==26332== by 0x165CC6C: llvm::sys::MutexImpl::MutexImpl(bool)<br>
>> >> > (Mutex.cpp:49)<br>
>> >> > ==26332== by 0xD41D5A:<br>
>> >> > llvm::sys::SmartMutex<true>::SmartMutex(bool) (Mutex.h:94)<br>
>> >> > ==26332== by 0xD3E81F: void*<br>
>> >> > llvm::object_creator<llvm::sys::SmartMutex<true> >()<br>
>> >> > (ManagedStatic.h:26)<br>
>> >> > ==26332== by 0x165AA95:<br>
>> >> > llvm::ManagedStaticBase::RegisterManagedStatic(void* (*)(), void<br>
>> >> > (*)(void*)) const (ManagedStatic.cpp:50)<br>
>> >> > ==26332== by 0xD3C372:<br>
>> >> > llvm::ManagedStatic<llvm::sys::SmartMutex<true> >::operator*()<br>
>> >> > (ManagedStatic.h:68)<br>
>> >> > ==26332== by 0x1605FE2:<br>
>> >> ><br>
>> >> ><br>
>> >> > llvm::PassRegistry::removeRegistrationListener(llvm::PassRegistrationListener*)<br>
>> >> > (PassRegistry.cpp:195)<br>
>> >> > ==26332== by 0x15F1FC5:<br>
>> >> > llvm::PassRegistrationListener::~PassRegistrationListener()<br>
>> >> > (Pass.cpp:211)<br>
>> >> > ==26332== by 0x15F2074: llvm::PassNameParser::~PassNameParser()<br>
>> >> > (Pass.cpp:221)<br>
>> >> > ==26332== by 0x1604973: llvm::cl::list<llvm::PassInfo const*,<br>
>> >> > bool,<br>
>> >> > llvm::PassNameParser>::~list() (in<br>
>> >> > /home/ryoqun/rubinius/ryoqun/bin/rbx)<br>
>> >> > ==26332== by 0x5EE4900: __run_exit_handlers (exit.c:78)<br>
>> >> > ==26332== by 0x5EE4984: exit (exit.c:100)<br>
>> >> ><br>
>> >> > *1<br>
>> >> > <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-January/046828.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-January/046828.html</a><br>
>> >> ><br>
>> >> > Regards,<br>
>> >> ><br>
>> >> > _______________________________________________<br>
>> >> > llvm-commits mailing list<br>
>> >> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> >> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> >> ><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> > --<br>
>> > ryo | 小野寺 諒 | <a href="mailto:ryoqun@gmail.com">ryoqun@gmail.com</a><br>
>> > qun | Ryo Onodera | <a href="http://ryoqun.github.com/" target="_blank">http://ryoqun.github.com/</a><br>
>> > | | 090-1264-2619<br>
><br>
><br>
><br>
><br>
> --<br>
> ryo | 小野寺 諒 | <a href="mailto:ryoqun@gmail.com">ryoqun@gmail.com</a><br>
> qun | Ryo Onodera | <a href="http://ryoqun.github.com/" target="_blank">http://ryoqun.github.com/</a><br>
> | | 090-1264-2619<br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div dir="ltr"> ryo | 小野寺 諒 | <a href="mailto:ryoqun@gmail.com" target="_blank">ryoqun@gmail.com</a><br>
qun | Ryo Onodera | <a href="http://ryoqun.github.com/" target="_blank">http://ryoqun.github.com/</a><br>
| | 090-1264-2619</div>
</div>