[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 <eggert@cs.ucla.edu>
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(-)
  

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 <stdalign.h> 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).
  

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 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?
  

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?
  

On 10/29/2014 09:35 PM, Paul Eggert wrote:
> 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?

It breaks GNU M4, for a starter :)  But at least we predicted that it
would happen, and I'm hoping the fallback of obstack_blank_fast does the
job.
  

On Dec 5, 2014, at 7:13 PM, Eric Blake <eblake@redhat.com> wrote:
> On 10/29/2014 09:35 PM, Paul Eggert wrote:
>> 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?
> 
> It breaks GNU M4, for a starter :)  But at least we predicted that it
> would happen, and I'm hoping the fallback of obstack_blank_fast does the
> job.

It does, thanks.

Although for someone not following bug-gnulib fairly carefully, it is far
from obvious why bumping gnulib makes your application suddenly use up all
the available memory.  Which is a shame, because, other than that M4 would
have been perfectly functional after the gnulib bump without any special
client code changes.

If there's a way to at least diagnose negative arguments rather than silently
change behavior, that would save other projects some migration headaches...

Cheers,
  

On Fri, Dec 05, 2014 at 09:02:46PM +0000, Gary V. Vaughan wrote:
> If there's a way to at least diagnose negative arguments rather than silently
> change behavior, that would save other projects some migration headaches...

We could diagnose one of the m4 uses of obstack_blank at compile time,
but not the one in m4/macro.c:trace_flush which has a non-constant
shrink value..

Yes, I agree this can cause some pain, but at least dying with
out-of-memory is a loud symptom.

The alternative is to do as Paul suggested and make obstack_blank
accept a negative length argument, but that would
- kill >2G obstacks on 32-bit targets,
- lose the nice symmetry with other obstack functions,
and obstack_blank_fast is the right interface to use for shrinking.
  

Alan Modra <amodra@gmail.com> writes:

> The alternative is to do as Paul suggested and make obstack_blank
> accept a negative length argument, but that would
> - kill >2G obstacks on 32-bit targets,
> - lose the nice symmetry with other obstack functions,
> and obstack_blank_fast is the right interface to use for shrinking.

It would reintroduce the nice symmetry that the *_fast functions are
just optimisations of the non-fast functions (if you know that the
obstack has enough space), without surprising behaviour change.

Andreas.