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

Ryo Onodera ryoqun at gmail.com
Thu May 30 16:26:11 PDT 2013 

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130531/38030a26/attachment.html>

More information about the llvm-commits mailing list