[PATCH] Avoid Valgrind's still-reachable leak warnings
Ryo Onodera
ryoqun at gmail.com
Tue Jun 25 07:21:26 PDT 2013
Hi,
What's the status of this patch? Is there any blocker for merging?
Regards,
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?
>> >
>> >
>> > On Tue, May 14, 2013 at 11:07 PM, Rafael Espíndola
>> > <rafael.espindola at gmail.com> wrote:
>> >>
>> >> Any idea why opt is still leaking? Can you turn toy.cpp into a unit
>> >> test and include it in the patch?
>> >>
>> >> On 13 May 2013 07:21, Ryo Onodera <ryoqun at gmail.com> wrote:
>> >> > Hi,
>> >> >
>> >> > Thanks for trying my patch!
>> >> >
>> >> > I've prepared a minimal test case. Could you give it a try?
>> >> >
>> >> > Build the attached source file and run like this:
>> >> >
>> >> > $ clang++ -g -O3 toy.cpp `/path/to//llvm-config --cppflags --libs`
>> >> > `/path/to/llvm-config --ldflags` -o toy && valgrind
>> --show-reachable=yes
>> >> > --leak-check=full ./toy
>> >> >
>> >> > Regards,
>> >> >
>> >> > On Wed, May 8, 2013 at 11:02 PM, Rafael Espíndola
>> >> > <rafael.espindola at gmail.com> wrote:
>> >> >>
>> >> >> How are you getting these warning? I tried running
>> >> >>
>> >> >> valgrind --leak-check=full --show-reachable=yes .opt -instnamer
>> >> >> test.ll -o test.bc
>> >> >>
>> >> >> And I get the same 96 bytes with or without your patch.
>> >> >>
>> >> >> On 1 April 2013 19:50, Ryo Onodera <ryoqun at gmail.com> wrote:
>> >> >> > (This is repost of my previous post to llvm-dev
>> >> >> > http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-March/060691.html
>> >> >> > I sent my patch to a wrong list, sorry)
>> >> >> >
>> >> >> > Hi, I'm Ryo Onodera.
>> >> >> >
>> >> >> > This is my first post to this mailing list. Fist, thanks for
>> creating
>> >> >> > a great library!
>> >> >> >
>> >> >> > We're using LLVM to support JIT in Rubinius (an alternative
>> >> >> > implementation of Ruby, a dynamic programming language).
>> >> >> >
>> >> >> > I'm submitting a small patch for a minor issue.
>> >> >> >
>> >> >> > It solves still-reachable leak warnings from Valgrind. Example
>> >> >> > warnings are shown at the end of this mail.
>> >> >> >
>> >> >> > As these warnings weren't considered problematic in the past(*1),
>> I
>> >> >> > know this is really minor issue. But, I want to fix it to reduce
>> >> >> > noises when using Valgrind, not resorting to Valgrind's
>> suppression
>> >> >> > mechanism.
>> >> >> >
>> >> >> > This patch contains no functional change. And it should cause no
>> >> >> > problem.
>> >> >> >
>> >> >> > Since this is my first patch to LLVM, please let me know any
>> >> >> > corrections. I'll greatly appreciate them and happily update my
>> >> >> > patch.
>> >> >> >
>> >> >> > Also, LLVM's codebase is really clean. It took little time to make
>> >> >> > this
>> >> >> > patch.
>> >> >> >
>> >> >> > The actual warnings are like this:
>> >> >> >
>> >> >> > ==26332== 40 bytes in 1 blocks are still reachable in loss record
>> 5
>> >> >> > of
>> >> >> > 10
>> >> >> > ==26332== at 0x4C2B3F8: malloc (in
>> >> >> > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> >> >> > ==26332== by 0x165CC6C: llvm::sys::MutexImpl::MutexImpl(bool)
>> >> >> > (Mutex.cpp:49)
>> >> >> > ==26332== by 0xD41D5A:
>> >> >> > llvm::sys::SmartMutex<true>::SmartMutex(bool) (Mutex.h:94)
>> >> >> > ==26332== by 0xD3E81F: void*
>> >> >> > llvm::object_creator<llvm::sys::SmartMutex<true> >()
>> >> >> > (ManagedStatic.h:26)
>> >> >> > ==26332== by 0x165AA95:
>> >> >> > llvm::ManagedStaticBase::RegisterManagedStatic(void* (*)(), void
>> >> >> > (*)(void*)) const (ManagedStatic.cpp:50)
>> >> >> > ==26332== by 0xD3C372:
>> >> >> > llvm::ManagedStatic<llvm::sys::SmartMutex<true> >::operator*()
>> >> >> > (ManagedStatic.h:68)
>> >> >> > ==26332== by 0x1605FE2:
>> >> >> >
>> >> >> >
>> >> >> >
>> llvm::PassRegistry::removeRegistrationListener(llvm::PassRegistrationListener*)
>> >> >> > (PassRegistry.cpp:195)
>> >> >> > ==26332== by 0x15F1FC5:
>> >> >> > llvm::PassRegistrationListener::~PassRegistrationListener()
>> >> >> > (Pass.cpp:211)
>> >> >> > ==26332== by 0x15F2074: llvm::PassNameParser::~PassNameParser()
>> >> >> > (Pass.cpp:221)
>> >> >> > ==26332== by 0x1604973: llvm::cl::list<llvm::PassInfo const*,
>> >> >> > bool,
>> >> >> > llvm::PassNameParser>::~list() (in
>> >> >> > /home/ryoqun/rubinius/ryoqun/bin/rbx)
>> >> >> > ==26332== by 0x5EE4900: __run_exit_handlers (exit.c:78)
>> >> >> > ==26332== by 0x5EE4984: exit (exit.c:100)
>> >> >> >
>> >> >> > *1
>> >> >> >
>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-January/046828.html
>> >> >> >
>> >> >> > Regards,
>> >> >> >
>> >> >> > _______________________________________________
>> >> >> > llvm-commits mailing list
>> >> >> > llvm-commits at cs.uiuc.edu
>> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > ryo | 小野寺 諒 | ryoqun at gmail.com
>> >> > qun | Ryo Onodera | http://ryoqun.github.com/
>> >> > | | 090-1264-2619
>> >
>> >
>> >
>> >
>> > --
>> > ryo | 小野寺 諒 | ryoqun at gmail.com
>> > qun | Ryo Onodera | http://ryoqun.github.com/
>> > | | 090-1264-2619
>>
>
>
>
> --
> ryo | 小野寺 諒 | ryoqun at gmail.com
> qun | Ryo Onodera | http://ryoqun.github.com/
> | | 090-1264-2619
>
--
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/20130625/42137681/attachment.html>
More information about the llvm-commits
mailing list