diff mbox

malloc/obstack: Merge obstack code back from gnulib

Message ID 1396004022-30929-1-git-send-email-will.newton@linaro.org
State Superseded
Headers show

Commit Message

Will Newton March 28, 2014, 10:53 a.m. UTC 
Merge the gnulib obstack code back from gnulib. This is mostly
cosmetic issues but also includes a fix for a -Wundef warning.
The only change made was to replace _Noreturn with
__attribute__ ((__noreturn__)).

2014-03-28  Will Newton  <will.newton@linaro.org>

	* malloc/obstack.c: Merge code from gnulib.
	* malloc/obstack.h: Likewise.
---
 malloc/obstack.c | 71 ++++++++++++++++++++++----------------------------------
 malloc/obstack.h | 62 ++++++++++++++++++++++++++++---------------------
 2 files changed, 64 insertions(+), 69 deletions(-)

Comments

Joseph Myers March 28, 2014, 9:11 p.m. UTC | #1 
On Fri, 28 Mar 2014, Paul Eggert wrote:

> On 03/28/2014 03:53 AM, Will Newton wrote:
> > The only change made was to replace _Noreturn with __attribute__
> > ((__noreturn__)).
> 
> It's better to keep the sources identical when possible.  Gnulib uses
> _Noreturn with the C11 meaning, and libc should move in that direction too.
> Attached is a revised patch to get the ball rolling on that.

I don't think we should #define _Noreturn in the case of GCC >= 4.7, when 
it's supported by GCC inside and outside C11 mode.

If we move to using _Noreturn (directly or indirectly via some other 
macro) in installed headers - logically right for supporting generic C11 
compilers, although I don't think many people are likely to be using the 
headers with compilers that don't define __GNUC__ - then there are the 
issues described in 
<https://sourceware.org/ml/libc-alpha/2012-01/msg00084.html> with where it 
can be placed in header declarations without either breaking use of the 
headers with GCC 2.7 or causing such use to lose the information that 
functions don't return.
Roland McGrath March 29, 2014, 12:52 a.m. UTC | #2 
As a general rule, we want each change described fully in the ChangeLog and
subject to our independent review.  We have to be this anal because we have
some issues to worry about that gnulib doesn't (and of course vice versa).
It's certainly worthwhile to mention at the top of the log paragraph that
you're sync'ing up with gnulib, but the individual changes need to be
described (and reviewed fully).

These particular changes look mostly quite harmless.  But the principle
stands.  The ones that are literally cosmetic (changing comments and
formatting) can be logged trivially (no need to cite every function in the
normal way, for example).  Adding attributes and such, while also harmless
in the cases I saw, needs to be logged explicitly.  Renaming functions
absolutely must be logged--most especially for globals, and that is most
likely not at all harmless and needs both careful review and your testimony
that you tested the changes in a libc build including 'make check' (start
with the quicker 'make check-abi' to confirm or refute my suspicion about
why the change might be bad before wading through 'make check' and
identifying all possible regressions).


Thanks,
Roland
Will Newton April 1, 2014, 1:21 p.m. UTC | #3 
On 28 March 2014 21:42, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 03/28/2014 02:11 PM, Joseph S. Myers wrote:
>>
>> I don't think we should #define _Noreturn in the case of GCC >= 4.7
>
>
> Thanks, revised patches attached.  The second one's unaffected.

The first patch is liable to cause -Wundef warnings as
__STDC_VERSION__ is not defined by default by gcc. I guess it could
also use a comment as the conditional is non-trivial.
Will Newton April 1, 2014, 3:22 p.m. UTC | #4 
On 29 March 2014 00:52, Roland McGrath <roland@hack.frob.com> wrote:
> As a general rule, we want each change described fully in the ChangeLog and
> subject to our independent review.  We have to be this anal because we have
> some issues to worry about that gnulib doesn't (and of course vice versa).
> It's certainly worthwhile to mention at the top of the log paragraph that
> you're sync'ing up with gnulib, but the individual changes need to be
> described (and reviewed fully).
>
> These particular changes look mostly quite harmless.  But the principle
> stands.  The ones that are literally cosmetic (changing comments and
> formatting) can be logged trivially (no need to cite every function in the
> normal way, for example).  Adding attributes and such, while also harmless
> in the cases I saw, needs to be logged explicitly.  Renaming functions
> absolutely must be logged--most especially for globals, and that is most
> likely not at all harmless and needs both careful review and your testimony
> that you tested the changes in a libc build including 'make check' (start
> with the quicker 'make check-abi' to confirm or refute my suspicion about
> why the change might be bad before wading through 'make check' and
> identifying all possible regressions).

