Memory leak in parallel/unique_copy

Hi,

In parallel/unique_copy.h __counter is never deleted.

I'm also trying to follow from other posts how to submit a patch but
is well possible I missed some of the conventions. Many apologies if
that's the case.

libstdc++-v3/

* include/parallel/unique_copy.h: prevent memory leak of __counter

On 02/07/14 22:42 +0100, Goncalo Carvalho wrote:
>Hi,
>
>In parallel/unique_copy.h __counter is never deleted.
>
>I'm also trying to follow from other posts how to submit a patch but
>is well possible I missed some of the conventions. Many apologies if
>that's the case.

Thanks for this, it looks correct.

(I thought I remembered finding something similar in another parallel
header, but don't see anything in the ChangeLog.)

Do you have a testcase to reproduce the leak that we could add to the
testsuite? Or even just to run once with valgrind and verify it's
fixed (I tried a trivial test and didn't see a leak).


>libstdc++-v3/
>
>* include/parallel/unique_copy.h: prevent memory leak of __counter
>
>Index: libstdc++-v3/include/parallel/unique_copy.h
>===================================================================
>--- libstdc++-v3/include/parallel/unique_copy.h (revision 212239)
>+++ libstdc++-v3/include/parallel/unique_copy.h (working copy)
>@@ -171,6 +171,7 @@
>       for (_ThreadIndex __t = 0; __t < __num_threads + 1; __t++)
>  __end_output += __counter[__t];
>
>+      delete[] __counter;
>       delete[] __borders;
>
>       return __result + __end_output;

Hi,

Many thanks! I'll try add a test to the suite (unsure how foolproof
will be in terms of detecting memory usage).

The 11000 is simply to go beyond the minimum unique_count needed to
specialise the parallel version. This was on
g++ (GCC) 4.4.5 20110214 (Red Hat 4.4.5-6) but the issue still exists
on latest SVN.

I'll try replicate the same test on a more recent install later on.

------------------------
//g++ -std=c++0x -Wall -ggdb -O3 -fopenmp -o uq_leak uq_leak.cpp


#include <vector>
#include <iostream>
#include <parallel/algorithm>

int main()
{
    {
        size_t difftypesiz =
sizeof(std::vector<double>::iterator::difference_type);
        std::cout << "num_threads: " << omp_get_max_threads() << "
difference_type_size: " << difftypesiz << std::endl;

        std::vector<double> v(11000);
        std::vector<double> u(v.size());

        for (auto i = 0u; i < v.size(); ++i)
            v[i] = std::sqrt(i);

        for (auto i = 0; i < 10; ++i)
        {
            auto e = std::__parallel::unique_copy(v.begin(), v.end(),
u.begin());
        }
    }
}


Valgrind output below (without patch is first):

gdecarv@devapp10 dev]$ valgrind ./uq_leak
==26489== Memcheck, a memory error detector
==26489== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==26489== Using Valgrind-3.6.0 and LibVEX; rerun with -h for copyright info
==26489== Command: ./uq_leak
==26489==
num_threads: 24 difference_type_size: 8
==26489==
==26489== HEAP SUMMARY:
==26489==     in use at exit: 13,416 bytes in 36 blocks
==26489==   total heap usage: 57 allocs, 21 frees, 227,784 bytes allocated
==26489==
==26489== LEAK SUMMARY:
==26489==    definitely lost: 2,000 bytes in 10 blocks
==26489==    indirectly lost: 0 bytes in 0 blocks
==26489==      possibly lost: 6,992 bytes in 23 blocks
==26489==    still reachable: 4,424 bytes in 3 blocks
==26489==         suppressed: 0 bytes in 0 blocks
==26489== Rerun with --leak-check=full to see details of leaked memory
==26489==
==26489== For counts of detected and suppressed errors, rerun with: -v
==26489== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 6 from 6)
[gdecarv@devapp10 dev]$ g++ -std=c++0x -Wall -ggdb -O3 -fopenmp -o
uq_leak uq_leak.cpp
[gdecarv@devapp10 dev]$ valgrind ./uq_leak
==26530== Memcheck, a memory error detector
==26530== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==26530== Using Valgrind-3.6.0 and LibVEX; rerun with -h for copyright info
==26530== Command: ./uq_leak
==26530==
num_threads: 24 difference_type_size: 8
==26530==
==26530== HEAP SUMMARY:
==26530==     in use at exit: 11,416 bytes in 26 blocks
==26530==   total heap usage: 57 allocs, 31 frees, 227,784 bytes allocated
==26530==
==26530== LEAK SUMMARY:
==26530==    definitely lost: 0 bytes in 0 blocks
==26530==    indirectly lost: 0 bytes in 0 blocks
==26530==      possibly lost: 6,992 bytes in 23 blocks
==26530==    still reachable: 4,424 bytes in 3 blocks
==26530==         suppressed: 0 bytes in 0 blocks
==26530== Rerun with --leak-check=full to see details of leaked memory
==26530==
==26530== For counts of detected and suppressed errors, rerun with: -v
==26530== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 6 from 6)

On 3 July 2014 11:47, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 02/07/14 22:42 +0100, Goncalo Carvalho wrote:
>>
>> Hi,
>>
>> In parallel/unique_copy.h __counter is never deleted.
>>
>> I'm also trying to follow from other posts how to submit a patch but
>> is well possible I missed some of the conventions. Many apologies if
>> that's the case.
>
>
> Thanks for this, it looks correct.
>
> (I thought I remembered finding something similar in another parallel
> header, but don't see anything in the ChangeLog.)
>
> Do you have a testcase to reproduce the leak that we could add to the
> testsuite? Or even just to run once with valgrind and verify it's
> fixed (I tried a trivial test and didn't see a leak).
>
>
>
>> libstdc++-v3/
>>
>> * include/parallel/unique_copy.h: prevent memory leak of __counter
>>
>> Index: libstdc++-v3/include/parallel/unique_copy.h
>> ===================================================================
>> --- libstdc++-v3/include/parallel/unique_copy.h (revision 212239)
>> +++ libstdc++-v3/include/parallel/unique_copy.h (working copy)
>> @@ -171,6 +171,7 @@
>>       for (_ThreadIndex __t = 0; __t < __num_threads + 1; __t++)
>>  __end_output += __counter[__t];
>>
>> +      delete[] __counter;
>>       delete[] __borders;
>>
>>       return __result + __end_output;

On 03/07/14 13:40 +0100, Goncalo Carvalho wrote:
>Hi,
>
>Many thanks! I'll try add a test to the suite (unsure how foolproof
>will be in terms of detecting memory usage).

Yes, it might not be worth adding to the testsuite, but I want to be
able to verify the patch changes something :-)

>The 11000 is simply to go beyond the minimum unique_count needed to
>specialise the parallel version.

Ah, that's what I was missing, I didn't bother checking the minimum I
needed to pass.

>I'll try replicate the same test on a more recent install later on.

No need, I can do so - thanks for the test.