Oooh. Neat. Thanks Dave. Please go ahead and commit that. Arnaud - I have no idea whether Dave's patch will help with this bug, but it's certainly worth testing. - Lang. On Wed, Sep 10, 2014 at 4:10 PM, David Blaikie <dblaikie at gmail.com> wrote:> While I'm not sure where the leak is, using some pre-canned memory > management might help... > > Attached is a patch that changes this allocation to use shared_ptr, > perhaps it'll address the bug? > > (ideally we shouldn't need the intrusive ref counting > (std::enable_shared_from_this) but instead have a weak_set that has > std::weak_ptr in it & implicitly removes elements as they become null > (probably on a harvesting schedule, rather than with a direct callback as > is currently implemented)) > pbqp_leak.diff > <https://docs.google.com/file/d/0B0jpkch3iC_7TXFVU2hCcUpfZXM/edit?usp=drive_web> > > > On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison < > arnaud.degrandmaison at arm.com> wrote: > >> Hi Lang, >> >> >> >> In PBQPBuilderWithCoalescing::build, around line 360, we have code >> looking like: >> >> >> >> … >> >> PBQP::Vector newCosts(g.getNodeCosts(node)); >> >> addPhysRegCoalesce(newCosts, pregOpt, cBenefit); >> >> g.setNodeCosts(node, newCosts); >> >> … >> >> >> >> I suspect the leak occurs around the setNodeCosts method, and I have >> trouble understanding how it handles the case where the node already has >> costs. >> >> >> >> It seems to me that: >> >> - we make of copy of the node’s costs (probably because someone else can >> refer to it ?) >> >> - we modify the copy >> >> - we set the node’s new costs. But what is supposed to happen there, >> especially in the CostAllocator part ? >> >> >> >> Could you shed some light there ? >> >> >> >> Thanks, >> >> Arnaud >> >> >> >> >> >> >> >> *From:* Lang Hames [mailto:lhames at gmail.com] >> *Sent:* 10 September 2014 19:26 >> *To:* Arnaud De Grandmaison >> *Subject:* Re: Leaks in PBQPBuilderWithCoalescing::build ? >> >> >> >> Thanks Arnaud! >> >> >> >> - Lang. >> >> >> >> On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison < >> arnaud.degrandmaison at arm.com> wrote: >> >> Hi Lang, >> >> >> >> For your information, the leak sanitizer found something when I committed >> my patch: >> >> >> >> >> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio >> >> >> >> I will try to have a look at it. >> >> >> >> Cheers, >> >> -- >> >> Arnaud A. de Grandmaison >> >> >> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/568877da/attachment.html>
Alexey Samsonov
2014-Sep-10 23:35 UTC
[LLVMdev] Leaks in PBQPBuilderWithCoalescing::build ?
FTR, similar leak was recently reported by LSan on our bootstrap bot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio On Wed, Sep 10, 2014 at 4:21 PM, Lang Hames <lhames at gmail.com> wrote:> Oooh. Neat. Thanks Dave. Please go ahead and commit that. > > Arnaud - I have no idea whether Dave's patch will help with this bug, but > it's certainly worth testing. > > - Lang. > > On Wed, Sep 10, 2014 at 4:10 PM, David Blaikie <dblaikie at gmail.com> wrote: > >> While I'm not sure where the leak is, using some pre-canned memory >> management might help... >> >> Attached is a patch that changes this allocation to use shared_ptr, >> perhaps it'll address the bug? >> >> (ideally we shouldn't need the intrusive ref counting >> (std::enable_shared_from_this) but instead have a weak_set that has >> std::weak_ptr in it & implicitly removes elements as they become null >> (probably on a harvesting schedule, rather than with a direct callback as >> is currently implemented)) >> pbqp_leak.diff >> <https://docs.google.com/file/d/0B0jpkch3iC_7TXFVU2hCcUpfZXM/edit?usp=drive_web> >> >> >> On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison < >> arnaud.degrandmaison at arm.com> wrote: >> >>> Hi Lang, >>> >>> >>> >>> In PBQPBuilderWithCoalescing::build, around line 360, we have code >>> looking like: >>> >>> >>> >>> … >>> >>> PBQP::Vector newCosts(g.getNodeCosts(node)); >>> >>> addPhysRegCoalesce(newCosts, pregOpt, cBenefit); >>> >>> g.setNodeCosts(node, newCosts); >>> >>> … >>> >>> >>> >>> I suspect the leak occurs around the setNodeCosts method, and I have >>> trouble understanding how it handles the case where the node already has >>> costs. >>> >>> >>> >>> It seems to me that: >>> >>> - we make of copy of the node’s costs (probably because someone else can >>> refer to it ?) >>> >>> - we modify the copy >>> >>> - we set the node’s new costs. But what is supposed to happen there, >>> especially in the CostAllocator part ? >>> >>> >>> >>> Could you shed some light there ? >>> >>> >>> >>> Thanks, >>> >>> Arnaud >>> >>> >>> >>> >>> >>> >>> >>> *From:* Lang Hames [mailto:lhames at gmail.com] >>> *Sent:* 10 September 2014 19:26 >>> *To:* Arnaud De Grandmaison >>> *Subject:* Re: Leaks in PBQPBuilderWithCoalescing::build ? >>> >>> >>> >>> Thanks Arnaud! >>> >>> >>> >>> - Lang. >>> >>> >>> >>> On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison < >>> arnaud.degrandmaison at arm.com> wrote: >>> >>> Hi Lang, >>> >>> >>> >>> For your information, the leak sanitizer found something when I >>> committed my patch: >>> >>> >>> >>> >>> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio >>> >>> >>> >>> I will try to have a look at it. >>> >>> >>> >>> Cheers, >>> >>> -- >>> >>> Arnaud A. de Grandmaison >>> >>> >>> >>> >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >>> >> > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-- Alexey Samsonov vonosmas at gmail.com -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/022d21c7/attachment.html>
Thanks Alexey! Arnaud - apologies for misleading you: It looks like the vectors are pooled too. Hopefully Dave's patch fixes this up, otherwise I'll try to take a look at this soon. - Lang. On Wed, Sep 10, 2014 at 4:35 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:> FTR, similar leak was recently reported by LSan on our bootstrap bot: > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio > > On Wed, Sep 10, 2014 at 4:21 PM, Lang Hames <lhames at gmail.com> wrote: > >> Oooh. Neat. Thanks Dave. Please go ahead and commit that. >> >> Arnaud - I have no idea whether Dave's patch will help with this bug, but >> it's certainly worth testing. >> >> - Lang. >> >> On Wed, Sep 10, 2014 at 4:10 PM, David Blaikie <dblaikie at gmail.com> >> wrote: >> >>> While I'm not sure where the leak is, using some pre-canned memory >>> management might help... >>> >>> Attached is a patch that changes this allocation to use shared_ptr, >>> perhaps it'll address the bug? >>> >>> (ideally we shouldn't need the intrusive ref counting >>> (std::enable_shared_from_this) but instead have a weak_set that has >>> std::weak_ptr in it & implicitly removes elements as they become null >>> (probably on a harvesting schedule, rather than with a direct callback as >>> is currently implemented)) >>> pbqp_leak.diff >>> <https://docs.google.com/file/d/0B0jpkch3iC_7TXFVU2hCcUpfZXM/edit?usp=drive_web> >>> >>> >>> On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison < >>> arnaud.degrandmaison at arm.com> wrote: >>> >>>> Hi Lang, >>>> >>>> >>>> >>>> In PBQPBuilderWithCoalescing::build, around line 360, we have code >>>> looking like: >>>> >>>> >>>> >>>> … >>>> >>>> PBQP::Vector newCosts(g.getNodeCosts(node)); >>>> >>>> addPhysRegCoalesce(newCosts, pregOpt, cBenefit); >>>> >>>> g.setNodeCosts(node, newCosts); >>>> >>>> … >>>> >>>> >>>> >>>> I suspect the leak occurs around the setNodeCosts method, and I have >>>> trouble understanding how it handles the case where the node already has >>>> costs. >>>> >>>> >>>> >>>> It seems to me that: >>>> >>>> - we make of copy of the node’s costs (probably because someone else >>>> can refer to it ?) >>>> >>>> - we modify the copy >>>> >>>> - we set the node’s new costs. But what is supposed to happen there, >>>> especially in the CostAllocator part ? >>>> >>>> >>>> >>>> Could you shed some light there ? >>>> >>>> >>>> >>>> Thanks, >>>> >>>> Arnaud >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> *From:* Lang Hames [mailto:lhames at gmail.com] >>>> *Sent:* 10 September 2014 19:26 >>>> *To:* Arnaud De Grandmaison >>>> *Subject:* Re: Leaks in PBQPBuilderWithCoalescing::build ? >>>> >>>> >>>> >>>> Thanks Arnaud! >>>> >>>> >>>> >>>> - Lang. >>>> >>>> >>>> >>>> On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison < >>>> arnaud.degrandmaison at arm.com> wrote: >>>> >>>> Hi Lang, >>>> >>>> >>>> >>>> For your information, the leak sanitizer found something when I >>>> committed my patch: >>>> >>>> >>>> >>>> >>>> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio >>>> >>>> >>>> >>>> I will try to have a look at it. >>>> >>>> >>>> >>>> Cheers, >>>> >>>> -- >>>> >>>> Arnaud A. de Grandmaison >>>> >>>> >>>> >>>> >>>> >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>>> >>>> >>> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >> > > > -- > Alexey Samsonov > vonosmas at gmail.com >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/4b679084/attachment.html>
On Wed, Sep 10, 2014 at 4:21 PM, Lang Hames <lhames at gmail.com> wrote:> Oooh. Neat. Thanks Dave. Please go ahead and commit that. >Committed in 217563. Running the PBQP test without my change under valgrind didn't show any memory leak, but hopefully with Arnaud's change and valgrind he can see the leak and then verify that my change addresses it... *fingers crossed*> > Arnaud - I have no idea whether Dave's patch will help with this bug, but > it's certainly worth testing. > > - Lang. > > On Wed, Sep 10, 2014 at 4:10 PM, David Blaikie <dblaikie at gmail.com> wrote: > >> While I'm not sure where the leak is, using some pre-canned memory >> management might help... >> >> Attached is a patch that changes this allocation to use shared_ptr, >> perhaps it'll address the bug? >> >> (ideally we shouldn't need the intrusive ref counting >> (std::enable_shared_from_this) but instead have a weak_set that has >> std::weak_ptr in it & implicitly removes elements as they become null >> (probably on a harvesting schedule, rather than with a direct callback as >> is currently implemented)) >> pbqp_leak.diff >> <https://docs.google.com/file/d/0B0jpkch3iC_7TXFVU2hCcUpfZXM/edit?usp=drive_web> >> >> >> On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison < >> arnaud.degrandmaison at arm.com> wrote: >> >>> Hi Lang, >>> >>> >>> >>> In PBQPBuilderWithCoalescing::build, around line 360, we have code >>> looking like: >>> >>> >>> >>> … >>> >>> PBQP::Vector newCosts(g.getNodeCosts(node)); >>> >>> addPhysRegCoalesce(newCosts, pregOpt, cBenefit); >>> >>> g.setNodeCosts(node, newCosts); >>> >>> … >>> >>> >>> >>> I suspect the leak occurs around the setNodeCosts method, and I have >>> trouble understanding how it handles the case where the node already has >>> costs. >>> >>> >>> >>> It seems to me that: >>> >>> - we make of copy of the node’s costs (probably because someone else can >>> refer to it ?) >>> >>> - we modify the copy >>> >>> - we set the node’s new costs. But what is supposed to happen there, >>> especially in the CostAllocator part ? >>> >>> >>> >>> Could you shed some light there ? >>> >>> >>> >>> Thanks, >>> >>> Arnaud >>> >>> >>> >>> >>> >>> >>> >>> *From:* Lang Hames [mailto:lhames at gmail.com] >>> *Sent:* 10 September 2014 19:26 >>> *To:* Arnaud De Grandmaison >>> *Subject:* Re: Leaks in PBQPBuilderWithCoalescing::build ? >>> >>> >>> >>> Thanks Arnaud! >>> >>> >>> >>> - Lang. >>> >>> >>> >>> On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison < >>> arnaud.degrandmaison at arm.com> wrote: >>> >>> Hi Lang, >>> >>> >>> >>> For your information, the leak sanitizer found something when I >>> committed my patch: >>> >>> >>> >>> >>> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio >>> >>> >>> >>> I will try to have a look at it. >>> >>> >>> >>> Cheers, >>> >>> -- >>> >>> Arnaud A. de Grandmaison >>> >>> >>> >>> >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >>> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140910/0e6870ae/attachment.html>
Arnaud A. de Grandmaison
2014-Sep-11 10:42 UTC
[LLVMdev] Leaks in PBQPBuilderWithCoalescing::build ?
I confirm the shared_ptr change fixes the issue. I will recommit the aarch64/pbqp test then. Thanks David ! -- Arnaud From: David Blaikie [mailto:dblaikie at gmail.com] Sent: 11 September 2014 01:06 To: Lang Hames Cc: Arnaud De Grandmaison; LLVM Developers Mailing List Subject: Re: [LLVMdev] Leaks in PBQPBuilderWithCoalescing::build ? On Wed, Sep 10, 2014 at 4:21 PM, Lang Hames <lhames at gmail.com> wrote: Oooh. Neat. Thanks Dave. Please go ahead and commit that. Committed in 217563. Running the PBQP test without my change under valgrind didn't show any memory leak, but hopefully with Arnaud's change and valgrind he can see the leak and then verify that my change addresses it... *fingers crossed* Arnaud - I have no idea whether Dave's patch will help with this bug, but it's certainly worth testing. - Lang. On Wed, Sep 10, 2014 at 4:10 PM, David Blaikie <dblaikie at gmail.com> wrote: While I'm not sure where the leak is, using some pre-canned memory management might help... Attached is a patch that changes this allocation to use shared_ptr, perhaps it'll address the bug? (ideally we shouldn't need the intrusive ref counting (std::enable_shared_from_this) but instead have a weak_set that has std::weak_ptr in it & implicitly removes elements as they become null (probably on a harvesting schedule, rather than with a direct callback as is currently implemented)) <https://docs.google.com/file/d/0B0jpkch3iC_7TXFVU2hCcUpfZXM/edit?usp=drive_web> pbqp_leak.diff On Wed, Sep 10, 2014 at 3:19 PM, Arnaud A. de Grandmaison <arnaud.degrandmaison at arm.com> wrote: Hi Lang, In PBQPBuilderWithCoalescing::build, around line 360, we have code looking like: … PBQP::Vector newCosts(g.getNodeCosts(node)); addPhysRegCoalesce(newCosts, pregOpt, cBenefit); g.setNodeCosts(node, newCosts); … I suspect the leak occurs around the setNodeCosts method, and I have trouble understanding how it handles the case where the node already has costs. It seems to me that: - we make of copy of the node’s costs (probably because someone else can refer to it ?) - we modify the copy - we set the node’s new costs. But what is supposed to happen there, especially in the CostAllocator part ? Could you shed some light there ? Thanks, Arnaud From: Lang Hames [mailto:lhames at gmail.com] Sent: 10 September 2014 19:26 To: Arnaud De Grandmaison Subject: Re: Leaks in PBQPBuilderWithCoalescing::build ? Thanks Arnaud! - Lang. On Wed, Sep 10, 2014 at 11:23 AM, Arnaud A. de Grandmaison <arnaud.degrandmaison at arm.com> wrote: Hi Lang, For your information, the leak sanitizer found something when I committed my patch: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/4506/steps/check-llvm%20asan/logs/stdio I will try to have a look at it. Cheers, -- Arnaud A. de Grandmaison _______________________________________________ LLVM Developers mailing list LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140911/1989b5e6/attachment.html>