Re: [PATCH] PR libstdc++/79190 add fallback aligned_alloc implementation

2017-01-26 Thread Jonathan Wakely 

On 26/01/17 01:05 +0100, Jakub Jelinek wrote:

On Tue, Jan 24, 2017 at 06:33:51PM +, Jonathan Wakely wrote:

--- a/libstdc++-v3/libsupc++/new_opa.cc
+++ b/libstdc++-v3/libsupc++/new_opa.cc
@@ -55,9 +55,30 @@ extern "C" void *memalign(std::size_t boundary, std::size_t 
size);
 #endif
 #define aligned_alloc memalign
 #else
-// The C library doesn't provide any aligned allocation functions, declare
-// aligned_alloc and get a link failure if aligned new is used.
-extern "C" void *aligned_alloc(std::size_t, std::size_t);
+// This is a modified version of code from gcc/config/i386/gmm_malloc.h
+static inline void*
+aligned_alloc (std::size_t al, std::size_t sz)
+{
+  // Alignment must be a power of two.
+  if (al & (al - 1))
+return nullptr;
+  else if (!sz)
+return nullptr;
+
+  // We need extra bytes to store the original value returned by malloc.
+  if (al < sizeof(void*))
+al = sizeof(void*);
+  void* const malloc_ptr = malloc(sz + al);
+  if (!malloc_ptr)
+return nullptr;
+  // Align to the requested value, leaving room for the original malloc value.
+  void* const aligned_ptr = (void *) (((size_t) malloc_ptr + al) & -al);


Shouldn't this be cast to uintptr_t rather than size_t?  On some targets
that is not the same thing, I think e.g. on m32c:
grep 'SIZE_TYPE\|UINTPTR_TYPE\|POINTER_SIZE\|INT_TYPE_SIZE' config/m32c/*
config/m32c/m32c.h:#define POINTER_SIZE (TARGET_A16 ? 16 : 32)
config/m32c/m32c.h:#define INT_TYPE_SIZE 16
config/m32c/m32c.h:#undef UINTPTR_TYPE
config/m32c/m32c.h:#define UINTPTR_TYPE (TARGET_A16 ? "unsigned int" : "long 
unsigned int")
config/m32c/m32c.h:#undef  SIZE_TYPE
config/m32c/m32c.h:#define SIZE_TYPE "unsigned int"
which means e.g. for -mcpu=m32c pointers are 24-bit, integers/size_t are
16-bit and uintptr_t is 32-bit, so if you cast a pointer to size_t, you'll
lose the upper 8 bits.


Good point, thanks.


Also, for the arguments you use std::size_t, but not here, shouldn't that
be std::uintptr_t then?


We use std::size_t there because we only included 
and not , so we're not guaranteed to have ::size_t declared.
If I include  for uintptr_t then it won't be in namespace
std and can be used unqualified. Elsewhere we guard use of stdint.h
with _GLIBCXX_USE_C99_STDINT_TR1 but I don't think we need that
nowadays,

Dave Anglin confirmed this fixes the problem, so I'll commit this
version (just adding  and using uintptr_t instead of
size_t).

I forgot to mention that the g++.dg/cpp1z/aligned-new3.C test needed
tweaking because it replaced the aligned-delete operator but not the
aligned-new operator. That meant it was failing when this fallback
implementation of aligned_alloc is used, because aligned-new did the
prefixed allocation, but the replaced operator delete doesn't free the
original pointer stored in the prefix. By replacing both operators we
avoid the prefixing entirely.
commit a3b99f1671a3a1989a6d2d90162d33365a409c02
Author: Jonathan Wakely 
Date:   Thu Jan 26 10:52:52 2017 +

PR libstdc++/79190 add fallback aligned_alloc implementation

libstdc++-v3:

	PR libstdc++/79190
	* libsupc++/del_opa.cc (operator delete(void*, std::align_val_t))
	[!_GLIBCXX_HAVE_ALIGNED_ALLOC && !_GLIBCXX_HAVE_POSIX_MEMALIGN
	&& !_GLIBCXX_HAVE_MEMALIGN && !_GLIBCXX_HAVE__ALIGNED_MALLOC]:
	Retrieve original pointer value allocated by malloc.
	* libsupc++/new_opa.cc [!_GLIBCXX_HAVE_ALIGNED_ALLOC
	&& !_GLIBCXX_HAVE_POSIX_MEMALIGN && !_GLIBCXX_HAVE_MEMALIGN
	&& !_GLIBCXX_HAVE__ALIGNED_MALLOC] (aligned_alloc(size_t, size_t)):
	Define, adjusting pointer value allocated by malloc and storing for
	retrieval by operator delete.

gcc/testsuite:

	PR libstdc++/79190
	* g++.dg/cpp1z/aligned-new3.C: Replace operator new so behaviour
	matches replaced operator delete.

diff --git a/gcc/testsuite/g++.dg/cpp1z/aligned-new3.C b/gcc/testsuite/g++.dg/cpp1z/aligned-new3.C
index 73e3343..e50e62c 100644
--- a/gcc/testsuite/g++.dg/cpp1z/aligned-new3.C
+++ b/gcc/testsuite/g++.dg/cpp1z/aligned-new3.C
@@ -7,6 +7,11 @@ struct alignas(64) A {
   int i;
 };
 
+void* operator new (std::size_t n, std::align_val_t)
+{
+  return operator new (n);
+}
+
 bool deleted = false;
 void operator delete (void *p, std::size_t, std::align_val_t)
 {
diff --git a/libstdc++-v3/libsupc++/del_opa.cc b/libstdc++-v3/libsupc++/del_opa.cc
index c9afb46..f7bf7a4 100644
--- a/libstdc++-v3/libsupc++/del_opa.cc
+++ b/libstdc++-v3/libsupc++/del_opa.cc
@@ -46,9 +46,13 @@ _GLIBCXX_END_NAMESPACE_VERSION
 _GLIBCXX_WEAK_DEFINITION void
 operator delete(void* ptr, std::align_val_t) _GLIBCXX_USE_NOEXCEPT
 {
-#if !_GLIBCXX_HAVE_ALIGNED_ALLOC && _GLIBCXX_HAVE__ALIGNED_MALLOC
+#if _GLIBCXX_HAVE_ALIGNED_ALLOC || _GLIBCXX_HAVE_POSIX_MEMALIGN \
+|| _GLIBCXX_HAVE_MEMALIGN
+  std::free (ptr);
+#elif _GLIBCXX_HAVE__ALIGNED_MALLOC
   _aligned_free (ptr);
 #else
-  std::free(ptr);
+  if (ptr)
+std::free (((void **) ptr)[-1]); // See

Re: [PATCH] PR libstdc++/79190 add fallback aligned_alloc implementation

2017-01-25 Thread Jakub Jelinek 
On Tue, Jan 24, 2017 at 06:33:51PM +, Jonathan Wakely wrote:
> --- a/libstdc++-v3/libsupc++/new_opa.cc
> +++ b/libstdc++-v3/libsupc++/new_opa.cc
> @@ -55,9 +55,30 @@ extern "C" void *memalign(std::size_t boundary, 
> std::size_t size);
>  #endif
>  #define aligned_alloc memalign
>  #else
> -// The C library doesn't provide any aligned allocation functions, declare
> -// aligned_alloc and get a link failure if aligned new is used.
> -extern "C" void *aligned_alloc(std::size_t, std::size_t);
> +// This is a modified version of code from gcc/config/i386/gmm_malloc.h
> +static inline void*
> +aligned_alloc (std::size_t al, std::size_t sz)
> +{
> +  // Alignment must be a power of two.
> +  if (al & (al - 1))
> +return nullptr;
> +  else if (!sz)
> +return nullptr;
> +
> +  // We need extra bytes to store the original value returned by malloc.
> +  if (al < sizeof(void*))
> +al = sizeof(void*);
> +  void* const malloc_ptr = malloc(sz + al);
> +  if (!malloc_ptr)
> +return nullptr;
> +  // Align to the requested value, leaving room for the original malloc 
> value.
> +  void* const aligned_ptr = (void *) (((size_t) malloc_ptr + al) & -al);

Shouldn't this be cast to uintptr_t rather than size_t?  On some targets
that is not the same thing, I think e.g. on m32c:
grep 'SIZE_TYPE\|UINTPTR_TYPE\|POINTER_SIZE\|INT_TYPE_SIZE' config/m32c/*
config/m32c/m32c.h:#define POINTER_SIZE (TARGET_A16 ? 16 : 32)
config/m32c/m32c.h:#define INT_TYPE_SIZE 16
config/m32c/m32c.h:#undef UINTPTR_TYPE
config/m32c/m32c.h:#define UINTPTR_TYPE (TARGET_A16 ? "unsigned int" : "long 
unsigned int")
config/m32c/m32c.h:#undef  SIZE_TYPE
config/m32c/m32c.h:#define SIZE_TYPE "unsigned int"
which means e.g. for -mcpu=m32c pointers are 24-bit, integers/size_t are
16-bit and uintptr_t is 32-bit, so if you cast a pointer to size_t, you'll
lose the upper 8 bits.
Also, for the arguments you use std::size_t, but not here, shouldn't that
be std::uintptr_t then?

> +
> +  // Store the original malloc value where it can be found by operator 
> delete.
> +  ((void **) aligned_ptr)[-1] = malloc_ptr;
> +
> +  return aligned_ptr;
> +}
>  #endif
>  #endif
>  

Jakub

[PATCH] PR libstdc++/79190 add fallback aligned_alloc implementation

2017-01-24 Thread Jonathan Wakely 

This provides a definition of aligned_alloc for targets which have
none of aligned_alloc, posix_memalign, memalign or _aligned_alloc.

The code is based on gcc/config/i386/gmm_malloc.h but modified to only
use sizeof(void*) not 2*sizeof(void*), because I don't understand why
that's used in gmm_malloc.h, or what the comment means.

libstdc++-v3:

PR libstdc++/79190
* libsupc++/del_opa.cc (operator delete(void*, std::align_val_t))
[!_GLIBCXX_HAVE_ALIGNED_ALLOC && !_GLIBCXX_HAVE_POSIX_MEMALIGN
&& !_GLIBCXX_HAVE_MEMALIGN && !_GLIBCXX_HAVE__ALIGNED_MALLOC]:
Retrieve original pointer value allocated by malloc.
* libsupc++/new_opa.cc [!_GLIBCXX_HAVE_ALIGNED_ALLOC
&& !_GLIBCXX_HAVE_POSIX_MEMALIGN && !_GLIBCXX_HAVE_MEMALIGN
&& !_GLIBCXX_HAVE__ALIGNED_MALLOC] (aligned_alloc(size_t, size_t)):
Define, adjusting pointer value allocated by malloc and storing for
retrieval by operator delete.

gcc/testsuite:

PR libstdc++/79190
* g++.dg/cpp1z/aligned-new3.C: Replace operator new so behaviour
matches replaced operator delete.

Tested powerpc64le-linux, and also tested after tweaking the
preprocessor checks so the new code was used.

If this works for HPUX I plan to commit it to trunk.


commit 2f17c7769b6917b6ab6884c87c4496e32d4cc057
Author: Jonathan Wakely 
Date:   Tue Jan 24 16:17:15 2017 +

PR libstdc++/79190 add fallback aligned_alloc implementation

libstdc++-v3:

PR libstdc++/79190
* libsupc++/del_opa.cc (operator delete(void*, std::align_val_t))
[!_GLIBCXX_HAVE_ALIGNED_ALLOC && !_GLIBCXX_HAVE_POSIX_MEMALIGN
&& !_GLIBCXX_HAVE_MEMALIGN && !_GLIBCXX_HAVE__ALIGNED_MALLOC]:
Retrieve original pointer value allocated by malloc.
* libsupc++/new_opa.cc [!_GLIBCXX_HAVE_ALIGNED_ALLOC
&& !_GLIBCXX_HAVE_POSIX_MEMALIGN && !_GLIBCXX_HAVE_MEMALIGN
&& !_GLIBCXX_HAVE__ALIGNED_MALLOC] (aligned_alloc(size_t, size_t)):
Define, adjusting pointer value allocated by malloc and storing for
retrieval by operator delete.

gcc/testsuite:

PR libstdc++/79190
* g++.dg/cpp1z/aligned-new3.C: Replace operator new so behaviour
matches replaced operator delete.

diff --git a/gcc/testsuite/g++.dg/cpp1z/aligned-new3.C 
b/gcc/testsuite/g++.dg/cpp1z/aligned-new3.C
index 73e3343..e50e62c 100644
--- a/gcc/testsuite/g++.dg/cpp1z/aligned-new3.C
+++ b/gcc/testsuite/g++.dg/cpp1z/aligned-new3.C
@@ -7,6 +7,11 @@ struct alignas(64) A {
   int i;
 };
 
+void* operator new (std::size_t n, std::align_val_t)
+{
+  return operator new (n);
+}
+
 bool deleted = false;
 void operator delete (void *p, std::size_t, std::align_val_t)
 {
diff --git a/libstdc++-v3/libsupc++/del_opa.cc 
b/libstdc++-v3/libsupc++/del_opa.cc
index c9afb46..f7bf7a4 100644
--- a/libstdc++-v3/libsupc++/del_opa.cc
+++ b/libstdc++-v3/libsupc++/del_opa.cc
@@ -46,9 +46,13 @@ _GLIBCXX_END_NAMESPACE_VERSION
 _GLIBCXX_WEAK_DEFINITION void
 operator delete(void* ptr, std::align_val_t) _GLIBCXX_USE_NOEXCEPT
 {
-#if !_GLIBCXX_HAVE_ALIGNED_ALLOC && _GLIBCXX_HAVE__ALIGNED_MALLOC
+#if _GLIBCXX_HAVE_ALIGNED_ALLOC || _GLIBCXX_HAVE_POSIX_MEMALIGN \
+|| _GLIBCXX_HAVE_MEMALIGN
+  std::free (ptr);
+#elif _GLIBCXX_HAVE__ALIGNED_MALLOC
   _aligned_free (ptr);
 #else
-  std::free(ptr);
+  if (ptr)
+std::free (((void **) ptr)[-1]); // See aligned_alloc in new_opa.cc
 #endif
 }
diff --git a/libstdc++-v3/libsupc++/new_opa.cc 
b/libstdc++-v3/libsupc++/new_opa.cc
index 0bc2b5f..5d6c185 100644
--- a/libstdc++-v3/libsupc++/new_opa.cc
+++ b/libstdc++-v3/libsupc++/new_opa.cc
@@ -55,9 +55,30 @@ extern "C" void *memalign(std::size_t boundary, std::size_t 
size);
 #endif
 #define aligned_alloc memalign
 #else
-// The C library doesn't provide any aligned allocation functions, declare
-// aligned_alloc and get a link failure if aligned new is used.
-extern "C" void *aligned_alloc(std::size_t, std::size_t);
+// This is a modified version of code from gcc/config/i386/gmm_malloc.h
+static inline void*
+aligned_alloc (std::size_t al, std::size_t sz)
+{
+  // Alignment must be a power of two.
+  if (al & (al - 1))
+return nullptr;
+  else if (!sz)
+return nullptr;
+
+  // We need extra bytes to store the original value returned by malloc.
+  if (al < sizeof(void*))
+al = sizeof(void*);
+  void* const malloc_ptr = malloc(sz + al);
+  if (!malloc_ptr)
+return nullptr;
+  // Align to the requested value, leaving room for the original malloc value.
+  void* const aligned_ptr = (void *) (((size_t) malloc_ptr + al) & -al);
+
+  // Store the original malloc value where it can be found by operator delete.
+  ((void **) aligned_ptr)[-1] = malloc_ptr;
+
+  return aligned_ptr;
+}
 #endif
 #endif