Reproducer (C++): ------------------------------- #include <assert.h> #include <pthread.h> #include <stdio.h> #include <unistd.h> #include <string> void* thread_func(void *) { std::string str = "Valgrind"; printf("Entering!\n"); usleep(100000); // Make sure the thread is killed printf("Finished!\n"); return 0; } int main() { pthread_attr_t attributes; pthread_attr_init(&attributes); pthread_attr_setdetachstate(&attributes, PTHREAD_CREATE_DETACHED); pthread_t thr; int rv = pthread_create(&thr, &attributes, thread_func, NULL); assert(rv == 0); pthread_attr_destroy(&attributes); usleep(10000); // Make sure the thread starts } ------------------------------- $ valgrind --leak-check=full ./a.out ==20096== Memcheck, a memory error detector ==20096== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al. ==20096== Using Valgrind-3.7.0.SVN and LibVEX; rerun with -h for copyright info ==20096== Command: ./a.out ==20096== Entering! ==20096== ... ==20096== 33 bytes in 1 blocks are possibly lost in loss record 1 of 2 ==20096== at 0x4C2B2A6: operator new(unsigned long) (vg_replace_malloc.c:261) ==20096== by 0x50F6D98: std::string::_Rep::_S_create(...) (new_allocator.h:89) ==20096== by 0x50F78B4: char* std::string::_S_construct<...>(...) (basic_string.tcc:139) ==20096== by 0x50F7A52: std::basic_string<...>::basic_string(char const*, std::allocator<char> const&) (basic_string.h:1546) ==20096== by 0x400B53: thread_func(void*) (nonjoin_string.cpp:9) ==20096== by 0x4E389C9: start_thread (pthread_create.c:300) ==20096== by 0x58E370C: clone (clone.S:112) AFAIK from reading the Dr. Memory paper, std::string holds a pointer to the *middle* of the allocated chunk (+4 or +8 bytes or something like that). That tool has special heuristics to treat such allocations as reachable (e.g. if the first 4/8 bytes look like a sane std::string allocation)
The report may change slightly: -------------------- void* thread_func(void *) { std::string str = "Valgrind"; str += " rocks\n"; printf("Entering!\n"); usleep(100000); // Make sure the thread is killed printf("Finished!\n"); return 0; } -------------------- results in: ==20839== 153 bytes in 1 blocks are possibly lost in loss record 1 of 2 ==20839== at 0x4C2B2A6: operator new(unsigned long) (vg_replace_malloc.c:261) ==20839== by 0x50F6D98: std::string::_Rep::_S_create(...) (new_allocator.h:89) ==20839== by 0x50F776A: std::string::_Rep::_M_clone(...) (basic_string.tcc:607) ==20839== by 0x50F829B: std::string::reserve(unsigned long) (basic_string.tcc:488) ==20839== by 0x50F84E7: std::string::append(...) (basic_string.tcc:309) ==20839== by 0x400BE8: thread_func(void*) (repstr.cpp:11) ==20839== by 0x4E389C9: start_thread (pthread_create.c:300) ==20839== by 0x58E370C: clone (clone.S:112)
Similar reports for arrays allocated new[] with non-trivial element destructors: ---------------------------- class MyClass { public: ~MyClass() { printf("BOO\n"); } }; void* thread_func(void *) { MyClass *ptr = new MyClass[256]; printf("Entering!\n"); usleep(100000); // Make sure the thread is killed printf("Finished!\n"); delete [] ptr; return 0; } ---------------------------- ==21510== 264 bytes in 1 blocks are possibly lost in loss record 1 of 2 ==21510== at 0x4C2AF1E: operator new[](unsigned long) (vg_replace_malloc.c:305) ==21510== by 0x40097A: thread_func(void*) (test.cpp:14) ==21510== by 0x4E389C9: start_thread (pthread_create.c:300) ==21510== by 0x58E370C: clone (clone.S:112)
Same for multiple inheritance when a pointer is stored to some "middle" subclass: ---------------------------- struct A { virtual ~A() { printf("BOO\n"); } }; struct B { virtual ~B() { printf("BOO\n"); } }; struct C : public A, public B { virtual ~C() { printf("BOO\n"); } }; void* thread_func(void *) { B *ptr = new C; // no reports if "A *ptr = " printf("Entering!\n"); usleep(100000); // Make sure the thread is killed printf("Finished!\n"); delete ptr; return 0; } ---------------------------- ==21869== 16 bytes in 1 blocks are possibly lost in loss record 1 of 2 ==21869== at 0x4C2B2A6: operator new(unsigned long) (vg_replace_malloc.c:261) ==21869== by 0x400AFA: thread_func(void*) (multiple_inheritance.cpp:19) ==21869== by 0x4E389C9: start_thread (pthread_create.c:300) ==21869== by 0x58E370C: clone (clone.S:112)
It's generally a good idea to stick to one issue per bug report. That said, all these things are sort of the same issue, which isn't really an issue at all. When valgrind reports a "possible leak" what it means is that it can't find a pointer to the start of the block, only a pointer to the middle of the block. Only you, as the end user, can know if that is a real problem or not. Because valgrind is operating down at the machine instruction level it has no semantic knowledge of the language your code is written in, or of the standard libraries used by that language. So basically, if you think that we should try and somehow add that semantic knowledge and hence avoid these reports, then each of these issues should be raised as a separate bug as they would need to be addressed separately. That said, it seems unlikely to me that we would be able to add such knowledge unless you've got some brilliant ideas on how we can do it.
> Only you, as the end user, can know if that is a real problem or not. I'd expect standard C++ types to be understandable by the tool. Otherwise, it gives so much noise that developers just disable the "possibly lost" reports [for example, see issue 201170]. > Unless you've got some brilliant ideas on how we can do it. You can look at the Dr. Memory publication at CGO 2011 http://www.burningcutlery.com/derek/docs/drmem-CGO11.pdf [section VI, C] The tool has basically the same "possibly lost" vs "definitely lost" classification and it has very little "possibly lost" noise, even on Windows
Well to me those Dr Memory heuristics look pretty scary. I assumed they were going to be doing something clever (but fragile) like intercepting the relevant routines but just trying guess like that scares the pants of me. What really surprises me in a lot of ways is that, as far as I know, nobody has ever complained about any of these before. We do occasionally get people with issues with std::string (and some of the standard library containers) but that's always to do with the library optimising things by caching "freed" space for later reuse and such like. Now it's true that Dr Memory will probably see a lot more C++ than we do simply because it supports Windows so that might be part of it. Did you actually run into these issues yourself in real world code? or did you set out to reproduce them based on the comments in the Dr Memory paper?
> but just trying guess like that scares the pants of me. Is there something you're aware of except for false negatives? btw, by doing the described trick they also don't suffer false "possible leak" reports in sqlite which has its own malloc wrapper. > Did you actually run into these issues yourself in real world code? Yes I did - Chromium has an enormous number of such reports [see also bug 201170 for Dan Kegel's feature request from his Chromium-Valgrind experience] We've just disabled printing Valgrind possible leaks just because of that. And we still print possible leaks while running Dr. Memory! --------------- Also, running Chromium unit_tests alone we get ~13000 suppressed reports like the comment #1 and #2: http://build.chromium.org/p/chromium.memory/builders/Linux%20Tests%20%28valgrind%29%286%29/builds/424/steps/memory%20test%3A%20unit/logs/stdio [part 1 of 2] Suppressions used: count name ... 683 bug_76386a 997 bug_76386b http://build.chromium.org/p/chromium.memory/builders/Linux%20Tests%20%28valgrind%29%287%29/builds/403/steps/memory%20test%3A%20unit/logs/stdio [part 2 of 2] Suppressions used: count name ... 2207 bug_76386b 8936 bug_76386a Suppression at this time are: http://src.chromium.org/viewvc/chrome/trunk/src/tools/valgrind/memcheck/suppressions.txt?revision=97188&view=markup { bug_76386a Memcheck:Leak fun:_Znw* fun:_ZNSs4_Rep9_S_createE*RKSaIcE ... fun:_ZNSsC1*KS* } { bug_76386b Memcheck:Leak fun:_Znw* fun:_ZNSs4_Rep9_S_createE*RKSaIcE fun:_ZNSs4_Rep8_M_cloneERKSaIcE* } I'm not sure if bug_76386 are actually "possibly lost" leaks in Valgrind's mind as we're disabling printing those. However, each time such a leak appears it looks very strange like a "leak" on some local string variable, so I can't support Valgrind's opinion there either. Maybe that's "library optimising things by caching "freed" space for later reuse and such like" you're talking about. But I'd suspect "caching "freed" space for later reuse" would still be reachable, no?
> What really surprises me in a lot of ways is that, as far as I know, > nobody has ever complained about any of these before. These report matter in the following cases: a) The running process receives a SIGTERM in the middle of execution b) There are non-joined threads [maybe more] a) and b) are true for Chromium with its complex process architecture and many threads. Not sure if it affects other C++ projects as much. WDYT?
Created attachment 64673 [details] Proposed patch for std::string and new[] I've made a patch [attached] that implements the described heuristics for std::string and new[]. Multiple inheritance is not a priority right now since we don't use it very often in Chromium.
Created attachment 64736 [details] Improved naming; add support for string16 and wstring Updating the patch. Tested on the following code: ------------------------------------------ #include <string> typedef std::basic_string<unsigned char> string16; std::string *ptr8; string16 *ptr16; std::wstring *ptr32; class MyClass { public: ~MyClass() { } } *new_array; int main(void) { ptr8 = new std::string; *ptr8 += "ASDASDASDASDASDASDASDSAD"; ptr16 = new string16; for (int i = 0; i < ptr8->size(); i++) *ptr16 += (unsigned char)(*ptr8)[i]; ptr32 = new std::wstring; *ptr32 += L"ASDASDASDASDASDASDASDSAD"; new_array = new MyClass[42]; }
fixed in rev r13582, which adds the option: --leak-check-heuristics=heur1,heur2,... which heuristics to use for improving leak search false positive [none] where heur is one of stdstring newarray multipleinheritance all none
Just curious - why is it [none] by default rather than [all] ?
(In reply to comment #12) > Just curious - why is it [none] by default rather than [all] It looked to me somewhat preferrable to keep the current behaviour, as there is a chance that the heuristics create false negative leaks and not everybody uses these c++ constructs. On the other hand, when an heuristic is used, it appears in the leak summary e.g. ... still reachable: 111 bytes in 7 blocks of which reachable via heuristic: stdstring : 56 bytes in 2 blocks newarray : 7 bytes in 1 blocks multipleinheritance: 24 bytes in 2 blocks So an [all] default would not create fully "invisible" false negative.
I'd argue we should set [all] as the default at least on 32-bit archs, see https://code.google.com/p/valgrind-variant/wiki/LeakCheckingOn32bits