[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

Paul Floyd  changed:

   What|Removed |Added

 Resolution|--- |FIXED
 Status|REPORTED|RESOLVED

--- Comment #13 from Paul Floyd  ---
This was done a while back.

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Mark Wielaard]

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

--- Comment #12 from Mark Wielaard  ---
BTW. I think it is more important to have these wrappers in now then to make
sure they are perfect. So if you think they are good enough now then please
check them in. But then please open bug reports for tracking alignment new vs
other allocation kinds, and one for reporting fishy alignment values. So we
don't forget.

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Mark Wielaard]

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

--- Comment #11 from Mark Wielaard  ---
I hadn't realized we were so short on bits. But I agree that reducing the szB
is questionable, at least on 32 bit. On the other hand we might be keeping a
stack of ExeContext pointers per allocation already. So maybe just making both
the allockind and szB a full word isn't that dramatic an idea. But lets do that
as a separate issue.

Just one comment about the alignment being upgraded and being a power of 2 in
ALLOC_or_NULL_ALIGNED and ALLOC_or_BOMB_ALIGNED. If I am reading the standard
correctly then ff the value of an alignment argument is not a valid alignment
value, then the behavior is undefined.

So it might make sense to don't adjust/fixup the alignment in these cases, but
let the mc_malloc_wrappers handle this, just like they handle bad sizes with
record_fishy_value_error.

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

Paul Floyd  changed:

   What|Removed |Added

 Attachment #131600|0   |1
is obsolete||

--- Comment #10 from Paul Floyd  ---
Created attachment 136085
  --> https://bugs.kde.org/attachment.cgi?id=136085=edit
Update after feedback from Mark

I think all items have been addressed except mismatch detection. I'd prefer
open another bugzilla item for that.

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

--- Comment #9 from Paul Floyd  ---
OK, I think that this is about done.

The one feature that is not present is detection of mismatches between aligned
and non-aligned new/delete. This is mostly a hypothetical problem, and will
only arise with users that have replaced these global operators. The reason
that I haven't made this change is that it will have a fairly significant
impact on 32 bit binaries. In mc_include.h there is

typedef
   enum {
  MC_AllocMalloc = 0,
  MC_AllocNew= 1,
  MC_AllocNewVec = 2,
  MC_AllocCustom = 3
   }
   MC_AllocKind;

If I add MC_AllocNewAligbed and MC_AllocNewVecAligned then this will require an
extra bit of storage for this enum. That will mean reducing the range of
MC_Chun szB, which is currently

SizeTszB : (sizeof(SizeT)*8)-2; // Size requested; 30 or 62 bits.

If I do this, then I'd rather only do it for 64 bit platforms. 61 bits for the
size is still plenty, whilst reducing the size from 1G to 512M on 32 bit might
have repercussions.

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

--- Comment #8 from Paul Floyd  ---
I'll fix the indentation, ENOMEM and feature detection. The patch also has some
merge conflicts now.

For the aligned delete operators, libcxx has

_LIBCPP_WEAK
void
operator delete(void* ptr, std::align_val_t) _NOEXCEPT
{
std::__libcpp_aligned_free(ptr);
}

and all of the other variations just call the above overload. In turn that
calls

inline _LIBCPP_INLINE_VISIBILITY
void __libcpp_aligned_free(void* __ptr) {
#if defined(_LIBCPP_MSVCRT_LIKE)
  ::_aligned_free(__ptr);
#else
  ::free(__ptr);
#endif
}

For libstd++ there is

_GLIBCXX_WEAK_DEFINITION void
operator delete(void* ptr, std::align_val_t) noexcept
{
#if _GLIBCXX_HAVE_ALIGNED_ALLOC || _GLIBCXX_HAVE_POSIX_MEMALIGN \
|| _GLIBCXX_HAVE_MEMALIGN
  std::free (ptr);
#elif _GLIBCXX_HAVE__ALIGNED_MALLOC
  _aligned_free (ptr);
#else
  if (ptr)
std::free (((void **) ptr)[-1]); // See aligned_alloc in new_opa.cc
#endif
}

and again everything seems to be implemented in terms of the above overload.
I believe _aligned_free is only on Windows. So it looks like nothing special is
done at the moment, but no guarantee that won't change so I will add builtins.

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Mark Wielaard]

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

Mark Wielaard  changed:

   What|Removed |Added

 CC||m...@klomp.org

--- Comment #7 from Mark Wielaard  ---
That is a big patch indeed. But mostly because it also adds support for libcxx
and solaris. It looks good in general I believe.