I tried make check-abi with Paul's most recent set of patches and it
appears to pass successfully on x86_64.

I also did a make check and the failures don't appear to be obviously
related to these changes:

FAIL: elf/tst-dlopen-aout
FAIL: resolv/tst-leaks
Summary of test results:
      2 FAIL
   1702 PASS
    122 XFAIL
      3 XPASS

Is there a specific concern you have about this patch besides the
noted issue of the changelog?

Thanks,
Roland McGrath April 1, 2014, 4:50 p.m. UTC | #5 
> Is there a specific concern you have about this patch besides the
> noted issue of the changelog?

The renaming of obstack_free looked like a problem, but I had not noticed
the macro hackery that undoes it.  Since it passed check-abi, that is not
an actual problem.
Ondřej Bílka May 18, 2014, 10:33 a.m. UTC | #6 
On Fri, Mar 28, 2014 at 10:53:42AM +0000, Will Newton wrote:
> Merge the gnulib obstack code back from gnulib. This is mostly
> cosmetic issues but also includes a fix for a -Wundef warning.
> The only change made was to replace _Noreturn with
> __attribute__ ((__noreturn__)).
> 
Hi, it looks this patch waits only for new changelog. Could you write
it?
diff mbox

Patch

diff --git a/malloc/obstack.c b/malloc/obstack.c
index 4064f61..82937bb 100644
--- a/malloc/obstack.c
+++ b/malloc/obstack.c
@@ -17,14 +17,11 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 
-#ifdef HAVE_CONFIG_H
-# include <config.h>
-#endif
-
 #ifdef _LIBC
 # include <obstack.h>
 # include <shlib-compat.h>
 #else
+# include <config.h>
 # include "obstack.h"
 #endif
 
@@ -39,7 +36,7 @@ 
    C Library, but also included in many other GNU distributions.  Compiling
    and linking in this code is a waste when using the GNU C library
    (especially if it is a shared library).  Rather than having every GNU
-   program understand `configure --with-gnu-libc' and omit the object
+   program understand 'configure --with-gnu-libc' and omit the object
    files, it is simpler to just do this in the source for each such file.  */
 
 #include <stdio.h>              /* Random thing to get __GNU_LIBRARY__.  */
@@ -55,12 +52,7 @@ 
 #ifndef ELIDE_CODE
 
 
-# if HAVE_INTTYPES_H
-#  include <inttypes.h>
-# endif
-# if HAVE_STDINT_H || defined _LIBC
-#  include <stdint.h>
-# endif
+# include <stdint.h>
 
 /* Determine default alignment.  */
 union fooround
@@ -85,23 +77,23 @@  enum
 
 /* When we copy a long block of data, this is the unit to do it with.
    On some machines, copying successive ints does not work;
-   in such a case, redefine COPYING_UNIT to `long' (if that works)
-   or `char' as a last resort.  */
+   in such a case, redefine COPYING_UNIT to 'long' (if that works)
+   or 'char' as a last resort.  */
 # ifndef COPYING_UNIT
 #  define COPYING_UNIT int
 # endif
 
 
-/* The functions allocating more room by calling `obstack_chunk_alloc'
-   jump to the handler pointed to by `obstack_alloc_failed_handler'.
+/* The functions allocating more room by calling 'obstack_chunk_alloc'
+   jump to the handler pointed to by 'obstack_alloc_failed_handler'.
    This can be set to a user defined function which should either
    abort gracefully or use longjump - but shouldn't return.  This
    variable by default points to the internal function
-   `print_and_abort'.  */
-static void print_and_abort (void);
+   'print_and_abort'.  */
+static __attribute__ ((__noreturn__)) void print_and_abort (void);
 void (*obstack_alloc_failed_handler) (void) = print_and_abort;
 
-/* Exit value used when `print_and_abort' is used.  */
+/* Exit value used when 'print_and_abort' is used.  */
 # include <stdlib.h>
 # ifdef _LIBC
 int obstack_exit_failure = EXIT_FAILURE;
