[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

Paul Floyd  changed:

   What|Removed |Added

 Resolution|--- |FIXED
 Status|REOPENED|RESOLVED

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Mark Wielaard]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #27 from Mark Wielaard  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #26 from Paul Floyd  ---
Created attachment 133174
  --> https://bugs.kde.org/attachment.cgi?id=133174=edit
Fix gmake check on Solaris

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

Paul Floyd  changed:

   What|Removed |Added

 Ever confirmed|0   |1
 Resolution|FIXED   |---
 Status|RESOLVED|REOPENED

--- Comment #25 from Paul Floyd  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Mark Wielaard]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #24 from Mark Wielaard  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

Paul Floyd  changed:

   What|Removed |Added

 Attachment #130196|0   |1
is obsolete||

--- Comment #23 from Paul Floyd  ---
Created attachment 133151
  --> https://bugs.kde.org/attachment.cgi?id=133151=edit
Update patch to cover both x86 and amd64

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #22 from Paul Floyd  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Mark Wielaard]

https://bugs.kde.org/show_bug.cgi?id=372347

Mark Wielaard  changed:

   What|Removed |Added

 CC||m...@klomp.org

--- Comment #21 from Mark Wielaard  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #20 from Paul Floyd  ---
Created attachment 130196
  --> https://bugs.kde.org/attachment.cgi?id=130196=edit
Patch to test this support

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #19 from Paul Floyd  ---
I think that -fsized-deallocation would be better as it’s more specific.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #18 from Philippe Waroquiers  ---
(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)

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #17 from Paul Floyd  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=372347

Philippe Waroquiers  changed:

   What|Removed |Added

 Status|UNCONFIRMED |RESOLVED
 Resolution|--- |FIXED

--- Comment #16 from Philippe Waroquiers  ---
Fixed in 6ef6f738a

Thanks for the patch

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Romain Geissler]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #15 from Romain Geissler  ---
Yes I am using it already, and it's working.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #14 from Paul Floyd  ---
Can you test the patch attached here?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Romain Geissler]

https://bugs.kde.org/show_bug.cgi?id=372347

Romain Geissler  changed:

   What|Removed |Added

 CC||romain.geiss...@amadeus.com

--- Comment #13 from Romain Geissler  ---
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

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #12 from Paul Floyd  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[rcmgleite]

https://bugs.kde.org/show_bug.cgi?id=372347

rcmgleite  changed:

   What|Removed |Added

 CC||r.cmgle...@gmail.com

--- Comment #11 from rcmgleite  ---
Any new information about this issue? having the same problem here..

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #10 from Paul Floyd  ---
Created attachment 103028
  --> https://bugs.kde.org/attachment.cgi?id=103028=edit
Patch for C++14 sized delete operators

Added operator[] overloads. Tested on Solaris 32 and 64bit, Linux 32 and 64
bit.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

Paul Floyd  changed:

   What|Removed |Added

 Attachment #102909|0   |1
is obsolete||

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #9 from Paul Floyd  ---
Created attachment 102909
  --> https://bugs.kde.org/attachment.cgi?id=102909=edit
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).

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

Paul Floyd  changed:

   What|Removed |Added

 Attachment #102906|0   |1
is obsolete||

--- Comment #8 from Paul Floyd  ---
Comment on attachment 102906
  --> https://bugs.kde.org/attachment.cgi?id=102906
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #7 from Paul Floyd  ---
Created attachment 102906
  --> https://bugs.kde.org/attachment.cgi?id=102906=edit
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Paul Floyd]

https://bugs.kde.org/show_bug.cgi?id=372347

Paul Floyd  changed:

   What|Removed |Added

 CC||pa...@free.fr

--- Comment #6 from Paul Floyd  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #5 from Philippe Waroquiers  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Christopher Smith]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #4 from Christopher Smith  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #3 from Philippe Waroquiers  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Christopher Smith]

https://bugs.kde.org/show_bug.cgi?id=372347

--- Comment #2 from Christopher Smith  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 372347] Replacement problem of the additional c++14/c++17 new and delete operators

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=372347

Philippe Waroquiers  changed:

   What|Removed |Added

Summary|Redirecting new but not |Replacement problem of the
   |delete  |additional c++14/c++17 new
   ||and delete operators

-- 
You are receiving this mail because:
You are watching all bug changes.