The ALLOC_or_NULL_ALIGNED now probably needs a ENOMEM check.

The indentation in coregrind/m_scheduler/scheduler.c looks off (tabs vs
spaces?)

I think we need a configure check for whether the c++ compiler supports
-std=c++17 for the testcase.

Don't we also need new builtin_delete_aligned and builtin_vec_delete_aligned to
get the new/malloc/free/delete mismatch correct?

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

Paul Floyd  changed:

   What|Removed |Added

 Attachment #113074|0   |1
is obsolete||
 CC||pa...@free.fr

--- Comment #6 from Paul Floyd  ---
Created attachment 131600
  --> https://bugs.kde.org/attachment.cgi?id=131600=edit
Updates the patch with redirs for libc++ and adds a regtest

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

--- Comment #5 from Paul Floyd  ---
Created attachment 113074
  --> https://bugs.kde.org/attachment.cgi?id=113074=edit
Patch adding aligned new delete support

A fairly big change as I added to the existing alloc/new functions and the
ALLOC macros.

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

--- Comment #4 from Paul Floyd  ---
This is starting to look good. I now get

--17711-- _ZnwmSt11align_val_t(size 64, al 64) = 0x5AB4CC0
--17711-- _ZdlPvSt11align_val_t(0x5AB4CC0)
--17711-- _ZnamSt11align_val_t(size 320, al 64) = 0x5AB4DC0
--17711-- _ZdaPvSt11align_val_t(0x5AB4DC0)
--17711-- _ZnwmSt11align_val_t(size 64, al 64) = 0x5AB4FC0
--17711-- _ZdlPvmSt11align_val_t(0x5AB4FC0)
--17711-- _ZnamSt11align_val_t(size 320, al 64) = 0x5AB50C0
--17711-- _ZdaPvmSt11align_val_t(0x5AB50C0)
--17711-- _ZnwmSt11align_val_tRKSt9nothrow_t(size 64, al 64) = 0x5AB52C0
--17711-- _ZdlPvSt11align_val_tRKSt9nothrow_t(0x5AB52C0)
--17711-- _ZnamSt11align_val_tRKSt9nothrow_t(size 320, al 64) = 0x5AB53C0
--17711-- _ZdaPvSt11align_val_tRKSt9nothrow_t(0x5AB53C0)

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

--- Comment #3 from Paul Floyd  ---
On Solaris the signatures are

32bit demangled
 U operator delete[](void*, std::align_val_t)
 U operator delete(void*, unsigned int, std::align_val_t)
 U operator new[](unsigned int, std::align_val_t)
 U operator new[](unsigned int, std::align_val_t, std::nothrow_t
const&)
 U operator new(unsigned int, std::align_val_t)
 U operator new(unsigned int, std::align_val_t, std::nothrow_t const&)

Mangled

 U _ZdaPvSt11align_val_t
 U _ZdlPvjSt11align_val_t
 U _ZnajSt11align_val_t
 U _ZnajSt11align_val_tRKSt9nothrow_t
 U _ZnwjSt11align_val_t
 U _ZnwjSt11align_val_tRKSt9nothrow_t

64 bit demangled

 U operator delete[](void*, std::align_val_t)
 U operator delete(void*, unsigned long, std::align_val_t)
 U operator new[](unsigned long, std::align_val_t)
 U operator new[](unsigned long, std::align_val_t,
std::nothrow_t const&)
 U operator new(unsigned long, std::align_val_t)
 U operator new(unsigned long, std::align_val_t, std::nothrow_t
const&)

Mangled

 U _ZdaPvSt11align_val_t
 U _ZdlPvmSt11align_val_t
 U _ZnamSt11align_val_t
 U _ZnamSt11align_val_tRKSt9nothrow_t
 U _ZnwmSt11align_val_t
 U _ZnwmSt11align_val_tRKSt9nothrow_t

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

--- Comment #2 from Paul Floyd  ---
Testcase to reproduce this behaviour:

# Makefile
.PHONY: clean
# GCC and Valgrind built from SVN/git head
CXX=${HOME}/tools/gcc/bin/g++
VG=${HOME}/tools/valgrind/bin/valgrind

test32: test.cpp
${CXX} -o $@ $< -m32 -Wl,-rpath,${HOME}/tools/gcc/lib -faligned-new
-std=c++17

test64: test.cpp
${CXX} -o $@ $< -m64 -Wl,-rpath,${HOME}/tools/gcc/lib64 -faligned-new
-std=c++17