@@ -151,15 +143,15 @@  compat_symbol (libc, _obstack_compat, _obstack, GLIBC_2_0);
 int
 _obstack_begin (struct obstack *h,
                 int size, int alignment,
-                void *(*chunkfun)(long),
-                void (*freefun)(void *))
+                void *(*chunkfun) (long),
+                void (*freefun) (void *))
 {
   struct _obstack_chunk *chunk; /* points to new chunk */
 
   if (alignment == 0)
     alignment = DEFAULT_ALIGNMENT;
   if (size == 0)
-  /* Default size is what GNU malloc can fit in a 4096-byte block.  */
+    /* Default size is what GNU malloc can fit in a 4096-byte block.  */
     {
       /* 12 is sizeof (mhead) and 4 is EXTRA from GNU malloc.
          Use the values for range checking, because if range checking is off,
@@ -175,15 +167,15 @@  _obstack_begin (struct obstack *h,
       size = 4096 - extra;
     }
 
-  h->chunkfun = (struct _obstack_chunk * (*)(void *, long))chunkfun;
-  h->freefun = (void (*)(void *, struct _obstack_chunk *))freefun;
+  h->chunkfun = (struct _obstack_chunk * (*) (void *, long)) chunkfun;
+  h->freefun = (void (*) (void *, struct _obstack_chunk *)) freefun;
   h->chunk_size = size;
   h->alignment_mask = alignment - 1;
   h->use_extra_arg = 0;
 
   chunk = h->chunk = CALL_CHUNKFUN (h, h->chunk_size);
   if (!chunk)
-    (*obstack_alloc_failed_handler)();
+    (*obstack_alloc_failed_handler) ();
   h->next_free = h->object_base = __PTR_ALIGN ((char *) chunk, chunk->contents,
                                                alignment - 1);
   h->chunk_limit = chunk->limit
@@ -197,8 +189,8 @@  _obstack_begin (struct obstack *h,
 
 int
 _obstack_begin_1 (struct obstack *h, int size, int alignment,
-                  void *(*chunkfun)(void *, long),
-                  void (*freefun)(void *, void *),
+                  void *(*chunkfun) (void *, long),
+                  void (*freefun) (void *, void *),
                   void *arg)
 {
   struct _obstack_chunk *chunk; /* points to new chunk */
@@ -206,7 +198,7 @@  _obstack_begin_1 (struct obstack *h, int size, int alignment,
   if (alignment == 0)
     alignment = DEFAULT_ALIGNMENT;
   if (size == 0)
-  /* Default size is what GNU malloc can fit in a 4096-byte block.  */
+    /* Default size is what GNU malloc can fit in a 4096-byte block.  */
     {
       /* 12 is sizeof (mhead) and 4 is EXTRA from GNU malloc.
          Use the values for range checking, because if range checking is off,
@@ -222,8 +214,8 @@  _obstack_begin_1 (struct obstack *h, int size, int alignment,
       size = 4096 - extra;
     }
 
-  h->chunkfun = (struct _obstack_chunk * (*)(void *, long))chunkfun;
-  h->freefun = (void (*)(void *, struct _obstack_chunk *))freefun;
+  h->chunkfun = (struct _obstack_chunk * (*)(void *,long)) chunkfun;
+  h->freefun = (void (*) (void *, struct _obstack_chunk *)) freefun;
   h->chunk_size = size;
   h->alignment_mask = alignment - 1;
   h->extra_arg = arg;
@@ -231,7 +223,7 @@  _obstack_begin_1 (struct obstack *h, int size, int alignment,
 
   chunk = h->chunk = CALL_CHUNKFUN (h, h->chunk_size);
   if (!chunk)
-    (*obstack_alloc_failed_handler)();
+    (*obstack_alloc_failed_handler) ();
   h->next_free = h->object_base = __PTR_ALIGN ((char *) chunk, chunk->contents,
                                                alignment - 1);
   h->chunk_limit = chunk->limit
@@ -324,7 +316,7 @@  libc_hidden_def (_obstack_newchunk)
 
 /* Suppress -Wmissing-prototypes warning.  We don't want to declare this in
    obstack.h because it is just for debugging.  */
-int _obstack_allocated_p (struct obstack *h, void *obj);
+int _obstack_allocated_p (struct obstack *h, void *obj) __attribute_pure__;
 
 int
 _obstack_allocated_p (struct obstack *h, void *obj)
@@ -350,7 +342,7 @@  _obstack_allocated_p (struct obstack *h, void *obj)
 # undef obstack_free
 
 void
-obstack_free (struct obstack *h, void *obj)
+__obstack_free (struct obstack *h, void *obj)
 {
   struct _obstack_chunk *lp;    /* below addr of any objects in this chunk */
   struct _obstack_chunk *plp;   /* point to previous chunk if any */
@@ -412,15 +404,7 @@  _obstack_memory_used (struct obstack *h)
 #  include <libio/iolibio.h>
 # endif
 
-# ifndef __attribute__
-/* This feature is available in gcc versions 2.5 and later.  */
-#  if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5)
-#   define __attribute__(Spec) /* empty */
-#  endif
-# endif
-
-static void
-__attribute__ ((noreturn))
+static __attribute__ ((__noreturn__)) void
 print_and_abort (void)
 {
   /* Don't change any of these strings.  Yes, it would be possible to add
@@ -429,10 +413,11 @@  print_and_abort (void)
      like this and the translation should be reused instead of creating
      a very similar string which requires a separate translation.  */
 # ifdef _LIBC
-  (void) __fxprintf (NULL, "%s\n", _ ("memory exhausted"));
+  (void) __fxprintf (NULL, "%s\n", _("memory exhausted"));
 # else
-  fprintf (stderr, "%s\n", _ ("memory exhausted"));
+  fprintf (stderr, "%s\n", _("memory exhausted"));
 # endif
   exit (obstack_exit_failure);
 }
+
 #endif  /* !ELIDE_CODE */
diff --git a/malloc/obstack.h b/malloc/obstack.h
index 85472f9..f92492f 100644
--- a/malloc/obstack.h
+++ b/malloc/obstack.h
@@ -32,11 +32,11 @@ 
    stack is of mature, fixed size and fixed address objects.
 
    These routines grab large chunks of memory, using a function you
-   supply, called `obstack_chunk_alloc'.  On occasion, they free chunks,
-   by calling `obstack_chunk_free'.  You must define them and declare
+   supply, called 'obstack_chunk_alloc'.  On occasion, they free chunks,
+   by calling 'obstack_chunk_free'.  You must define them and declare
    them before using any obstack macros.
 
-   Each independent stack is represented by a `struct obstack'.
+   Each independent stack is represented by a 'struct obstack'.
    Each of the obstack macros expects a pointer to such a structure
    as the first argument.
 
@@ -93,7 +93,7 @@ 
         Exactly one object is growing in an obstack at any one time.
         You can run one obstack per control block.
         You may have as many control blocks as you dare.
-        Because of the way we do it, you can `unwind' an obstack
+        Because of the way we do it, you can "unwind" an obstack
           back to a previous state. (You may remove objects much
           as you would with a stack.)
  */
@@ -104,10 +104,6 @@ 
 #ifndef _OBSTACK_H
 #define _OBSTACK_H 1
 
-#ifdef __cplusplus
-extern "C" {
-#endif
-
 /* We need the type of a pointer subtraction.  If __PTRDIFF_TYPE__ is
    defined, as with GNU C, use that; that way we don't pollute the
    namespace with <stddef.h>'s symbols.  Otherwise, include <stddef.h>
@@ -124,9 +120,9 @@  extern "C" {
    aligning P to the next multiple of A + 1.  B and P must be of type
    char *.  A + 1 must be a power of 2.  */
 
-#define __BPTR_ALIGN(B, P, A) ((B) + (((P) -(B) + (A)) & ~(A)))
+#define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))
 
-/* Similiar to _BPTR_ALIGN (B, P, A), except optimize the common case
+/* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case
    where pointers can be converted to integers, aligned as integers,
    and converted back again.  If PTR_INT_TYPE is narrower than a
    pointer (e.g., the AS/400), play it safe and compute the alignment
@@ -139,6 +135,14 @@  extern "C" {
 
 #include <string.h>
 
+#ifndef __attribute_pure__
+# define __attribute_pure__ _GL_ATTRIBUTE_PURE
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 struct _obstack_chunk           /* Lives at front of each chunk. */
 {
   char *limit;                  /* 1 past end of this chunk */
@@ -159,7 +163,7 @@  struct obstack          /* control current object in current chunk */
     void *tempptr;
   } temp;                       /* Temporary for some macros.  */
   int alignment_mask;           /* Mask of alignment for each object. */
-  /* These prototypes vary based on `use_extra_arg', and we use
+  /* These prototypes vary based on 'use_extra_arg', and we use
      casts to the prototypeless function type in all assignments,
      but having prototypes here quiets -Wstrict-prototypes.  */
   struct _obstack_chunk *(*chunkfun) (void *, long);
@@ -183,18 +187,23 @@  extern int _obstack_begin (struct obstack *, int, int,
 extern int _obstack_begin_1 (struct obstack *, int, int,
                              void *(*)(void *, long),
                              void (*)(void *, void *), void *);
-extern int _obstack_memory_used (struct obstack *);
+extern int _obstack_memory_used (struct obstack *) __attribute_pure__;
 
-void obstack_free (struct obstack *__obstack, void *__glibc_block);
+/* The default name of the function for freeing a chunk is 'obstack_free',
+   but gnulib users can override this by defining '__obstack_free'.  */
+#ifndef __obstack_free
+# define __obstack_free obstack_free
+#endif
+extern void __obstack_free (struct obstack *, void *);
 
 
-/* Error handler called when `obstack_chunk_alloc' failed to allocate
+/* Error handler called when 'obstack_chunk_alloc' failed to allocate
    more memory.  This can be set to a user defined function which
    should either abort gracefully or use longjump - but shouldn't
    return.  The default action is to print a message and abort.  */
 extern void (*obstack_alloc_failed_handler) (void);
 
-/* Exit value used when `print_and_abort' is used.  */
+/* Exit value used when 'print_and_abort' is used.  */
 extern int obstack_exit_failure;
 
 /* Pointer to beginning of object being allocated or to be allocated next.
@@ -249,17 +258,14 @@  extern int obstack_exit_failure;
 #define obstack_memory_used(h) _obstack_memory_used (h)
 
 #if defined __GNUC__
-/* NextStep 2.0 cc is really gcc 1.93 but it defines __GNUC__ = 2 and
-   does not implement __extension__.  But that compiler doesn't define
-   __GNUC_MINOR__.  */
-# if __GNUC__ < 2 || (__NeXT__ && !__GNUC_MINOR__)
+# if ! (2 < __GNUC__ + (8 <= __GNUC_MINOR__))
 #  define __extension__
 # endif
 
 /* For GNU C, if not -traditional,
    we can define these macros to compute all args only once
    without using a global variable.
-   Also, we can avoid using the `temp' slot, to make faster code.  */
+   Also, we can avoid using the 'temp' slot, to make faster code.  */
 
 # define obstack_object_size(OBSTACK)					      \
   __extension__								      \
@@ -337,14 +343,16 @@  extern int obstack_exit_failure;
 # define obstack_ptr_grow_fast(OBSTACK, aptr)				      \
   __extension__								      \
     ({ struct obstack *__o1 = (OBSTACK);				      \
-       *(const void **) __o1->next_free = (aptr);			      \
+       void *__p1 = __o1->next_free;					      \
+       *(const void **) __p1 = (aptr);					      \
        __o1->next_free += sizeof (const void *);			      \
        (void) 0; })
 
 # define obstack_int_grow_fast(OBSTACK, aint)				      \
   __extension__								      \
     ({ struct obstack *__o1 = (OBSTACK);				      \
-       *(int *) __o1->next_free = (aint);				      \
+       void *__p1 = __o1->next_free;					      \
+       *(int *) __p1 = (aint);						      \
        __o1->next_free += sizeof (int);					      \
        (void) 0; })
 
@@ -398,7 +406,7 @@  extern int obstack_exit_failure;
        void *__obj = (OBJ);						      \
        if (__obj > (void *) __o->chunk && __obj < (void *) __o->chunk_limit)  \
          __o->next_free = __o->object_base = (char *) __obj;		      \
-       else (obstack_free) (__o, __obj); })
+       else (__obstack_free) (__o, __obj); })
 
 #else /* not __GNUC__ */
 
@@ -494,12 +502,14 @@  extern int obstack_exit_failure;
   ((h)->temp.tempint = (char *) (obj) - (char *) (h)->chunk,		      \
    ((((h)->temp.tempint > 0						      \
       && (h)->temp.tempint < (h)->chunk_limit - (char *) (h)->chunk))	      \
-   ? (((h)->next_free = (h)->object_base				      \
-                          = (h)->temp.tempint + (char *) (h)->chunk), 0)      \
-   : ((obstack_free) ((h), (h)->temp.tempint + (char *) (h)->chunk), 0)))
+    ? (void) ((h)->next_free = (h)->object_base				      \
+                          = (h)->temp.tempint + (char *) (h)->chunk)	      \
+    : (__obstack_free) (h, (h)->temp.tempint + (char *) (h)->chunk)))
+
 #endif /* not __GNUC__ */
 
 #ifdef __cplusplus
 }       /* C++ */
 #endif
+
 #endif /* obstack.h */