[PATCH v4] ungetc: Guarantee single char pushback
Maciej W. Rozycki
macro@redhat.com
Mon Dec 16 02:52:55 GMT 2024
Hi Sid,
Thanks for your effort, this is looking mostly good to me.
My main concern is `_IO_free_backup_buf', which I think will perform
better as a static inline function. I have some questions as well as to
the test functions including a request to add introductory comments for
them with the answers. Plus a couple of small nits, as all detailed
below.
> The C standard requires that ungetc guarantees at least one pushback, so
> put a single byte pushback buffer in the FILE struct to enable that.
Please mention here why this single byte pushback buffer is needed to
fulfil the C standard's requirement (i.e. that we fail to fulfil it now
because we use `malloc', which can fail).
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Hmm, please clarify your copyright status.
> diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
> index d8d26639d1..a5e0679de3 100644
> --- a/libio/bits/types/struct_FILE.h
> +++ b/libio/bits/types/struct_FILE.h
> @@ -1,4 +1,5 @@
> /* Copyright (C) 1991-2024 Free Software Foundation, Inc.
> + Copyright The GNU Toolchain Authors.
OK. Consistent with DCO (but see above).
> @@ -70,7 +71,8 @@ struct _IO_FILE
> struct _IO_FILE *_chain;
>
> int _fileno;
> - int _flags2;
> + int _flags2:24;
> + char _short_backupbuf[1];
OK. Taking advantage of a char member sharing the storage unit with the
preceding bit-field, so there's no change in the size of the structure or
member offsets. Please add a short description of the new member, just as
with most of the existing ones (all should have one IMO).
> diff --git a/libio/fileops.c b/libio/fileops.c
> index 759d737ec7..d49e489f55 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -1,4 +1,5 @@
> /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
> + Copyright The GNU Toolchain Authors.
OK. Consistent with DCO (but see above).
> @@ -480,7 +481,7 @@ _IO_new_file_underflow (FILE *fp)
> /* Maybe we already have a push back pointer. */
> if (fp->_IO_save_base != NULL)
> {
> - free (fp->_IO_save_base);
> + _IO_free_backup_buf (fp, fp->_IO_save_base);
OK. Mechanical update.
> @@ -932,7 +933,7 @@ _IO_new_file_seekoff (FILE *fp, off64_t offset, int dir, int mode)
> /* It could be that we already have a pushback buffer. */
> if (fp->_IO_read_base != NULL)
> {
> - free (fp->_IO_read_base);
> + _IO_free_backup_buf (fp, fp->_IO_read_base);
OK. Mechanical update.
> @@ -1282,7 +1283,7 @@ _IO_file_xsgetn (FILE *fp, void *data, size_t n)
> /* Maybe we already have a push back pointer. */
> if (fp->_IO_save_base != NULL)
> {
> - free (fp->_IO_save_base);
> + _IO_free_backup_buf (fp, fp->_IO_save_base);
OK. Mechanical update.
> diff --git a/libio/genops.c b/libio/genops.c
> index d7e35e67d5..dddd420ee2 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -1,4 +1,5 @@
> /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
> + Copyright The GNU Toolchain Authors.
OK. Consistent with DCO (but see above).
> @@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
> }
> #endif
>
> +void
> +_IO_free_backup_buf (FILE *fp, char *ptr)
> +{
> + if (ptr != fp->_short_backupbuf)
> + free (ptr);
> +}
> +
OK, this replaces explicit calls to `free', taking care of the special
case of the backup buffer.
But is there a need for this to be an external function?
ISTM there could be a performance benefit from making it static inline:
an arrangement for making calls here is likely not to be cheaper in terms
of instruction size/count or execution time than making the comparison and
branching around `free', even for simplistic predictors that predict all
forward branches untaken.
From the look of the code I infer we normally expect `ptr' not to point
at the backup buffer as that will only happen in the case of a `malloc'
failure, so firstly such a forward branch usually won't indeed be taken,
making it virtually free for the fall-through case, and secondly please
annotate the expression with `__glibc_unlikely' accordingly.
> @@ -212,7 +220,7 @@ _IO_free_backup_area (FILE *fp)
> {
> if (_IO_in_backup (fp))
> _IO_switch_to_main_get_area (fp); /* Just in case. */
> - free (fp->_IO_save_base);
> + _IO_free_backup_buf (fp, fp->_IO_save_base);
OK. Mechanical update.
> @@ -260,7 +268,7 @@ save_for_backup (FILE *fp, char *end_p)
> memcpy (new_buffer + avail,
> fp->_IO_read_base + least_mark,
> needed_size);
> - free (fp->_IO_save_base);
> + _IO_free_backup_buf (fp, fp->_IO_save_base);
OK. Mechanical update.
> @@ -636,7 +644,7 @@ _IO_default_finish (FILE *fp, int dummy)
>
> if (fp->_IO_save_base)
> {
> - free (fp->_IO_save_base);
> + _IO_free_backup_buf (fp, fp->_IO_save_base);
OK. Mechanical update.
> @@ -998,11 +1006,14 @@ _IO_default_pbackfail (FILE *fp, int c)
> else if (!_IO_have_backup (fp))
> {
> /* No backup buffer: allocate one. */
> - /* Use nshort buffer, if unused? (probably not) FIXME */
> int backup_size = 128;
> char *bbuf = (char *) malloc (backup_size);
> if (bbuf == NULL)
> - return EOF;
> + {
> + /* Guarantee a 1-char pushback. */
> + bbuf = fp->_short_backupbuf;
> + backup_size = 1;
> + }
OK. In the unlikely case of a `malloc' failure we'll resort to the
single-character backup buffer, avoiding an unsuccessful return.
Thanks for discarding a comment that's no longer relevant. From
observation such bits are too easily missed.
> @@ -1022,7 +1033,7 @@ _IO_default_pbackfail (FILE *fp, int c)
> return EOF;
> memcpy (new_buf + (new_size - old_size), fp->_IO_read_base,
> old_size);
> - free (fp->_IO_read_base);
> + _IO_free_backup_buf (fp, fp->_IO_read_base);
OK. Mechanical update.
> diff --git a/libio/libioP.h b/libio/libioP.h
> index 34bf91fcd8..287caf8664 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -1,4 +1,5 @@
> /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
> + Copyright The GNU Toolchain Authors.
OK. Consistent with DCO (but see above).
> @@ -357,6 +358,8 @@ typedef FILE *_IO_ITER;
>
> /* Generic functions */
>
> +extern void _IO_free_backup_buf (FILE *, char *);
> +libc_hidden_proto (_IO_free_backup_buf)
OK. But this won't be needed with a static inline function.
> @@ -911,13 +914,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
> # define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
> { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
> - 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
> + 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
OK. New member initialised.
> # else
> # define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
> { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
> NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, \
> - NULL, NULL, (FILE *) CHAIN, FD, \
> - 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD,\
> + NULL, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
> + _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
OK. New member initialised.
I think it will make sense to keep the line breaks between the same
members across all the four FILEBUF_LITERAL definitions so as to make it
easier to people to match the variants against each other.
Please coordinate with Alejandro Colomar (CC'd) on cleaning up these
definitions, which went out of sync; cf.
<https://inbox.sourceware.org/libc-alpha/042e25d3-1b02-c448-1f8c-84f52549f5b5@redhat.com/>.
> @@ -925,12 +928,12 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
> # define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
> { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
> - 0, _IO_pos_BAD }
> + 0, { 0 }, _IO_pos_BAD }
OK. New member initialised.
> # else
> # define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
> { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
> - 0, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
> + 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
OK. New member initialised.
> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
> index 8f775c9094..03f4d76a57 100644
> --- a/libio/oldfileops.c
> +++ b/libio/oldfileops.c
> @@ -1,4 +1,5 @@
> /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
> + Copyright The GNU Toolchain Authors.
OK. Consistent with DCO (but see above).
> @@ -311,7 +312,7 @@ _IO_old_file_underflow (FILE *fp)
> /* Maybe we already have a push back pointer. */
> if (fp->_IO_save_base != NULL)
> {
> - free (fp->_IO_save_base);
> + _IO_free_backup_buf (fp, fp->_IO_save_base);
OK. Mechanical update.
> @@ -464,7 +465,7 @@ _IO_old_file_seekoff (FILE *fp, off64_t offset, int dir, int mode)
> /* It could be that we already have a pushback buffer. */
> if (fp->_IO_read_base != NULL)
> {
> - free (fp->_IO_read_base);
> + _IO_free_backup_buf (fp, fp->_IO_read_base);
OK. Mechanical update.
> diff --git a/libio/wfileops.c b/libio/wfileops.c
> index 16beab1f3a..a96bfa589b 100644
> --- a/libio/wfileops.c
> +++ b/libio/wfileops.c
> @@ -1,4 +1,5 @@
> /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
> + Copyright The GNU Toolchain Authors.
OK. Consistent with DCO (but see above).
> @@ -175,7 +176,7 @@ _IO_wfile_underflow (FILE *fp)
> /* Maybe we already have a push back pointer. */
> if (fp->_IO_save_base != NULL)
> {
> - free (fp->_IO_save_base);
> + _IO_free_backup_buf (fp, fp->_IO_save_base);
OK. Mechanical update.
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index e76e40e587..b1a04fd064 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -1,4 +1,5 @@
> # Copyright (C) 1991-2024 Free Software Foundation, Inc.
> +# Copyright The GNU Toolchain Authors.
OK. Consistent with DCO (but see above).
> @@ -303,6 +304,7 @@ tests := \
> tst-tmpnam \
> tst-ungetc \
> tst-ungetc-leak \
> + tst-ungetc-nomem \
OK. New test added.
> diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
> new file mode 100644
> index 0000000000..49db33fff3
> --- /dev/null
> +++ b/stdio-common/tst-ungetc-nomem.c
> @@ -0,0 +1,115 @@
> +/* Test ungetc behavior with malloc failures.
> + Copyright The GNU Toolchain Authors.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
OK, alphabetic order.
> +
> +extern void *__libc_malloc (size_t)
> + __attribute__ ((malloc)) __attribute__ ((alloc_size (1)));
Please list both attributes together, also avoiding the question as to
how much to indent here (as the prototype will fit in one line then).
> +
> +static volatile bool fail = false;
OK, marked as `volatile' to prevent the compiler from interfering, as
previously advised. I note this could skip the initialiser so as to place
it in BSS, but I'm fine with the current arrangement if you consider it
desirable to make it explicit.
> +
> +void *
> +malloc (size_t sz)
> +{
> + if (fail)
> + return NULL;
> +
> + return __libc_malloc (sz);
> +}
OK, this interposes `malloc' so as to conditionally induce a failure and
refers to `__libc_malloc' if the condition does not stand. A bit hackish
IMO, but we're in control here, so let it be.
I think this function deserves an introductory comment, even if a single
terse line.
> +
> +static int
> +do_test (void)
> +{
> + char *filename = NULL;
> + struct stat props = {};
As nice as empty initialisers are they're a C23-ism, so please rewrite
using older syntax.
> + size_t bufsz = 0;
> +
> + create_temp_file ("tst-ungetc-nomem.", &filename);
> + if (stat (filename, &props) != 0)
> + FAIL_EXIT1 ("Could not get file status: %m\n");
> +
> + FILE *fp = fopen (filename, "w");
> +
> + /* The libio buffer sizes are the same as block size. */
> + bufsz = props.st_blksize + 2;
Why do we want the file to be the size of the libio buffer plus 2? The
answer seems like a good candidate for the function's introductory
comment.
> +
> + char *buf = xmalloc (bufsz);
> + memset (buf, 'a', bufsz);
> +
> + if (fwrite (buf, sizeof (char), bufsz, fp) != bufsz)
> + FAIL_EXIT1 ("fwrite failed: %m\n");
> + xfclose (fp);
OK, we make a test file made up of "a" letters.
> +
> + /* Begin test. */
> + fp = xfopen (filename, "r");
> +
> + while (!feof (fp))
> + {
> + /* Reset the pushback buffer state. */
> + fseek (fp, 0, SEEK_CUR);
> +
> + fail = true;
> + /* 1: First ungetc should always succeed, as the standard requires. */
> + TEST_COMPARE (ungetc ('y', fp), 'y');
> +
> + /* 2: This will result in resizing, which should fail. */
> + TEST_COMPARE (ungetc ('w', fp), EOF);
> +
> + /* 3: Now allow the resizing, which should immediately fill up the buffer
> + too, since this allocates only double the current buffer, i.e.
> + 2-bytes. */
> + fail = false;
> + TEST_COMPARE (ungetc ('x', fp), 'x');
This does verify new semantics, and I take it it's intentional that after
a `malloc' failure for the initial buffer we don't go back to the minimum
of 128 bytes for the buffer, but instead start from 2 up in a hope for a
smaller allocation to succeed where a somewhat larger one might not. But
I think this new semantics should be mentioned in the change description.
> +
> + /* 4: And fail again because this again forces an alloc, which fails. */
> + fail = true;
> + TEST_COMPARE (ungetc ('x', fp), EOF);
> +
> + /* 5: Enable allocations again so that we now get a 4-byte buffer. Now
> + both calls should work. */
> + fail = false;
> + TEST_COMPARE (ungetc ('x', fp), 'x');
> + fail = true;
> + TEST_COMPARE (ungetc ('x', fp), 'x');
> +
> + /* Drain out the x's. */
> + TEST_COMPARE (fgetc (fp), 'x');
> + TEST_COMPARE (fgetc (fp), 'x');
> + TEST_COMPARE (fgetc (fp), 'x');
Shouldn't the `ungetc' calls use different characters each, so that we
have an additional check that rejected characters do not come back and
that the accepted ones come back in the correct order?
> +
> + /* Finally, drain out the first char we had pushed back, followed by one more char
> + from the stream, if present. */
Please wrap the comment, cf:
<https://sourceware.org/glibc/wiki/Style_and_Conventions#A79-Column_Lines>.
> + TEST_COMPARE (fgetc (fp), 'y');
> + char c = fgetc (fp);
> + if (!feof (fp))
> + TEST_COMPARE (c, 'a');
> + }
So this loop, if successful, runs libio buffer size plus 2 times. Please
state in the function's introductory comment why this specific iteration
count has been chosen.
> +
> + /* Final sanity check before we're done. */
> + TEST_COMPARE (ferror (fp), 0);
> + xfclose (fp);
OK. Checking for no error and closing the test file. File removed
automagically by test support clean-up.
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
OK.
Please resend with the updates applied (but I note further clarification
might be needed first).
Maciej
More information about the Libc-alpha
mailing list