[FFmpeg-devel,1/3] avutil/mem: Fix *realloc() alignment with memalign hack

Submitted by Michael Niedermayer on Nov. 21, 2016, 3:18 p.m.

Details

Message ID 20161121151858.703-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Nov. 21, 2016, 3:18 p.m.
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavutil/mem.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

Comments

wm4 Nov. 21, 2016, 3:34 p.m.
On Mon, 21 Nov 2016 16:18:56 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavutil/mem.c | 44 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 

"Fix" is not a good description. Was it broken? What was broken?

Also, I thought the memalign hack was generally not needed anymore.
Which platforms still need it?

> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index 1a8fc21..6273d89 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -86,12 +86,15 @@ void *av_malloc(size_t size)
>          return NULL;
>  
>  #if CONFIG_MEMALIGN_HACK
> -    ptr = malloc(size + ALIGN);
> +    ptr = malloc(size + ALIGN + 8);
>      if (!ptr)
>          return ptr;
> -    diff              = ((~(long)ptr)&(ALIGN - 1)) + 1;
> +
> +    diff = ((((uintptr_t)ptr) + 9 + ALIGN - 1) & ~(ALIGN - 1)) - (uintptr_t)ptr;
> +    av_assert0(diff>0 && diff<=ALIGN + 8);

Why 9?

