[PATCH v4] ungetc: Guarantee single char pushback
Siddhesh Poyarekar
siddhesh@sourceware.org
Mon Dec 16 12:58:28 GMT 2024
On 2024-12-15 21:52, Maciej W. Rozycki wrote:
> 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).
Ack, I'll update the comment.
>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>
> Hmm, please clarify your copyright status.
I had my personal copyright assignment to the FSF revoked in ~2019. My
employer (Red Hat) too has disclaimed copyright to my contributions to
GNU projects. As author, I am the owner of the copyright to code I
write for GNU projects, hence the Signed-off-by.
>> 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).
OK.
>> 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.
That's a good point, I'll move it to libioP.h.
>> @@ -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/>.
I'll rebase on top of whatever he pushes, or alternatively I've asked if
he could send a separate patch for this file which I can include in my
series and push whenever my patch is ready.
>> @@ -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).
OK.
>> +
>> +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.
OK.
>> +
>> +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.
We use this quite extensively across glibc, so I'm inclined to keep this
unless you have a strong objection; it just looks so clean :) Besides,
as Alejandro pointed out, it's been a GNU extension since before that.
>> + 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.
Ack, it's basically to test to the point of running out of buffer space
so that we test at the read underflow border.
>> +
>> + 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.
OK, I'll add a comment in the pushback code change.
>
>> +
>> + /* 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?
I've grouped them, but sure, I could make each call unget/get a distinct
char.
>> +
>> + /* 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>.
Oops.
>> + 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.
Same as mentioned above, to test at the read underflow boundary, which
happens at libio buffer size.
>> +
>> + /* 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).
Thanks,
Sid
More information about the Libc-alpha
mailing list