Re: [PATCH v5 02/11] stdio-common: Introduce buffers for implementing printf

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella Netto:

>> +/* Writes COUNT repeats of CH to BUF.  */
>> +void __printf_buffer_pad_1 (struct __printf_buffer *buf,
>> +                            char ch, size_t count) attribute_hidden;
>> +
>> +/* __printf_buffer_pad with fast path for no padding.  COUNT is a
>> +   long int, to accomodate signed uses in printf and elsewhere.  */
>
> Wouldn't ssize works better here than long int?

I'm going to test the change.

>> +static inline void
>> +__printf_buffer_pad (struct __printf_buffer *buf, char ch, long int count)
>> +{
>> +  if (count > 0)
>> +    __printf_buffer_pad_1 (buf, ch, count);
>> +}
>> +
>> +/* Write COUNT bytes starting at S to BUF.  S must not overlap with
>> +   the internal buffer.  */
>> +void __printf_buffer_write (struct __printf_buffer *buf, const char *s,
>> +                            size_t count) attribute_hidden;
>> +
>> +/* Write S to BUF.  S must not overlap with the internal buffer.  */
>> +void __printf_buffer_puts_1 (struct __printf_buffer *buf, const char *s)
>> +  attribute_hidden;
>
> Maybe a space here?

Added an empty line.

>> +static inline void
>> +__printf_buffer_puts (struct __printf_buffer *buf, const char *s)
>> +{
>> +  if (__builtin_constant_p (__builtin_strlen (s)))
>> +    __printf_buffer_write (buf, s, __builtin_strlen (s));
>> +  else
>> +    __printf_buffer_puts_1 (buf, s);
>> +}
>> +
>> +/* Returns the number of bytes written through the buffer, or -1 if
>> +   there was an error (that is, __printf_buffer_has_failed (BUF) is true).
>> +
>> +   The number of written bytes includes pending bytes in the buffer
>> +   (between BUF->write_base and BUF->write_ptr).
>> +
>> +   If the number is larger than INT_MAX, returns -1 and sets errno to
>> +   EOVERFLOW.  This function does not flush the buffer.  If the caller
>> +   needs the side effect of flushing, it has to do this
>> +   separately.  */
>> +int __printf_buffer_done (struct __printf_buffer *buf) attribute_hidden;
>
> Wouldn't ssize_t work better for return type?

No, it's really for use in printf etc., and those return int
(incorrectly, too late to fix now), so we have to check against INT_MAX.

>> +int
>> +Xprintf_buffer_done (struct Xprintf_buffer *buf)
>> +{
>> +  if (Xprintf_buffer_has_failed (buf))
>> +    return -1;
>> +
>> +  /* Use uintptr_t here because for sprintf, the buffer range may
>> +     cover more than half of the address space.  */
>> +  uintptr_t written_current = buf->write_ptr - buf->write_base;
>
> Is this really a valid case?  Working with buffer ranges larger than half
> of address space is not valid for malloc buffer anyway, are you handling
> some iffy case for mmap case?

It's not really supported, but I wanted to mention it.

>> +void
>> +Xprintf (buffer_puts_1) (struct Xprintf_buffer *buf, const CHAR_T *s)
>> +{
>> +  if (__glibc_unlikely (Xprintf_buffer_has_failed (buf)))
>> +    return;
>> +
>> +  while (*s != 0)
>
> Maybe use NULL here?

No, *s is CHAR_T.

>> +static const struct _IO_jump_t _IO_printf_buffer_as_file_jumps libio_vtable =
>> +{
>> +  JUMP_INIT_DUMMY,
>> +  JUMP_INIT(finish, NULL),
>> +  JUMP_INIT(overflow, __printf_buffer_as_file_overflow),
>> +  JUMP_INIT(underflow, NULL),
>> +  JUMP_INIT(uflow, NULL),
>> +  JUMP_INIT(pbackfail, NULL),
>> +  JUMP_INIT(xsputn, __printf_buffer_as_file_xsputn),
>> +  JUMP_INIT(xsgetn, NULL),
>> +  JUMP_INIT(seekoff, NULL),
>> +  JUMP_INIT(seekpos, NULL),
>> +  JUMP_INIT(setbuf, NULL),
>> +  JUMP_INIT(sync, NULL),
>> +  JUMP_INIT(doallocate, NULL),
>> +  JUMP_INIT(read, NULL),
>> +  JUMP_INIT(write, NULL),
>> +  JUMP_INIT(seek, NULL),
>> +  JUMP_INIT(close, NULL),
>> +  JUMP_INIT(stat, NULL),
>> +  JUMP_INIT(showmanyc, NULL),
>> +  JUMP_INIT(imbue, NULL)
>> +};
>
> The _IO_printf_buffer_as_file_jumps should be added on
> tst-relro-symbols.py required list.

Done.

>> diff --git a/stdio-common/printf_buffer_to_file.h b/stdio-common/printf_buffer_to_file.h
>> new file mode 100644
>> index 0000000000..97ec1d6d95
>> --- /dev/null
>> +++ b/stdio-common/printf_buffer_to_file.h
>> @@ -0,0 +1,57 @@
>> +/* Multibyte and wide printf buffers writing data to a FILE * stream.
>> +   Copyright (C) 2022 Free Software Foundation, Inc.
>> +   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/>.  */
>> +
>> +#ifndef PRINTF_BUFFER_TO_FILE_H
>> +#define PRINTF_BUFFER_TO_FILE_H
>> +
>> +#include <bits/types/FILE.h>
>> +#include <printf_buffer.h>
>> +
>> +struct __printf_buffer_to_file
>> +{
>> +  struct __printf_buffer base;
>> +  FILE *fp;
>> +
>> +  /* Staging buffer.  All data goes through this buffer before
>> +     reaching the fp stream.  (Any buffer in fp is not yet used.)  */
>> +  char stage[128];
>> +};
>
> Out of curiosity, why 128?

Few lines are longer than 128, and this path is used for line-buffered
streams (among others).

>> +static const struct _IO_jump_t _IO_wprintf_buffer_as_file_jumps libio_vtable =
>> +{
>> +  JUMP_INIT_DUMMY,
>> +  JUMP_INIT(finish, NULL),
>> +  JUMP_INIT(overflow, (_IO_overflow_t) __wprintf_buffer_as_file_overflow),
>> +  JUMP_INIT(underflow, NULL),
>> +  JUMP_INIT(uflow, NULL),
>> +  JUMP_INIT(pbackfail, NULL),
>> +  JUMP_INIT(xsputn, __wprintf_buffer_as_file_xsputn),
>> +  JUMP_INIT(xsgetn, NULL),
>> +  JUMP_INIT(seekoff, NULL),
>> +  JUMP_INIT(seekpos, NULL),
>> +  JUMP_INIT(setbuf, NULL),
>> +  JUMP_INIT(sync, NULL),
>> +  JUMP_INIT(doallocate, NULL),
>> +  JUMP_INIT(read, NULL),
>> +  JUMP_INIT(write, NULL),
>> +  JUMP_INIT(seek, NULL),
>> +  JUMP_INIT(close, NULL),
>> +  JUMP_INIT(stat, NULL),
>> +  JUMP_INIT(showmanyc, NULL),
>> +  JUMP_INIT(imbue, NULL)
>> +};
>> +
>
> As for _IO_printf_buffer_as_file_jumps,
> _IO_wprintf_buffer_as_file_jumps also should be added on
> tst-relro-symbols.py required list.

Done.

Thanks,
Florian