[PATCH v2 05/14] stdio-common: Introduce buffers for implementing printf

[Adhemerval Zanella]

On 02/06/2022 14:58, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Some comments below.  I am not very found on this approach, it is *another*
>> layer on our already convoluted libio/stdio code; although I don't have a
>> better solution.  I *really* wish we could move away from libio...
> 
> On the plus side, it completely eliminates libio for sprintf, strfromd,
> strfmon.
> 
>>> +/* <printf_buffer_as_file.h> introduces a way to use struct
>>> +   __printf_buffer objects from FILE * streams.  To avoid storing a
>>> +   function pointer (or vtable pointer) in struct __printf_buffer
>>> +   (which would defeat libio vtable hardening), a switch statement
>>> +   over the different flush implementations is used to implement
>>> +   __printf_buffer_flush.
>>> +
>>> +   __printf_buffer_mode_failed is special: it is the sticky failure
>>> +   indicator.  Unlike struct alloc_buffer, this is not folded into
>>> +   write_ptr, so that snprintf and other string-writing functions can
>>> +   discover the end of the string even in the error case, to be able
>>> +   to add the null terminator.  */
>>> +enum __printf_buffer_mode
>>> +  {
>>> +    __printf_buffer_mode_failed,
>>> +    __printf_buffer_mode_to_file,
>>> +  };
>>
>> Do we really need to use namespace safe names for types as well? I usually
>> think it is usually clear to use otherwise.
> 
> It's needed to enable the type-generic implementations.
> stdio-common/Xprintf_buffer_flush.c uses Xprintf (buffer_mode_failed).
> I used to have two macros (Xprintf and __Xprintf), but I moved away from
> this because using __printf_ everywhere seemed more consistent.

Ok.

> 
>>> +/* Marks the buffer as failed, so that __printf_buffer_has_failed
>>> +   returns true and future flush operations are no-ops.  */
>>> +static inline void
>>> +__printf_buffer_mark_failed (struct __printf_buffer *buf)
>>
>> For other internal interfaces it seems that current pratice is to use
>> non double underscore static inline function wrappers over the namespace
>> safe symbols.  Maybe we should use the same strategy here.
> 
> Same reason as above.
>>
>>> +/* Write COUNT bytes starting at S to BUF.  S must not overlap with
>>> +   the internal buffer.  */
>>
>> Maybe add a overlpa check on Xprintf_buffer_write.c ?  Not sure if it is
>> worth though.
> 
> We actually can't because for sprintf, the target buffer (as specified)
> extends to the end of the address space, it's unbounded.
> 
>>> +int
>>> +Xprintf_buffer_done (struct Xprintf_buffer *buf)
>>> +{
>>> +  if (Xprintf_buffer_has_failed (buf))
>>> +    return -1;
>>> +
>>> +  /* Use uintptr_t to deal with ptrdiff_t overflows (to some degree).  */
>>
>> I am not sure using uintptr_t yield any gain in fact, if 
>> 'buf->write_ptr - buf->write_base' can not fit in ptrdiff_t it means the
>> code already trigger UB in write_ptr update.
> 
> I should probably remove that.
> 
>>> +  uintptr_t written_current = buf->write_ptr - buf->write_base;
>>> +  unsigned int written_total;
>>> +  if (__builtin_add_overflow (buf->written, written_current, &written_total)
>>
>> I think it should use INT_ADD_OVERFLOW since we still support GCC 6.5 and
>> __builtin_{add,sub}_overflow is not reliable with GCC 6.X [1].
>>
>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269
> 
> We require GCC 7.1 on s390x, see 844b4d8b4b937fe6943d2c0c80ce7d871cdb1eb5.
> 
> I'm not sure if INT_ADD_OVERFLOW deals correctly with mixed types.

I think it does have some drawbacks for some usages.  As a side note, I wonder
if should just create another header with just what we use from intprops.h 
(TYPE_MINIMUM, TYPE_MAXIMUM, INT_BUFSIZE_BOUND) so we don't need to keep with
extra unused overflow/wraps macros.  And it is one *less* thing we need to
keep in sync with gnulib.

> 
>>> +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)
>>> +    {
>>> +      if (buf->write_ptr == buf->write_end && !Xprintf_buffer_flush (buf))
>>> +        return;
>>> +      assert (buf->write_ptr != buf->write_end);
>>> +      size_t to_copy = STRNLEN (s, buf->write_end - buf->write_ptr);
>>> +      buf->write_ptr = MEMPCPY (buf->write_ptr, s, to_copy);
>>> +      s += to_copy;
>>> +    }
>>> +}
>>
>> Does it really need a loop here?
> 
> Yes, we might reach the end of the buffer with data left to write, so we
> have to flush it.  This can happen with asprintf and of course in the
> FILE * case.
> 

But in this case case you will just return right? I am trying to understand
where the loop will be executed twice in this case.