Re: [RFA/ARI fix] Remove use of abort function in common/buffer.c

Re: [RFA/ARI fix] Remove use of abort function in common/buffer.c

[Pedro Alves]

On 05/22/2012 10:17 AM, Pierre Muller wrote:

>   ARI clearly states that abort function
> should never be used, and to use internal_error
> instead.
> 
>   I first suspected that this was related to missing declarations in 
> common directory, but internal_error does
> exist there also.
> 
>   Finally, looking at common/buffer.c source, I found that
> gdb_assert was probably a better fit here
> (the macro does call internal_error).
> 
> 
>   Is this patch OK or what else should we use here?


We should just remove the abort; it's dead code.  xrealloc already handles
that for us.  common/common-utils.c:xrealloc:

...
  /* (...) It never returns NULL.  */
...
  if (val == NULL)
    malloc_failure (size);

  return val;
}

-- 
Pedro Alves

[RFA-v2/ARI fix] Remove use of abort function in common/buffer.c

[Pierre Muller]

> We should just remove the abort; it's dead code.  xrealloc already handles
> that for us.  common/common-utils.c:xrealloc:

 Pedro, you are right of course...
Here is an update that just removes
the call to abort, and added a small
comment to explain.

Is this OK?

Pierre Muller
GDB pascal language maintainer



2012-05-22  Pierre Muller  <muller@ics.u-strasbg.fr>

        * common/buffer.c (buffer_grow): ARI fix: Remove unneeded
        call to abort.

Index: common/buffer.c
===================================================================
RCS file: /cvs/src/src/gdb/common/buffer.c,v
retrieving revision 1.3
diff -u -p -r1.3 buffer.c
--- common/buffer.c     11 May 2012 22:24:22 -0000      1.3
+++ common/buffer.c     22 May 2012 12:02:18 -0000
@@ -47,8 +47,8 @@ buffer_grow (struct buffer *buffer, cons
   while (buffer->used_size + size > new_buffer_size)
     new_buffer_size *= 2;
   new_buffer = xrealloc (buffer->buffer, new_buffer_size);
-  if (!new_buffer)
-    abort ();
+  /* new_buffer is non NULL otherwise
+     xrealloc calls malloc_failure which does not return here.  */
   memcpy (new_buffer + buffer->used_size, data, size);
   buffer->buffer = new_buffer;
   buffer->buffer_size = new_buffer_size;

Re: [RFA-v2/ARI fix] Remove use of abort function in common/buffer.c

[Joel Brobecker]

Hi Pierre, Hi Pedro,

Thanks for taking care of this, Pierre.

> +  /* new_buffer is non NULL otherwise
> +     xrealloc calls malloc_failure which does not return here.  */

I suggest a slightly different way of saying this:

    /* xrealloc guaranties that new_buffer cannot be null.  */

It avoids the problem with the location of the line break and any issue
with punctuation...

-- 
Joel

Re: [RFA-v2/ARI fix] Remove use of abort function in common/buffer.c

[Pedro Alves]

On 05/22/2012 01:18 PM, Joel Brobecker wrote:

> Hi Pierre, Hi Pedro,
> 
> Thanks for taking care of this, Pierre.
> 
>> +  /* new_buffer is non NULL otherwise
>> +     xrealloc calls malloc_failure which does not return here.  */
> 
> I suggest a slightly different way of saying this:
> 
>     /* xrealloc guaranties that new_buffer cannot be null.  */
> 
> It avoids the problem with the location of the line break and any issue
> with punctuation...
> 


Fine with me, but I'd suggest to just say nothing.  The whole point of the xrealloc,
xmalloc, etc. functions is to abort on failure instead of returning NULL.  We have
about 150 or so xrealloc calls in the tree, about 70 xcalloc calls, and
about 700 xmalloc calls.  We don't put just comment on most (any?) of those, and
I don't see what makes this particular instance special.

-- 
Pedro Alves

[RFA-v3/ARI fix] Remove use of abort function in common/buffer.c

[Pierre Muller]

> Fine with me, but I'd suggest to just say nothing.  The whole point of the
> xrealloc,
> xmalloc, etc. functions is to abort on failure instead of returning NULL.
> We have
> about 150 or so xrealloc calls in the tree, about 70 xcalloc calls, and
> about 700 xmalloc calls.  We don't put just comment on most (any?) of
those,
> and
> I don't see what makes this particular instance special.

  You are right,
OK for that even simpler patch?


2012-05-22  Pierre Muller  <muller@ics.u-strasbg.fr>

        * common/buffer.c (buffer_grow): ARI fix: Remove unneeded
        call to abort.

Index: common/buffer.c
===================================================================
RCS file: /cvs/src/src/gdb/common/buffer.c,v
retrieving revision 1.3
diff -u -p -r1.3 buffer.c
--- common/buffer.c     11 May 2012 22:24:22 -0000      1.3
+++ common/buffer.c     22 May 2012 15:13:24 -0000
@@ -47,8 +47,6 @@ buffer_grow (struct buffer *buffer, cons
   while (buffer->used_size + size > new_buffer_size)
     new_buffer_size *= 2;
   new_buffer = xrealloc (buffer->buffer, new_buffer_size);
-  if (!new_buffer)
-    abort ();
   memcpy (new_buffer + buffer->used_size, data, size);
   buffer->buffer = new_buffer;
   buffer->buffer_size = new_buffer_size;

Re: [RFA-v3/ARI fix] Remove use of abort function in common/buffer.c

[Pedro Alves]

On 05/22/2012 04:14 PM, Pierre Muller wrote:

> OK for that even simpler patch?


OK.  Thanks.

-- 
Pedro Alves

RE: [RFA-v3/ARI fix] Remove use of abort function in common/buffer.c

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : mardi 22 mai 2012 17:19
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA-v3/ARI fix] Remove use of abort function in
common/buffer.c
> 
> On 05/22/2012 04:14 PM, Pierre Muller wrote:
> 
> > OK for that even simpler patch?
> 
> 
> OK.  Thanks.

  Thanks for the feedback,
patch committed
http://sourceware.org/ml/gdb-cvs/2012-05/msg00164.html


Pierre