Re: [PATCH 0/5] obstacks again
Thanks for doing all this. The gnulib patches are good as far as they go, but
they need one more change: alignments should also change from int to size_t.
The first attached gnulib patch does that, plus it fixes a longstanding integer
overflow bug that can occur with large alignments (plus large sizes). While
we're in the neighborhood we should be using C11's alignof rather than
reinventing that particular wheel; the second attached gnulib patch does that.
I've installed your five gnulib patches plus the two attached patches into gnulib.
Two things for the glibc patch. First, the updated gnulib patches need to be
merged into the glibc patch. Second, the manual needs to be updated to match
the revised API induced by all these patches.
And thanks again.
From 421a53ad006c9ad9a1cad0b1e42246c01ffa3154 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Tue, 28 Oct 2014 23:58:42 -0700
Subject: [PATCH 1/2] obstack: use size_t alignments and check for overflow
* lib/obstack.c, lib/obstack.h (_obstack_begin, _obstack_begin_1):
* lib/obstack.c (_obstack_begin_worker, _obstack_newchunk):
* lib/obstack.h (struct obstack.alignment_mask):
Use _OBSTACK_SIZE_T, not int, for alignments.
* lib/obstack.c (_obstack_newchunk): Fail if the size calculation
overflows, e.g., when adding the alignment.
---
ChangeLog | 10 ++
lib/obstack.c | 18 +++---
lib/obstack.h | 8 +---
3 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 7aac7eb..8bf2baa 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2014-10-28 Paul Eggert
+
+ obstack: use size_t alignments and check for overflow
+ * lib/obstack.c, lib/obstack.h (_obstack_begin, _obstack_begin_1):
+ * lib/obstack.c (_obstack_begin_worker, _obstack_newchunk):
+ * lib/obstack.h (struct obstack.alignment_mask):
+ Use _OBSTACK_SIZE_T, not int, for alignments.
+ * lib/obstack.c (_obstack_newchunk): Fail if the size calculation
+ overflows, e.g., when adding the alignment.
+
2014-10-24 Paul Eggert
socketlib, sockets, sys_socket: Use AC_REQUIRE to pacify autoconf.
diff --git a/lib/obstack.c b/lib/obstack.c
index 8e247fb..342f9f8 100644
--- a/lib/obstack.c
+++ b/lib/obstack.c
@@ -106,7 +106,7 @@ typedef void (*freefun_type) (void *, struct _obstack_chunk
*);
static int
_obstack_begin_worker (struct obstack *h,
- _OBSTACK_SIZE_T size, int alignment,
+ _OBSTACK_SIZE_T size, _OBSTACK_SIZE_T alignment,
chunkfun_type chunkfun, freefun_type freefun)
{
struct _obstack_chunk *chunk; /* points to new chunk */
@@ -150,7 +150,7 @@ _obstack_begin_worker (struct obstack *h,
int
_obstack_begin (struct obstack *h,
-_OBSTACK_SIZE_T size, int alignment,
+_OBSTACK_SIZE_T size, _OBSTACK_SIZE_T alignment,
void *(*chunkfun) (size_t),
void (*freefun) (void *))
{
@@ -162,7 +162,7 @@ _obstack_begin (struct obstack *h,
int
_obstack_begin_1 (struct obstack *h,
- _OBSTACK_SIZE_T size, int alignment,
+ _OBSTACK_SIZE_T size, _OBSTACK_SIZE_T alignment,
void *(*chunkfun) (void *, size_t),
void (*freefun) (void *, void *),
void *arg)
@@ -184,18 +184,22 @@ void
_obstack_newchunk (struct obstack *h, _OBSTACK_SIZE_T length)
{
struct _obstack_chunk *old_chunk = h->chunk;
- struct _obstack_chunk *new_chunk;
- size_t new_size;
+ struct _obstack_chunk *new_chunk = 0;
size_t obj_size = h->next_free - h->object_base;
char *object_base;
/* Compute size for new chunk. */
- new_size = (obj_size + length) + (obj_size >> 3) + h->alignment_mask + 100;
+ size_t sum1 = obj_size + length;
+ size_t sum2 = sum1 + h->alignment_mask;
+ size_t new_size = sum2 + (obj_size >> 3) + 100;
+ if (new_size < sum2)
+new_size = sum2;
if (new_size < h->chunk_size)
new_size = h->chunk_size;
/* Allocate and initialize the new chunk. */
- new_chunk = CALL_CHUNKFUN (h, new_size);
+ if (obj_size <= sum1 && sum1 <= sum2)
+new_chunk = CALL_CHUNKFUN (h, new_size);
if (!new_chunk)
(*obstack_alloc_failed_handler)();
h->chunk = new_chunk;
diff --git a/lib/obstack.h b/lib/obstack.h
index 909d0d3..ba4de1d 100644
--- a/lib/obstack.h
+++ b/lib/obstack.h
@@ -166,7 +166,7 @@ struct obstack /* control current object in
current chunk */
_OBSTACK_SIZE_T i;
void *p;
} temp; /* Temporary for some macros. */
- int alignment_mask; /* Mask of alignment for each object. */
+ _OBSTACK_SIZE_T alignment_mask; /* Mask of alignment for each object. */
/* 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. */
@@ -187,9 +187,11 @@ struct obstack
Re: [PATCH 0/5] obstacks again
On Wed, 29 Oct 2014, Paul Eggert wrote: > While we're in the neighborhood we should be using C11's alignof rather than > reinventing that particular wheel; the second attached gnulib patch does that. Note that we can't use in glibc sources unless we move to requiring GCC >= 4.7 (and it's not clear whether we'd get consensus for such a requirement if proposed). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] 64-bit obstack support
On Wed, 29 Oct 2014, Alan Modra wrote: > And >2G on 32-bit. > [BZ #14483] in the ChangeLog entry. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH 0/5] obstacks again
On Wed, Oct 29, 2014 at 12:33:19AM -0700, Paul Eggert wrote:
> Thanks for doing all this. The gnulib patches are good as far as they go,
> but they need one more change: alignments should also change from int to
> size_t. The first attached gnulib patch does that, plus it fixes a
> longstanding integer overflow bug that can occur with large alignments (plus
> large sizes). While we're in the neighborhood we should be using C11's
> alignof rather than reinventing that particular wheel; the second attached
> gnulib patch does that. I've installed your five gnulib patches plus the two
> attached patches into gnulib.
One thing though, I didn't put the ChangeLog diffs in the patch as I
usually add them when committing.
> Two things for the glibc patch. First, the updated gnulib patches need to
> be merged into the glibc patch. Second, the manual needs to be updated to
> match the revised API induced by all these patches.
Ow! The manual reflects obstacks as they were 20 years ago.
Hmm, looking over it I see a misfeature that I've removed.
@cindex shrinking objects
You can use @code{obstack_blank} with a negative size argument to make
the current object smaller. Just don't try to shrink it beyond zero
length---there's no telling what will happen if you do that.
It is no longer possible to shrink an obstack with obstack_blank (but
you can still do that with obstack_blank_fast).
--
Alan Modra
Australia Development Lab, IBM
Re: [PATCH 0/5] obstacks again
Joseph S. Myers wrote:
Note that we can't use in glibc sources unless we move to
requiring GCC >= 4.7
Ah, thanks for mentioning that; the attached patch should fix that. I pushed
this into gnulib.
From d91a04a3dfc05b42031e8fd00af2cd29b6fa585d Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 29 Oct 2014 16:15:41 -0700
Subject: [PATCH] obstack: prefer __alignof__ to alignof
This is for portability to pre-4.7 GCC when compiling glibc.
See Joseph S. Myers in:
http://sourceware.org/ml/libc-alpha/2014-10/msg00703.html
* lib/obstack.c (__alignof__) [!_LIBC && !__GNUC__]:
New macro, defined by including and using .
(MAX): New macro.
(DEFAULT_ALIGNMENT, DEFAULT_ROUNDING): Redefine in terms of these.
Do not use enums as they are not portable to some broken compilers.
* modules/obstack (Depends-on): Depend on alignof, not stdalign.
---
ChangeLog | 11 +++
lib/obstack.c | 32 ++--
modules/obstack | 2 +-
3 files changed, 30 insertions(+), 15 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index ffe7285..cea54fc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
2014-10-29 Paul Eggert
+ obstack: prefer __alignof__ to alignof
+ This is for portability to pre-4.7 GCC when compiling glibc.
+ See Joseph S. Myers in:
+ http://sourceware.org/ml/libc-alpha/2014-10/msg00703.html
+ * lib/obstack.c (__alignof__) [!_LIBC && !__GNUC__]:
+ New macro, defined by including and using .
+ (MAX): New macro.
+ (DEFAULT_ALIGNMENT, DEFAULT_ROUNDING): Redefine in terms of these.
+ Do not use enums as they are not portable to some broken compilers.
+ * modules/obstack (Depends-on): Depend on alignof, not stdalign.
+
obstack: prefer alignof to calculating alignments by hand
* lib/obstack.c: Include .
(struct fooalign): Remove.
diff --git a/lib/obstack.c b/lib/obstack.c
index 03281ae..ba7dff3 100644
--- a/lib/obstack.c
+++ b/lib/obstack.c
@@ -48,26 +48,30 @@
#endif
#ifndef _OBSTACK_ELIDE_CODE
-# include
+# if !defined _LIBC && !defined __GNUC__
+# include
+# define __alignof__(type) alignof_type (type)
+# endif
# include
# include
+# ifndef MAX
+# define MAX(a,b) ((a) > (b) ? (a) : (b))
+# endif
+
/* Determine default alignment. */
-union fooround
-{
- uintmax_t i;
- long double d;
- void *p;
-};
+
/* If malloc were really smart, it would round addresses to DEFAULT_ALIGNMENT.
But in fact it might be less smart and round addresses to as much as
- DEFAULT_ROUNDING. So we prepare for it to do that. */
-enum
-{
- DEFAULT_ALIGNMENT = alignof (union fooround),
- DEFAULT_ROUNDING = sizeof (union fooround)
-};
-
+ DEFAULT_ROUNDING. So we prepare for it to do that.
+
+ DEFAULT_ALIGNMENT cannot be an enum constant; see gnulib's alignof.h. */
+#define DEFAULT_ALIGNMENT MAX (__alignof__ (long double),\
+ MAX (__alignof__ (uintmax_t), \
+__alignof__ (void *)))
+#define DEFAULT_ROUNDING MAX (sizeof (long double), \
+ MAX (sizeof (uintmax_t),
\
+sizeof (void *)))
/* Define a macro that either calls functions with the traditional malloc/free
calling interface, or calls functions with the mmalloc/mfree interface
diff --git a/modules/obstack b/modules/obstack
index c2c6390..36dc444 100644
--- a/modules/obstack
+++ b/modules/obstack
@@ -6,9 +6,9 @@ lib/obstack.h
lib/obstack.c
Depends-on:
+alignof
gettext-h
exitfail
-stdalign
stdint
stdlib
--
1.9.3
Re: [PATCH] 64-bit obstack support
On Wed, Oct 29, 2014 at 06:34:02PM +, Joseph S. Myers wrote:
> On Wed, 29 Oct 2014, Alan Modra wrote:
>
> > And >2G on 32-bit.
> >
>
> [BZ #14483]
>
> in the ChangeLog entry.
Thanks, added. Since the stdalign.h change won't do for glibc, I've
split the glibc patch into two. The first being
Import new obstack support from gnulib
* malloc/obstack.h: Import from gnulib.
* malloc/obstack.c: Likewise.
The second
64-bit obstack support
And >2G on 32-bit.
[BZ #14483]
* include/gnu-versions.h (_GNU_OBSTACK_INTERFACE_VERSION): Bump.
* include/obstack.h: Include shlib-compat.h and gnu-versions.h.
(_OBSTACK_ELIDE_CODE, _OBSTACK_NO_ERROR_HANDLER, _OBSTACK_COMPAT,
_OBSTACK_ALIAS): Define.
(_obstack_allocated_p, _obstack_newchunk, _obstack_free,
_obstack_begin, _obstack_begin_1, _obstack_memory_used): Define.
(_obstack_newchunk): Only use libc_hidden_proto on the version
we will use inside glibc.
* malloc/obstack.c: Revert gnulib commit e8f86ce9, ie. don't
use stdalign.h and alignof.
* malloc/obstackv1.c: New file.
* malloc/obstackv2.c: New file.
* malloc/Makefile (routines): Remove obstack. Add obstackv1 and
obstackv2.
(CFLAGS-obstack.c): Don't define.
(CFLAGS-obstackv1.c, CFLAGS-obstackv2.c): Define.
(malloc/Versions): Add GLIBC_2.21 _obstack functions.
* config.h.in (SIZEOF_INT, SIZEOF_SIZE_T): Add.
* configure.in: AC_CHECK_SIZEOF int and size_t.
* configure: Regenerate.
* manual/memory.texi: Update obstack documentation.
Added manual patch follows. It's missing @safety markup for the
previously undocumented obstack_begin, obstack_specify_allocation, and
obstack_specify_allocation_with_arg macros. I'm hoping someone else
can do that as I'd just be guessing. (Are they even appropriate for
macros?)
diff --git a/manual/memory.texi b/manual/memory.texi
index 0729e70..5ab16b7 100644
--- a/manual/memory.texi
+++ b/manual/memory.texi
@@ -1922,8 +1922,7 @@ the padding needed to start each object on a suitable
boundary.
use obstacks.
* Allocation in an Obstack::Allocating objects in an obstack.
* Freeing Obstack Objects:: Freeing objects in an obstack.
-* Obstack Functions:: The obstack functions are both
-functions and macros.
+* Obstack Functions:: The obstack functions are really macros.
* Growing Objects:: Making an object bigger by stages.
* Extra Fast Growing:: Extra-high-efficiency (though more
complicated) growing objects.
@@ -1948,7 +1947,7 @@ An obstack is represented by a data structure of type
@code{struct
obstack}. This structure has a small fixed size; it records the status
of the obstack and how to find the space in which objects are allocated.
It does not contain any of the objects themselves. You should not try
-to access the contents of the structure directly; use only the functions
+to access the contents of the structure directly; use only the macros
described in this chapter.
@end deftp
@@ -1958,7 +1957,7 @@ of object. Dynamic allocation of obstacks allows your
program to have a
variable number of different stacks. (You can even allocate an
obstack structure in another obstack, but this is rarely useful.)
-All the functions that work with obstacks require you to specify which
+All the macros that work with obstacks require you to specify which
obstack to use. You do this with a pointer of type @code{struct obstack
*}. In the following, we often say ``an obstack'' when strictly
speaking the object at hand is such a pointer.
@@ -1978,7 +1977,7 @@ These matters are described in the following section.
@node Preparing for Obstacks
@subsubsection Preparing for Using Obstacks
-Each source file in which you plan to use the obstack functions
+Each source file in which you plan to use obstacks
must include the header file @file{obstack.h}, like this:
@smallexample
@@ -2011,7 +2010,9 @@ larger blocks of memory. @xref{Obstack Chunks}, for full
details.
At run time, before the program can use a @code{struct obstack} object
as an obstack, it must initialize the obstack by calling
-@code{obstack_init}.
+@code{obstack_init} or one of its variants, @code{obstack_begin},
+@code{obstack_specify_allocation}, or
+@code{obstack_specify_allocation_with_arg}.
@comment obstack.h
@comment GNU
@@ -2031,10 +2032,10 @@ as an obstack, it must initialize the obstack by calling
@c fxprintf dup @asucorrupt @aculock @acucorrupt
@c exit @acucorrupt?
Initialize obstack @var{obstack-ptr} for allocation of objects. This
-function calls the obstack's @code{obstack_chunk_alloc} function. If
+macro calls the obstack's @code{obstack_chunk_alloc} function. If
allocation of memory fails, the function pointed to by
Re: [PATCH 0/5] obstacks again
Alan Modra wrote: One thing though, I didn't put the ChangeLog diffs in the patch as I usually add them when committing. Oh, I missed that. I added them now. For Gnulib it's better to put them into the patch. It is no longer possible to shrink an obstack with obstack_blank (but you can still do that with obstack_blank_fast). Ouch, I hadn't noticed that. That's an incompatible change and I expect it will break real-world usage for no particularly good reason, so we really need to fix this. How about making the 2nd argument to obstack_blank and obstack_blank_fast be of type ptrdiff_t rather than size_t?
Re: [PATCH 0/5] obstacks again
On Wed, Oct 29, 2014 at 08:35:18PM -0700, Paul Eggert wrote: > Alan Modra wrote: > >It is no longer possible to shrink an obstack with obstack_blank (but > >you can still do that with obstack_blank_fast). > > Ouch, I hadn't noticed that. That's an incompatible change and I expect it > will break real-world usage for no particularly good reason, so we really > need to fix this. How about making the 2nd argument to obstack_blank and > obstack_blank_fast be of type ptrdiff_t rather than size_t? We could, but.. I expect there is going to be other downstream code breakage anyway. Witness the patches needed to make gcc work with a more modern obstack.h, where a seemingly innocuous change like casting the obstack_base value to void* broke code doing pointer arithmetic, and gcc's use of the internal _obstack_begin function meant that gcc was exposed to the changed chunkfun type. Now if the 2nd arg of obstack_blank changes to type ptrdiff_t, then it becomes an odd case, all the other obstack "functions" taking a size_t. Also, I'd say obstack_blank is the wrong macro to use for shrinking, the most natural one being obstack_blank_fast due to it bypassing the obstack_room check. So there are my reasons for leaving obstack_blank as is. However, if you and others prefer to go with ptrdiff_t I'm happy enough to submit a patch. Oh, and while I'm advocating breakage, the doc for obstack_next_free says it returns a void* but actually returns a char*. Since it was deemed a good idea to similarly correct obstack_base, should we do the same for obstack_next_free? -- Alan Modra Australia Development Lab, IBM
