Summary: | Replacement problem of the additional c++14/c++17 new and delete operators | ||
---|---|---|---|
Product: | [Developer tools] valgrind | Reporter: | Christopher Smith <chris> |
Component: | memcheck | Assignee: | Julian Seward <jseward> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | chris, mark, philippe.waroquiers, pjfloyd, r.cmgleite, romain.geissler |
Priority: | NOR | ||
Version: | 3.12 SVN | ||
Target Milestone: | --- | ||
Platform: | Ubuntu | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | ||
Attachments: |
Patch to add C++14 sized delete overloads
Patch to add C++14 sized delete overloads Patch for C++14 sized delete operators Patch to test this support Update patch to cover both x86 and amd64 Fix gmake check on Solaris |
Description
Christopher Smith
2016-11-11 14:20:30 UTC
I am not a specialist in c++ (in fact, I know close to 0), but what I think is happening is: In c++14, a new operator void operator delete ( void* ptr, std::size_t sz ); was added. The compiler seems to emit automatically this operator if it knows the size (at compile time?). The operator new in your program is compiled as: callq 0x400858 <_Znwm> which matches the symbol name replaced by valgrind. The operator delete in your program is compiled as: callq 0x40088d <_ZdlPvm> and this is unknown by valgrind replacement logic. If I compiled with -std=c++11, it gives: callq 0x400872 <_ZdlPv> which is replaced by Valgrind. As I understand, the problem is that the valgrind replacement logic has not been updated to support the additional new and delete operators of c++14 (and c++17). So, this seems to be a real Valgrind bug. You might bypass this by using --show-mismatched-frees=no or by compiling in c++11. When using c++14 or above: I am wondering what such half replacement can cause as other problems: we might have that the memory allocated by the valgrind replacement cannot properly be deleted by the (not replaced) delete operator. Thanks for the reply. If you plan on making Valgrind override my delete operator as well, that could cause issues for my intermediate code. One thing I could do, would be to use the Client Request Mechanism to notify Valgrind of my memory changes, but then disable the operator replacement. Is it possible to disable the replacement? I'm reading over the docs and it mentions VALGRIND_CREATE_MEMPOOL. If I'm running on Linux, I wouldn't have access to that. Would it be possible to just override malloc/free and NOT new/delete? I would expect there to be an option for this. (In reply to Christopher Smith from comment #2) > Thanks for the reply. > > If you plan on making Valgrind override my delete operator as well, that > could cause issues for my intermediate code. If your new/delete operators will have the 'normal/expected' semantic, then why would the Valgrind replacement create a problem ? Or, in other words, what is special in your operators that make them non replaceable ? You lose a lot of Valgrind functionalities if you do not replace the heap functions. > One thing I could do, would be > to use the Client Request Mechanism to notify Valgrind of my memory changes, > but then disable the operator replacement. Is it possible to disable the > replacement? You can disable replacement of new/delete if they are statically linked, or if they are in a 'non standard library' (i.e. a lib different of libc.so* on linux). See user manual option --soname-synonyms=syn1=pattern1,syn2=pattern2,... for more details. > > I'm reading over the docs and it mentions VALGRIND_CREATE_MEMPOOL. If I'm > running on Linux, I wouldn't have access to that. I do not understand what you mean with this "wouldn't have access to that." > Would it be possible to > just override malloc/free and NOT new/delete? I would expect there to be an > option for this. There is no specific option to do partial replacement of some allocation functions. (In reply to Philippe Waroquiers from comment #3) > If your new/delete operators will have the 'normal/expected' semantic, > then why would the Valgrind replacement create a problem ? > Or, in other words, what is special in your operators that make them > non replaceable ? > You lose a lot of Valgrind functionalities if you do not replace > the heap functions. Certain parts of my program do a lot of allocation/deallocation. I plan on adding a sort of "cache" for same sized sections of memory to prevent having to run malloc each time. Although I suppose this is purely a performance optimization and can be omitted during testing. For now, I'll use `--show-mismatched-frees=no`. I'm looking forward to this fix. (In reply to Christopher Smith from comment #4) > (In reply to Philippe Waroquiers from comment #3) > > If your new/delete operators will have the 'normal/expected' semantic, > > then why would the Valgrind replacement create a problem ? > > Or, in other words, what is special in your operators that make them > > non replaceable ? > > You lose a lot of Valgrind functionalities if you do not replace > > the heap functions. > > Certain parts of my program do a lot of allocation/deallocation. I plan on > adding a sort of "cache" for same sized sections of memory to prevent having > to run malloc each time. Although I suppose this is purely a performance > optimization and can be omitted during testing. If the only thing that your replacement functions are doing is to do a (local) cache of small objects, then it should be no problem to have them replaced by Valgrind, and then benefit from all the Valgrind features (leak search, redzone protection, etc). It is only when your specific heap does very special actions (such as for example releasing all small blocks at once) that no replacement should be done. For such special heaps, you then must use the various *MEMPOOL* requests to allow valgrind to (somewhat) understand your specific heap. > > For now, I'll use `--show-mismatched-frees=no`. I'm looking forward to this > fix. Without any options, g++ now defaults to -std=gnu++14 (see here https://gcc.gnu.org/onlinedocs/gcc/Standards.html) It's possible to use compiler options like -std=c++11 or -fno-sized-deallocator to avoid the Valgrind issue, but this will mean that the sized operator delete won't be used. There's a full list of post-C++11 new and delete operators here http://en.cppreference.com/w/cpp/memory/new/operator_delete I guess that the 12 overloads marked as C++14 and C++17 aren't currently supported by Valgrind. I'll look at making a patch for this. Created attachment 102906 [details]
Patch to add C++14 sized delete overloads
Tested on Solaris.
==9916== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Will also test on Linux.
Comment on attachment 102906 [details]
Patch to add C++14 sized delete overloads
1st attempt not good. Contained a typo and doesn't correctly handle 32/64bit versions of size_t.
Created attachment 102909 [details]
Patch to add C++14 sized delete overloads
Tested on Solaris 32bit and 64bit, Linux 64 bit (don't currently have 32bit dev support installed).
Created attachment 103028 [details]
Patch for C++14 sized delete operators
Added operator[] overloads. Tested on Solaris 32 and 64bit, Linux 32 and 64 bit.
Any new information about this issue? having the same problem here.. Are you having issues with C++14 or C++17? I have a patch for C++14. I need to do a bit more work to get my regression tests for it into the Valgrind format. For the C++17, it looks like a lot more work and I'll probably open another Bugzilla item for that. Hi, I just found I have exactly the same issue (with sized deallocator from C++14) and using tcmalloc_debug. Is there any chance to see this patch upstream'ed (I checked current git and it is currently not merged). Cheers, Romain Can you test the patch attached here? Yes I am using it already, and it's working. Fixed in 6ef6f738a Thanks for the patch Great. I also have a small test case for this, but it uses a Makefile rather than the Valgrind perl mechanism. I'll look into adapting it to the Valgrind infrastructure. (In reply to Paul Floyd from comment #17) > Great. I also have a small test case for this, but it uses a Makefile rather > than the Valgrind perl mechanism. I'll look into adapting it to the Valgrind > infrastructure. That would be nice. You might maybe have to check if gcc accepts the -std=c++14 flag (not too sure what past gcc we want to support) I think that -fsized-deallocation would be better as it’s more specific. Created attachment 130196 [details]
Patch to test this support
(In reply to Paul Floyd from comment #20) > Created attachment 130196 [details] > Patch to test this support So this tests that we see (matching) new, delete and new[], delete[] where the deletes are sized. It filters out free, but not malloc. Why is it trying to match that malloc? Except for the malloc (which I simply don't understand why we are interested in, does it come from the test or some library call?) the test itself looks good. The test boilerplate also looks good. I think you can drop the prereq: test -e sized_delete because the test will always be build, or is there a way that it isn't build? Is the test really amd64 specific? It would be nice if it was a generic test. If the symbols change between 64 and 32 bit arches you could provide an alternative .exp file. Filtering out the free was enough to get the test to pass on the 3 OSes that I tested on [Linux, Solaris and FreeBSD]. You are right that it is not necessary for the test, and I'll add malloc to the filter. I want to keep the prereq because in theory someone could be building with a very old GCC that does not support -fsized-deallocation. The test is currently amd64 specific. The signature includes size_t, which is a synonym for unsigned int on x86 (_ZdlPvj) and unsigned long on amd64 (_ZdlPvm). That's for the scalar form of delete, there's also the array form with the same difference. I'll move it up to memcheck/tests and add an x86 expected. Created attachment 133151 [details]
Update patch to cover both x86 and amd64
(In reply to Paul Floyd from comment #23) > Created attachment 133151 [details] > Update patch to cover both x86 and amd64 Looks good to me. I believe the sized_delete.stderr.exp file should match not just amd64 but all 64bit arches and the sized_delete.stderr.exp-x86 one should match all 32bit ones. This didn't quite work as planned on Solaris. gmake check fails with the venerable system GCC 4.8.2. Will add a patch asap. Created attachment 133174 [details]
Fix gmake check on Solaris
(In reply to Paul Floyd from comment #22) > I want to keep the prereq because in theory someone could be building with a > very old GCC that does not support -fsized-deallocation. And you were correct :) (In reply to Paul Floyd from comment #26) > Created attachment 133174 [details] > Fix gmake check on Solaris Looks good. |