[Mesa-dev] i965: Use new/delete instead of realloc() in brw_ir_allocator.h

Submitted by Juha-Pekka Heikkila on Feb. 11, 2015, 2:56 p.m.

Details

Message ID 1423666616-28205-1-git-send-email-juhapekka.heikkila@gmail.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Juha-Pekka Heikkila Feb. 11, 2015, 2:56 p.m.
There is no error path available thus instead of giving
realloc possibility to fail use new which will never
return null pointer and throws bad_alloc on failure.

Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
 src/mesa/drivers/dri/i965/brw_ir_allocator.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_ir_allocator.h b/src/mesa/drivers/dri/i965/brw_ir_allocator.h
index b1237ed..5330bff 100644
--- a/src/mesa/drivers/dri/i965/brw_ir_allocator.h
+++ b/src/mesa/drivers/dri/i965/brw_ir_allocator.h
@@ -40,8 +40,8 @@  namespace brw {
 
       ~simple_allocator()
       {
-         free(offsets);
-         free(sizes);
+         delete[] offsets;
+         delete[] sizes;
       }
 
       unsigned
@@ -49,8 +49,16 @@  namespace brw {
       {
          if (capacity <= count) {
             capacity = MAX2(16, capacity * 2);
-            sizes = (unsigned *)realloc(sizes, capacity * sizeof(unsigned));
-            offsets = (unsigned *)realloc(offsets, capacity * sizeof(unsigned));
+
+            unsigned *tmp_sizes = new unsigned[capacity];
+            memcpy(tmp_sizes, sizes, count * sizeof(unsigned));
+            delete[] sizes;
+            sizes = tmp_sizes;
+
+            unsigned *tmp_offsets = new unsigned[capacity];
+            memcpy(tmp_offsets, offsets, count * sizeof(unsigned));
+            delete[] offsets;
+            offsets = tmp_offsets;
          }
 
          sizes[count] = size;

Comments

Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> writes:

> There is no error path available thus instead of giving
> realloc possibility to fail use new which will never
> return null pointer and throws bad_alloc on failure.
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  src/mesa/drivers/dri/i965/brw_ir_allocator.h | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_ir_allocator.h b/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> index b1237ed..5330bff 100644
> --- a/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> +++ b/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> @@ -40,8 +40,8 @@ namespace brw {
>  
>        ~simple_allocator()
>        {
> -         free(offsets);
> -         free(sizes);
> +         delete[] offsets;
> +         delete[] sizes;
>        }
>  
>        unsigned
> @@ -49,8 +49,16 @@ namespace brw {
>        {
>           if (capacity <= count) {
>              capacity = MAX2(16, capacity * 2);
> -            sizes = (unsigned *)realloc(sizes, capacity * sizeof(unsigned));
> -            offsets = (unsigned *)realloc(offsets, capacity * sizeof(unsigned));
> +
> +            unsigned *tmp_sizes = new unsigned[capacity];
> +            memcpy(tmp_sizes, sizes, count * sizeof(unsigned));
> +            delete[] sizes;
> +            sizes = tmp_sizes;
> +
> +            unsigned *tmp_offsets = new unsigned[capacity];
> +            memcpy(tmp_offsets, offsets, count * sizeof(unsigned));
> +            delete[] offsets;
> +            offsets = tmp_offsets;
>           }
>  
>           sizes[count] = size;
> -- 
> 1.8.5.1

Looks OK to me,
Reviewed-by: Francisco Jerez <currojerez@riseup.net>

>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Wednesday, February 11, 2015 04:56:56 PM Juha-Pekka Heikkila wrote:
> There is no error path available thus instead of giving
> realloc possibility to fail use new which will never
> return null pointer and throws bad_alloc on failure.
> 
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  src/mesa/drivers/dri/i965/brw_ir_allocator.h | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_ir_allocator.h b/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> index b1237ed..5330bff 100644
> --- a/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> +++ b/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> @@ -40,8 +40,8 @@ namespace brw {
>  
>        ~simple_allocator()
>        {
> -         free(offsets);
> -         free(sizes);
> +         delete[] offsets;
> +         delete[] sizes;
>        }
>  
>        unsigned
> @@ -49,8 +49,16 @@ namespace brw {
>        {
>           if (capacity <= count) {
>              capacity = MAX2(16, capacity * 2);
> -            sizes = (unsigned *)realloc(sizes, capacity * sizeof(unsigned));
> -            offsets = (unsigned *)realloc(offsets, capacity * sizeof(unsigned));
> +
> +            unsigned *tmp_sizes = new unsigned[capacity];
> +            memcpy(tmp_sizes, sizes, count * sizeof(unsigned));
> +            delete[] sizes;
> +            sizes = tmp_sizes;
> +
> +            unsigned *tmp_offsets = new unsigned[capacity];
> +            memcpy(tmp_offsets, offsets, count * sizeof(unsigned));
> +            delete[] offsets;
> +            offsets = tmp_offsets;
>           }
>  
>           sizes[count] = size;
> 

Okay, I'll go ahead and say what Matt and Jason were probably thinking:

NAK.

I'm not against using new/delete in general, but using realloc to double
the size of arrays is a pattern we use all over the codebase.  I don't
see any reason not to use that same pattern here.  Plus, the old code
was 2 lines of code...and now it's 8 lines of code, for no real benefit.

Most idiomatic C++ code I've seen would just some kind of flexible array
class, like std::vector, rather than open coding this.  So, we're
already divergent from C++ best practices, and instead are following the
typical C idiom.  This is neither.

In fact, your new code is more likely to fail than the old one: instead
of growing the array from size N to size 2*N---which realloc may be able
to do in-place---you're briefly keeping both around, using N+2*N=3*N
space.  If realloc failed, this would absolutely fail too.

If we compiled without exceptions, this would crash in an identical
manner - no actual error handling is done here.  With exceptions, it's
not much better - we throw an exception all the way outside of libGL and
back to the application (since we sure don't handle those), leaving our
work in some unknown state.

Plus, as has been repeatedly mentioned - malloc doesn't fail if you're
out of memory - it only fails if you're out of virtual address space.

--Ken
Hi,

On 02/11/2015 08:45 PM, Kenneth Graunke wrote:
> On Wednesday, February 11, 2015 04:56:56 PM Juha-Pekka Heikkila wrote:
>> There is no error path available thus instead of giving
>> realloc possibility to fail use new which will never
>> return null pointer and throws bad_alloc on failure.
...
> Okay, I'll go ahead and say what Matt and Jason were probably thinking:
>
> NAK.
>
> I'm not against using new/delete in general, but using realloc to double
> the size of arrays is a pattern we use all over the codebase.  I don't
> see any reason not to use that same pattern here.  Plus, the old code
> was 2 lines of code...and now it's 8 lines of code, for no real benefit.
>
> Most idiomatic C++ code I've seen would just some kind of flexible array
> class, like std::vector, rather than open coding this.  So, we're
> already divergent from C++ best practices, and instead are following the
> typical C idiom.  This is neither.
>
> In fact, your new code is more likely to fail than the old one: instead
> of growing the array from size N to size 2*N---which realloc may be able
> to do in-place---you're briefly keeping both around, using N+2*N=3*N
> space.  If realloc failed, this would absolutely fail too.
>
> If we compiled without exceptions, this would crash in an identical
> manner - no actual error handling is done here.  With exceptions, it's
> not much better - we throw an exception all the way outside of libGL and
> back to the application (since we sure don't handle those), leaving our
> work in some unknown state.

Qt had at least earlier nasty habit of re-throwing uncaught exceptions
in its main loop, which completely hid the cause for a random
application crash.  There may be other SW packages that do similarly
silly things.  Core dump at place where the problem first occurs is
IMHO much nicer.


> Plus, as has been repeatedly mentioned - malloc doesn't fail if you're
> out of memory - it only fails if you're out of virtual address space.

A valid 3D use-case for that is Steam games.  Majority of them are
32-bit only, so they can run out of memory & address space without
system being low on memory.


	- Eero

PS. In previous life I bumped into another, funny use-case for that,
programs that leak joinable threads (don't join them after exit).

We were wondering how programs could generate GB sized core dumps on
128MB device, until we noticed that they run out of 32-bit address
space because they had too many threads...  Default Linux thread stack
size (8MB) was quickly lowered after that and whole system tracking
added for thread leakage. :-)