>      ptr               = (char *)ptr + diff;
> -    ((char *)ptr)[-1] = diff;
> +    ((char *)ptr)[-9] = diff;
> +    ((int64_t *)ptr)[-1] = size;
>  #elif HAVE_POSIX_MEMALIGN
>      if (size) //OS X on SDK 10.6 has a broken posix_memalign implementation
>      if (posix_memalign(&ptr, ALIGN, size))
> @@ -146,6 +149,7 @@ void *av_realloc(void *ptr, size_t size)
>  {
>  #if CONFIG_MEMALIGN_HACK
>      int diff;
> +    void *ptr2;
>  #endif
>  
>      /* let's disallow possibly ambiguous cases */
> @@ -153,15 +157,31 @@ void *av_realloc(void *ptr, size_t size)
>          return NULL;
>  
>  #if CONFIG_MEMALIGN_HACK
> -    //FIXME this isn't aligned correctly, though it probably isn't needed
>      if (!ptr)
>          return av_malloc(size);
> -    diff = ((char *)ptr)[-1];
> -    av_assert0(diff>0 && diff<=ALIGN);  
> -    ptr = realloc((char *)ptr - diff, size + diff);
> -    if (ptr)
> -        ptr = (char *)ptr + diff;
> -    return ptr;
> +    diff = ((char *)ptr)[-9];
> +    av_assert0(diff>0 && diff<=ALIGN + 8);
> +
> +    // Quick path for apparently and likly aligned realloc()
> +    if (diff == ALIGN) {
> +        ptr = realloc((char *)ptr - diff, size + diff);
> +        if (ptr)
> +            ptr = (char *)ptr + diff;
> +        if (!(((uintptr_t)ptr) & (ALIGN - 1))) {
> +            if (ptr)
> +                ((int64_t *)ptr)[-1] = size;
> +            return ptr;
> +        }
> +    }
> +
> +    ptr2 = av_malloc(size);
> +    if (!ptr2)
> +        return NULL;
> +
> +    memcpy(ptr2, ptr, FFMIN(((int64_t *)ptr)[-1], size));
> +    ((int64_t *)ptr2)[-1] = size;
> +    av_free(ptr);
> +    return ptr2;

I thought av_realloc() never keeps alignment. It only needs to be sure
to not choke on the alignment hack, since calling av_realloc() on a
av_malloc() allocated block is still ok, even if it can lose the
alignment.

>  #elif HAVE_ALIGNED_MALLOC
>      return _aligned_realloc(ptr, size + !size, ALIGN);
>  #else
> @@ -229,8 +249,8 @@ void av_free(void *ptr)
>  {
>  #if CONFIG_MEMALIGN_HACK
>      if (ptr) {
> -        int v= ((char *)ptr)[-1];
> -        av_assert0(v>0 && v<=ALIGN);  
> +        int v= ((char *)ptr)[-9];
> +        av_assert0(v>0 && v<=ALIGN + 8);
>          free((char *)ptr - v);
>      }
>  #elif HAVE_ALIGNED_MALLOC
Michael Niedermayer Nov. 21, 2016, 6:30 p.m.
On Mon, Nov 21, 2016 at 04:34:54PM +0100, wm4 wrote:
> On Mon, 21 Nov 2016 16:18:56 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavutil/mem.c | 44 ++++++++++++++++++++++++++++++++------------
> >  1 file changed, 32 insertions(+), 12 deletions(-)
> > 
> 
> "Fix" is not a good description. Was it broken? What was broken?
> 

well
there was a "FIXME" in there about the realloc alignment
and as the heap limit i wanted to implement needed very similar changes
i fixed that. I did remember that at some point in the past we wanted to
have this codepath align correctly.

Maybe the comment mislead me
still, this code could come in handy for debuging and tracing things
as it allows debug code to access the allocated size of heap
allocations easily
also as its already written, someone might want to check if it has any
speed effect to have misaligned output from realloc()


> Also, I thought the memalign hack was generally not needed anymore.
> Which platforms still need it?

i dont know, iam not a user of an affected platform

[...]

Patch hide | download patch | download mbox

diff --git a/libavutil/mem.c b/libavutil/mem.c
index 1a8fc21..6273d89 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -86,12 +86,15 @@  void *av_malloc(size_t size)
         return NULL;
 
 #if CONFIG_MEMALIGN_HACK
-    ptr = malloc(size + ALIGN);
+    ptr = malloc(size + ALIGN + 8);
     if (!ptr)
         return ptr;
-    diff              = ((~(long)ptr)&(ALIGN - 1)) + 1;
+
+    diff = ((((uintptr_t)ptr) + 9 + ALIGN - 1) & ~(ALIGN - 1)) - (uintptr_t)ptr;
+    av_assert0(diff>0 && diff<=ALIGN + 8);
     ptr               = (char *)ptr + diff;
-    ((char *)ptr)[-1] = diff;
+    ((char *)ptr)[-9] = diff;
+    ((int64_t *)ptr)[-1] = size;
 #elif HAVE_POSIX_MEMALIGN
     if (size) //OS X on SDK 10.6 has a broken posix_memalign implementation
     if (posix_memalign(&ptr, ALIGN, size))
@@ -146,6 +149,7 @@  void *av_realloc(void *ptr, size_t size)
 {
 #if CONFIG_MEMALIGN_HACK
     int diff;
+    void *ptr2;
 #endif
 
     /* let's disallow possibly ambiguous cases */
@@ -153,15 +157,31 @@  void *av_realloc(void *ptr, size_t size)
         return NULL;
 
 #if CONFIG_MEMALIGN_HACK
-    //FIXME this isn't aligned correctly, though it probably isn't needed
     if (!ptr)
         return av_malloc(size);
-    diff = ((char *)ptr)[-1];
-    av_assert0(diff>0 && diff<=ALIGN);
-    ptr = realloc((char *)ptr - diff, size + diff);
-    if (ptr)
-        ptr = (char *)ptr + diff;
-    return ptr;
+    diff = ((char *)ptr)[-9];
+    av_assert0(diff>0 && diff<=ALIGN + 8);
+
+    // Quick path for apparently and likly aligned realloc()
+    if (diff == ALIGN) {
+        ptr = realloc((char *)ptr - diff, size + diff);
+        if (ptr)
+            ptr = (char *)ptr + diff;
+        if (!(((uintptr_t)ptr) & (ALIGN - 1))) {
+            if (ptr)
+                ((int64_t *)ptr)[-1] = size;
+            return ptr;
+        }
+    }
+
+    ptr2 = av_malloc(size);
+    if (!ptr2)
+        return NULL;
+
+    memcpy(ptr2, ptr, FFMIN(((int64_t *)ptr)[-1], size));
+    ((int64_t *)ptr2)[-1] = size;
+    av_free(ptr);
+    return ptr2;
 #elif HAVE_ALIGNED_MALLOC
     return _aligned_realloc(ptr, size + !size, ALIGN);
 #else
@@ -229,8 +249,8 @@  void av_free(void *ptr)
 {
 #if CONFIG_MEMALIGN_HACK
     if (ptr) {
-        int v= ((char *)ptr)[-1];
-        av_assert0(v>0 && v<=ALIGN);
+        int v= ((char *)ptr)[-9];
+        av_assert0(v>0 && v<=ALIGN + 8);
         free((char *)ptr - v);
     }
 #elif HAVE_ALIGNED_MALLOC