Bug 372347 - Replacement problem of the additional c++14/c++17 new and delete operators
Summary: Replacement problem of the additional c++14/c++17 new and delete operators
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (show other bugs)
Version: 3.12 SVN
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-11 14:20 UTC by Christopher Smith
Modified: 2020-11-14 07:40 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch to add C++14 sized delete overloads (1.32 KB, patch)
2016-12-20 21:16 UTC, Paul Floyd
Details
Patch to add C++14 sized delete overloads (1.82 KB, patch)
2016-12-20 22:33 UTC, Paul Floyd
Details
Patch for C++14 sized delete operators (3.60 KB, patch)
2016-12-27 21:30 UTC, Paul Floyd
Details
Patch to test this support (2.94 KB, patch)
2020-07-17 09:46 UTC, Paul Floyd
Details
Update patch to cover both x86 and amd64 (3.28 KB, patch)
2020-11-08 16:50 UTC, Paul Floyd
Details
Fix gmake check on Solaris (1.68 KB, patch)
2020-11-09 17:39 UTC, Paul Floyd
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Smith 2016-11-11 14:20:30 UTC
I am creating custom new/delete operators which will then be forwarded to the stdlib malloc/free functions. However, when I run this, Valgrind will redirect the new operator, but not delete. This results in a Mismatched free() / delete / delete[] error.

In my case, I do NOT want Valgrind to replace the new operator.

See my question on Stackoverflow for my information (also see comments): https://stackoverflow.com/questions/40509986/valgrind-reporting-mismatched-free-delete-delete
Comment 1 Philippe Waroquiers 2016-11-13 13:42:34 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.
Comment 2 Christopher Smith 2016-11-13 14:28:53 UTC
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.
Comment 3 Philippe Waroquiers 2016-11-13 17:52:54 UTC
(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.
Comment 4 Christopher Smith 2016-11-13 19:02:33 UTC
(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.
Comment 5 Philippe Waroquiers 2016-11-13 19:53:52 UTC
(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.
Comment 6 Paul Floyd 2016-12-20 11:25:50 UTC
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.
Comment 7 Paul Floyd 2016-12-20 21:16:57 UTC
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 8 Paul Floyd 2016-12-20 22:07:31 UTC
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.
Comment 9 Paul Floyd 2016-12-20 22:33:18 UTC
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).
Comment 10 Paul Floyd 2016-12-27 21:30:26 UTC
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.
Comment 11 rcmgleite 2017-06-27 12:25:50 UTC
Any new information about this issue? having the same problem here..
Comment 12 Paul Floyd 2017-06-28 09:08:33 UTC
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.
Comment 13 Romain Geissler 2018-05-16 23:44:20 UTC
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
Comment 14 Paul Floyd 2018-05-26 17:26:15 UTC
Can you test the patch attached here?
Comment 15 Romain Geissler 2018-05-27 10:00:16 UTC
Yes I am using it already, and it's working.
Comment 16 Philippe Waroquiers 2018-05-29 19:22:42 UTC
Fixed in 6ef6f738a

Thanks for the patch
Comment 17 Paul Floyd 2018-05-29 19:51:18 UTC
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.
Comment 18 Philippe Waroquiers 2018-05-29 19:55:51 UTC
(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)
Comment 19 Paul Floyd 2018-05-29 20:03:48 UTC
I think that -fsized-deallocation would be better as it’s more specific.
Comment 20 Paul Floyd 2020-07-17 09:46:57 UTC
Created attachment 130196 [details]
Patch to test this support
Comment 21 Mark Wielaard 2020-11-07 22:16:01 UTC
(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.
Comment 22 Paul Floyd 2020-11-08 06:09:53 UTC
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.
Comment 23 Paul Floyd 2020-11-08 16:50:44 UTC
Created attachment 133151 [details]
Update patch to cover both x86 and amd64
Comment 24 Mark Wielaard 2020-11-08 19:15:23 UTC
(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.
Comment 25 Paul Floyd 2020-11-09 17:18:48 UTC
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.
Comment 26 Paul Floyd 2020-11-09 17:39:08 UTC
Created attachment 133174 [details]
Fix gmake check on Solaris
Comment 27 Mark Wielaard 2020-11-09 19:33:00 UTC
(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.