vg32: test32
${VG} --trace-malloc=yes --show-mismatched-frees=yes ./$<

vg64: test64
${VG} --trace-malloc=yes --show-mismatched-frees=yes ./$<

clean:
rm -f test32 test64 test32_clang test64_clang

Source code:
#include 
#include 


class alignas(64) MyClass {
int i;
};

int main() {
MyClass* myClass = new MyClass();
delete myClass;

MyClass* myClassNt = new (std::nothrow) MyClass();
delete myClassNt;

MyClass* myClass5 = new MyClass[5];
delete [] myClass5;

MyClass* myClass5Nt = new (std::nothrow) MyClass[5];
delete [] myClass5Nt;
}

There are no mismatches but the output is

--4614-- memalign(al 64, size 64) = 0x4425AC0
--4614-- free(0x4425AC0)
--4614-- memalign(al 64, size 64) = 0x4425B80
--4614-- free(0x4425B80)
--4614-- memalign(al 64, size 320) = 0x4425C80
--4614-- free(0x4425C80)
--4614-- memalign(al 64, size 320) = 0x4425E40
--4614-- free(0x4425E40)

Since Valgrind does not pick up the aligned new/deletes but it does pick up the
underlying memaligns and frees.


Here are the mangled function signatures
Linux 32bit
 U _ZdaPvSt11align_val_t@@CXXABI_1.3.11
 U _ZdlPvjSt11align_val_t@@CXXABI_1.3.11
 U _ZnajSt11align_val_t@@CXXABI_1.3.11
 U _ZnajSt11align_val_tRKSt9nothrow_t@@CXXABI_1.3.11
 U _ZnwjSt11align_val_t@@CXXABI_1.3.11
 U _ZnwjSt11align_val_tRKSt9nothrow_t@@CXXABI_1.3.11

Unmangled
 U operator delete[](void*, std::align_val_t)@@CXXABI_1.3.11
 U operator delete(void*, unsigned int,
std::align_val_t)@@CXXABI_1.3.11
 U operator new[](unsigned int, std::align_val_t)@@CXXABI_1.3.11
 U operator new[](unsigned int, std::align_val_t, std::nothrow_t
const&)@@CXXABI_1.3.11
 U operator new(unsigned int, std::align_val_t)@@CXXABI_1.3.11
 U operator new(unsigned int, std::align_val_t, std::nothrow_t
const&)@@CXXABI_1.3.11

Linux 64bit
U _ZdaPvSt11align_val_t@@CXXABI_1.3.11
 U _ZdlPvmSt11align_val_t@@CXXABI_1.3.11
 U _ZnamSt11align_val_t@@CXXABI_1.3.11
 U _ZnamSt11align_val_tRKSt9nothrow_t@@CXXABI_1.3.11
 U _ZnwmSt11align_val_t@@CXXABI_1.3.11
 U _ZnwmSt11align_val_tRKSt9nothrow_t@@CXXABI_1.3.11

Unmangled
 U operator delete[](void*, std::align_val_t)@@CXXABI_1.3.11
 U operator delete(void*, unsigned long,
std::align_val_t)@@CXXABI_1.3.11
 U operator new[](unsigned long,
std::align_val_t)@@CXXABI_1.3.11
 U operator new[](unsigned long, std::align_val_t,
std::nothrow_t const&)@@CXXABI_1.3.11
 U operator new(unsigned long, std::align_val_t)@@CXXABI_1.3.11
 U operator new(unsigned long, std::align_val_t, std::nothrow_t
const&)@@CXXABI_1.3.11

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

[valgrind] [Bug 388787] Support for C++17 new/delete

[Paul Floyd]

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

--- Comment #1 from Paul Floyd  ---
The LLVM libcxx implementation of aligned new does the following:

void *
operator new(std::size_t size, std::align_val_t alignment) _THROW_BAD_ALLOC
{
set size to at least 1
set align to at least size of void*
::posix_memalign(, static_cast(alignment), size)
   if failed throw bad_alloc
   return pointer
}

The other aligned operator news just call the above one. The aligned deletes
just call ordinary delete. There is some conditional compilation for MSVCRT but
that should not be a concern.

The libstdc++ is quite similar

operator new (std::size_t sz, std::align_val_t al)
{
  set size to at least 1
  round up size to a multiple of alignment
  allocate with 'aligned_alloc' which is a wrapper around posix_memalign or
malloc if posix_memalign is not available

  if failed throw bad_alloc
   return pointer
}

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