[v5,10/11] libio: Convert __obstack_vprintf_internal to buffers (bug 27124)

Message ID 45234c5d6ba9556451424940fa746e9e66b1a6f4.1670858473.git.fweimer@redhat.com
State Superseded
Headers
Series vfprintf refactor |

Commit Message

Florian Weimer Dec. 12, 2022, 3:24 p.m. UTC
  This fixes bug 27124 because the problematic built-in vtable is gone.
---
 include/printf_buffer.h            |   4 +
 libio/obprintf.c                   | 170 +++++++++--------------------
 stdio-common/printf_buffer_flush.c |   4 +
 3 files changed, 58 insertions(+), 120 deletions(-)
  

Comments

Adhemerval Zanella Netto Dec. 15, 2022, 7:14 p.m. UTC | #1
On 12/12/22 12:24, Florian Weimer via Libc-alpha wrote:
> This fixes bug 27124 because the problematic built-in vtable is gone.

Patch looks good, some minor nit below.

> ---
>  include/printf_buffer.h            |   4 +
>  libio/obprintf.c                   | 170 +++++++++--------------------
>  stdio-common/printf_buffer_flush.c |   4 +
>  3 files changed, 58 insertions(+), 120 deletions(-)
> 
> diff --git a/include/printf_buffer.h b/include/printf_buffer.h
> index 3d4ef1d06c..a8211dd657 100644
> --- a/include/printf_buffer.h
> +++ b/include/printf_buffer.h
> @@ -54,6 +54,7 @@ enum __printf_buffer_mode
>      __printf_buffer_mode_fp,         /* For __printf_fp_l_buffer.  */
>      __printf_buffer_mode_fp_to_wide, /* For __wprintf_fp_l_buffer.  */
>      __printf_buffer_mode_fphex_to_wide, /* For __wprintf_fphex_l_buffer.  */
> +    __printf_buffer_mode_obstack,

Maybe add 'For __printf_buffer_flush_obstack'?

>    };
>  
>  /* Buffer for fast character writing with overflow handling.
> @@ -319,6 +320,9 @@ struct __printf_buffer_fphex_to_wide;
>  void __printf_buffer_flush_fphex_to_wide (struct
>                                            __printf_buffer_fphex_to_wide *)
>    attribute_hidden;
> +struct __printf_buffer_obstack;
> +void __printf_buffer_flush_obstack (struct __printf_buffer_obstack *)
> +  attribute_hidden;
>  
>  struct __wprintf_buffer_to_file;
>  void __wprintf_buffer_flush_to_file (struct __wprintf_buffer_to_file *)
> diff --git a/libio/obprintf.c b/libio/obprintf.c
> index c220be9adc..459d73ebf3 100644
> --- a/libio/obprintf.c
> +++ b/libio/obprintf.c
> @@ -16,131 +16,59 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -
> -#include <stdlib.h>
> -#include "libioP.h"
> -#include "strfile.h"
>  #include <assert.h>
> -#include <string.h>
> -#include <errno.h>
> +#include <math_ldbl_opt.h>
>  #include <obstack.h>
> +#include <printf.h>
>  #include <stdarg.h>
> -#include <stdio_ext.h>
> -
> +#include <printf_buffer.h>
>  
> -struct _IO_obstack_file
> +struct __printf_buffer_obstack
>  {
> -  struct _IO_FILE_plus file;
> +  struct __printf_buffer base;
>    struct obstack *obstack;
> -};
> -
> -
> -static int
> -_IO_obstack_overflow (FILE *fp, int c)
> -{
> -  struct obstack *obstack = ((struct _IO_obstack_file *) fp)->obstack;
> -  int size;
> -
> -  /* Make room for another character.  This might as well allocate a
> -     new chunk a memory and moves the old contents over.  */
> -  assert (c != EOF);
> -  obstack_1grow (obstack, c);
> -
> -  /* Setup the buffer pointers again.  */
> -  fp->_IO_write_base = obstack_base (obstack);
> -  fp->_IO_write_ptr = obstack_next_free (obstack);
> -  size = obstack_room (obstack);
> -  fp->_IO_write_end = fp->_IO_write_ptr + size;
> -  /* Now allocate the rest of the current chunk.  */
> -  obstack_blank_fast (obstack, size);
> -
> -  return c;
> -}
>  
> +  /* obstack_1grow is called for compatibility reasons.  This needs
> +     one extra character, and this is the backing store for it.  */
> +  char ch;
> +};
>  
> -static size_t
> -_IO_obstack_xsputn (FILE *fp, const void *data, size_t n)
> +void
> +__printf_buffer_flush_obstack (struct __printf_buffer_obstack *buf)
>  {
> -  struct obstack *obstack = ((struct _IO_obstack_file *) fp)->obstack;
> +  /* About to switch buffers, so record the bytes written so far.  */
> +  buf->base.written += buf->base.write_ptr - buf->base.write_base;
>  
> -  if (fp->_IO_write_ptr + n > fp->_IO_write_end)
> +  if (buf->base.write_ptr == &buf->ch + 1)
>      {
> -      int size;
> -
> -      /* We need some more memory.  First shrink the buffer to the
> -	 space we really currently need.  */
> -      obstack_blank_fast (obstack, fp->_IO_write_ptr - fp->_IO_write_end);
> -
> -      /* Now grow for N bytes, and put the data there.  */
> -      obstack_grow (obstack, data, n);
> -
> -      /* Setup the buffer pointers again.  */
> -      fp->_IO_write_base = obstack_base (obstack);
> -      fp->_IO_write_ptr = obstack_next_free (obstack);
> -      size = obstack_room (obstack);
> -      fp->_IO_write_end = fp->_IO_write_ptr + size;
> -      /* Now allocate the rest of the current chunk.  */
> -      obstack_blank_fast (obstack, size);
> +      /* Errors are reported via a callback mechanism (presumably for
> +	 process termination).  */
> +      obstack_1grow (buf->obstack, buf->ch);
> +      buf->base.write_base = obstack_next_free (buf->obstack);
> +      buf->base.write_ptr = buf->base.write_base;
> +      size_t size = obstack_room (buf->obstack);
> +      buf->base.write_end = buf->base.write_ptr + size;
> +      /* Reserve the space on the obstack size.  */
> +      obstack_blank_fast (buf->obstack, size);
>      }
>    else
> -    fp->_IO_write_ptr = __mempcpy (fp->_IO_write_ptr, data, n);
> -
> -  return n;
> +    {
> +      /* Obtain the extra character.  */
> +      buf->base.write_base = &buf->ch;
> +      buf->base.write_ptr = &buf->ch;
> +      buf->base.write_end = &buf->ch + 1;
> +    }
>  }
>  
> -
> -/* the jump table.  */
> -const struct _IO_jump_t _IO_obstack_jumps libio_vtable attribute_hidden =
> -{
> -  JUMP_INIT_DUMMY,
> -  JUMP_INIT(finish, NULL),
> -  JUMP_INIT(overflow, _IO_obstack_overflow),
> -  JUMP_INIT(underflow, NULL),
> -  JUMP_INIT(uflow, NULL),
> -  JUMP_INIT(pbackfail, NULL),
> -  JUMP_INIT(xsputn, _IO_obstack_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)
> -};
> -
> -

Remove _IO_obstack_jumps from tst-relro-libc.out list.

>  int
>  __obstack_vprintf_internal (struct obstack *obstack, const char *format,
>  			    va_list args, unsigned int mode_flags)
>  {
> -  struct obstack_FILE
> -    {
> -      struct _IO_obstack_file ofile;
> -  } new_f;
> -  int result;
> -  int size;
> -  int room;
> -
> -#ifdef _IO_MTSAFE_IO
> -  new_f.ofile.file.file._lock = NULL;
> -#endif
> -
> -  _IO_no_init (&new_f.ofile.file.file, _IO_USER_LOCK, -1, NULL, NULL);
> -  _IO_JUMPS (&new_f.ofile.file) = &_IO_obstack_jumps;
> -  room = obstack_room (obstack);
> -  size = obstack_object_size (obstack) + room;
> +  /* Legacy setup code for compatibility.  */
> +  size_t room = obstack_room (obstack);
> +  size_t size = obstack_object_size (obstack) + room;
>    if (size == 0)
>      {
> -      /* We have to handle the allocation a bit different since the
> -	 `_IO_str_init_static' function would handle a size of zero
> -	 different from what we expect.  */
> -
>        /* Get more memory.  */
>        obstack_make_room (obstack, 64);
>  
> @@ -151,27 +79,29 @@ __obstack_vprintf_internal (struct obstack *obstack, const char *format,
>        assert (size != 0);
>      }
>  
> -  _IO_str_init_static_internal ((struct _IO_strfile_ *) &new_f.ofile,
> -				obstack_base (obstack),
> -				size, obstack_next_free (obstack));
> +  struct __printf_buffer_obstack buf;
> +  {
> +    /* The obstack write location might be in the middle of an object.  */
> +    char *ptr = obstack_next_free (obstack);
> +    char *end = obstack_base (obstack) + size;
> +    __printf_buffer_init (&buf.base, ptr, end - ptr,
> +			  __printf_buffer_mode_obstack);
> +  }
> +  buf.obstack = obstack;
> +
>    /* Now allocate the rest of the current chunk.  */
> -  assert (size == (new_f.ofile.file.file._IO_write_end
> -		   - new_f.ofile.file.file._IO_write_base));
> -  assert (new_f.ofile.file.file._IO_write_ptr
> -	  == (new_f.ofile.file.file._IO_write_base
> -	      + obstack_object_size (obstack)));
>    obstack_blank_fast (obstack, room);
>  
> -  new_f.ofile.obstack = obstack;
> -
> -  result = __vfprintf_internal (&new_f.ofile.file.file, format, args,
> -				mode_flags);
> +  __printf_buffer (&buf.base, format, args, mode_flags);
>  
> -  /* Shrink the buffer to the space we really currently need.  */
> -  obstack_blank_fast (obstack, (new_f.ofile.file.file._IO_write_ptr
> -				- new_f.ofile.file.file._IO_write_end));
> +  if (buf.base.write_ptr == &buf.ch + 1)
> +    /* buf.ch is in use.  Put it into the obstack.  */
> +    obstack_1grow (buf.obstack, buf.ch);
> +  else if (buf.base.write_ptr != &buf.ch)
> +    /* Shrink the buffer to the space we really currently need.  */
> +    obstack_blank_fast (buf.obstack, buf.base.write_ptr - buf.base.write_end);
>  
> -  return result;
> +  return __printf_buffer_done (&buf.base);
>  }
>  
>  int

Ok.

> diff --git a/stdio-common/printf_buffer_flush.c b/stdio-common/printf_buffer_flush.c
> index 922340cc54..42afe49413 100644
> --- a/stdio-common/printf_buffer_flush.c
> +++ b/stdio-common/printf_buffer_flush.c
> @@ -32,6 +32,7 @@
>  # pragma weak __printf_buffer_flush_fp
>  # pragma weak __printf_buffer_flush_fp_to_wide
>  # pragma weak __printf_buffer_flush_fphex_to_wide
> +# pragma weak __printf_buffer_flush_obstack
>  #endif /* !SHARED */
>  
>  static void
> @@ -72,6 +73,9 @@ __printf_buffer_do_flush (struct __printf_buffer *buf)
>        __printf_buffer_flush_fphex_to_wide
>          ((struct __printf_buffer_fphex_to_wide *) buf);
>        return;
> +    case __printf_buffer_mode_obstack:
> +      __printf_buffer_flush_obstack ((struct __printf_buffer_obstack *) buf);
> +      return;
>      }
>    __builtin_trap ();
>  }
  
Florian Weimer Dec. 16, 2022, 6:25 p.m. UTC | #2
* Adhemerval Zanella Netto:

> On 12/12/22 12:24, Florian Weimer via Libc-alpha wrote:
>> This fixes bug 27124 because the problematic built-in vtable is gone.
>
> Patch looks good, some minor nit below.
>
>> ---
>>  include/printf_buffer.h            |   4 +
>>  libio/obprintf.c                   | 170 +++++++++--------------------
>>  stdio-common/printf_buffer_flush.c |   4 +
>>  3 files changed, 58 insertions(+), 120 deletions(-)
>> 
>> diff --git a/include/printf_buffer.h b/include/printf_buffer.h
>> index 3d4ef1d06c..a8211dd657 100644
>> --- a/include/printf_buffer.h
>> +++ b/include/printf_buffer.h
>> @@ -54,6 +54,7 @@ enum __printf_buffer_mode
>>      __printf_buffer_mode_fp,         /* For __printf_fp_l_buffer.  */
>>      __printf_buffer_mode_fp_to_wide, /* For __wprintf_fp_l_buffer.  */
>>      __printf_buffer_mode_fphex_to_wide, /* For __wprintf_fphex_l_buffer.  */
>> +    __printf_buffer_mode_obstack,
>
> Maybe add 'For __printf_buffer_flush_obstack'?

Fixed.

>> -/* the jump table.  */
>> -const struct _IO_jump_t _IO_obstack_jumps libio_vtable attribute_hidden =
>> -{

> Remove _IO_obstack_jumps from tst-relro-libc.out list.

Right, fixed.

Thanks,
Florian
  

Patch

diff --git a/include/printf_buffer.h b/include/printf_buffer.h
index 3d4ef1d06c..a8211dd657 100644
--- a/include/printf_buffer.h
+++ b/include/printf_buffer.h
@@ -54,6 +54,7 @@  enum __printf_buffer_mode
     __printf_buffer_mode_fp,         /* For __printf_fp_l_buffer.  */
     __printf_buffer_mode_fp_to_wide, /* For __wprintf_fp_l_buffer.  */
     __printf_buffer_mode_fphex_to_wide, /* For __wprintf_fphex_l_buffer.  */
+    __printf_buffer_mode_obstack,
   };
 
 /* Buffer for fast character writing with overflow handling.
@@ -319,6 +320,9 @@  struct __printf_buffer_fphex_to_wide;
 void __printf_buffer_flush_fphex_to_wide (struct
                                           __printf_buffer_fphex_to_wide *)
   attribute_hidden;
+struct __printf_buffer_obstack;
+void __printf_buffer_flush_obstack (struct __printf_buffer_obstack *)
+  attribute_hidden;
 
 struct __wprintf_buffer_to_file;
 void __wprintf_buffer_flush_to_file (struct __wprintf_buffer_to_file *)
diff --git a/libio/obprintf.c b/libio/obprintf.c
index c220be9adc..459d73ebf3 100644
--- a/libio/obprintf.c
+++ b/libio/obprintf.c
@@ -16,131 +16,59 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-
-#include <stdlib.h>
-#include "libioP.h"
-#include "strfile.h"
 #include <assert.h>
-#include <string.h>
-#include <errno.h>
+#include <math_ldbl_opt.h>
 #include <obstack.h>
+#include <printf.h>
 #include <stdarg.h>
-#include <stdio_ext.h>
-
+#include <printf_buffer.h>
 
-struct _IO_obstack_file
+struct __printf_buffer_obstack
 {
-  struct _IO_FILE_plus file;
+  struct __printf_buffer base;
   struct obstack *obstack;
-};
-
-
-static int
-_IO_obstack_overflow (FILE *fp, int c)
-{
-  struct obstack *obstack = ((struct _IO_obstack_file *) fp)->obstack;
-  int size;
-
-  /* Make room for another character.  This might as well allocate a
-     new chunk a memory and moves the old contents over.  */
-  assert (c != EOF);
-  obstack_1grow (obstack, c);
-
-  /* Setup the buffer pointers again.  */
-  fp->_IO_write_base = obstack_base (obstack);
-  fp->_IO_write_ptr = obstack_next_free (obstack);
-  size = obstack_room (obstack);
-  fp->_IO_write_end = fp->_IO_write_ptr + size;
-  /* Now allocate the rest of the current chunk.  */
-  obstack_blank_fast (obstack, size);
-
-  return c;
-}
 
+  /* obstack_1grow is called for compatibility reasons.  This needs
+     one extra character, and this is the backing store for it.  */
+  char ch;
+};
 
-static size_t
-_IO_obstack_xsputn (FILE *fp, const void *data, size_t n)
+void
+__printf_buffer_flush_obstack (struct __printf_buffer_obstack *buf)
 {
-  struct obstack *obstack = ((struct _IO_obstack_file *) fp)->obstack;
+  /* About to switch buffers, so record the bytes written so far.  */
+  buf->base.written += buf->base.write_ptr - buf->base.write_base;
 
-  if (fp->_IO_write_ptr + n > fp->_IO_write_end)
+  if (buf->base.write_ptr == &buf->ch + 1)
     {
-      int size;
-
-      /* We need some more memory.  First shrink the buffer to the
-	 space we really currently need.  */
-      obstack_blank_fast (obstack, fp->_IO_write_ptr - fp->_IO_write_end);
-
-      /* Now grow for N bytes, and put the data there.  */
-      obstack_grow (obstack, data, n);
-
-      /* Setup the buffer pointers again.  */
-      fp->_IO_write_base = obstack_base (obstack);
-      fp->_IO_write_ptr = obstack_next_free (obstack);
-      size = obstack_room (obstack);
-      fp->_IO_write_end = fp->_IO_write_ptr + size;
-      /* Now allocate the rest of the current chunk.  */
-      obstack_blank_fast (obstack, size);
+      /* Errors are reported via a callback mechanism (presumably for
+	 process termination).  */
+      obstack_1grow (buf->obstack, buf->ch);
+      buf->base.write_base = obstack_next_free (buf->obstack);
+      buf->base.write_ptr = buf->base.write_base;
+      size_t size = obstack_room (buf->obstack);
+      buf->base.write_end = buf->base.write_ptr + size;
+      /* Reserve the space on the obstack size.  */
+      obstack_blank_fast (buf->obstack, size);
     }
   else
-    fp->_IO_write_ptr = __mempcpy (fp->_IO_write_ptr, data, n);
-
-  return n;
+    {
+      /* Obtain the extra character.  */
+      buf->base.write_base = &buf->ch;
+      buf->base.write_ptr = &buf->ch;
+      buf->base.write_end = &buf->ch + 1;
+    }
 }
 
-
-/* the jump table.  */
-const struct _IO_jump_t _IO_obstack_jumps libio_vtable attribute_hidden =
-{
-  JUMP_INIT_DUMMY,
-  JUMP_INIT(finish, NULL),
-  JUMP_INIT(overflow, _IO_obstack_overflow),
-  JUMP_INIT(underflow, NULL),
-  JUMP_INIT(uflow, NULL),
-  JUMP_INIT(pbackfail, NULL),
-  JUMP_INIT(xsputn, _IO_obstack_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)
-};
-
-
 int
 __obstack_vprintf_internal (struct obstack *obstack, const char *format,
 			    va_list args, unsigned int mode_flags)
 {
-  struct obstack_FILE
-    {
-      struct _IO_obstack_file ofile;
-  } new_f;
-  int result;
-  int size;
-  int room;
-
-#ifdef _IO_MTSAFE_IO
-  new_f.ofile.file.file._lock = NULL;
-#endif
-
-  _IO_no_init (&new_f.ofile.file.file, _IO_USER_LOCK, -1, NULL, NULL);
-  _IO_JUMPS (&new_f.ofile.file) = &_IO_obstack_jumps;
-  room = obstack_room (obstack);
-  size = obstack_object_size (obstack) + room;
+  /* Legacy setup code for compatibility.  */
+  size_t room = obstack_room (obstack);
+  size_t size = obstack_object_size (obstack) + room;
   if (size == 0)
     {
-      /* We have to handle the allocation a bit different since the
-	 `_IO_str_init_static' function would handle a size of zero
-	 different from what we expect.  */
-
       /* Get more memory.  */
       obstack_make_room (obstack, 64);
 
@@ -151,27 +79,29 @@  __obstack_vprintf_internal (struct obstack *obstack, const char *format,
       assert (size != 0);
     }
 
-  _IO_str_init_static_internal ((struct _IO_strfile_ *) &new_f.ofile,
-				obstack_base (obstack),
-				size, obstack_next_free (obstack));
+  struct __printf_buffer_obstack buf;
+  {
+    /* The obstack write location might be in the middle of an object.  */
+    char *ptr = obstack_next_free (obstack);
+    char *end = obstack_base (obstack) + size;
+    __printf_buffer_init (&buf.base, ptr, end - ptr,
+			  __printf_buffer_mode_obstack);
+  }
+  buf.obstack = obstack;
+
   /* Now allocate the rest of the current chunk.  */
-  assert (size == (new_f.ofile.file.file._IO_write_end
-		   - new_f.ofile.file.file._IO_write_base));
-  assert (new_f.ofile.file.file._IO_write_ptr
-	  == (new_f.ofile.file.file._IO_write_base
-	      + obstack_object_size (obstack)));
   obstack_blank_fast (obstack, room);
 
-  new_f.ofile.obstack = obstack;
-
-  result = __vfprintf_internal (&new_f.ofile.file.file, format, args,
-				mode_flags);
+  __printf_buffer (&buf.base, format, args, mode_flags);
 
-  /* Shrink the buffer to the space we really currently need.  */
-  obstack_blank_fast (obstack, (new_f.ofile.file.file._IO_write_ptr
-				- new_f.ofile.file.file._IO_write_end));
+  if (buf.base.write_ptr == &buf.ch + 1)
+    /* buf.ch is in use.  Put it into the obstack.  */
+    obstack_1grow (buf.obstack, buf.ch);
+  else if (buf.base.write_ptr != &buf.ch)
+    /* Shrink the buffer to the space we really currently need.  */
+    obstack_blank_fast (buf.obstack, buf.base.write_ptr - buf.base.write_end);
 
-  return result;
+  return __printf_buffer_done (&buf.base);
 }
 
 int
diff --git a/stdio-common/printf_buffer_flush.c b/stdio-common/printf_buffer_flush.c
index 922340cc54..42afe49413 100644
--- a/stdio-common/printf_buffer_flush.c
+++ b/stdio-common/printf_buffer_flush.c
@@ -32,6 +32,7 @@ 
 # pragma weak __printf_buffer_flush_fp
 # pragma weak __printf_buffer_flush_fp_to_wide
 # pragma weak __printf_buffer_flush_fphex_to_wide
+# pragma weak __printf_buffer_flush_obstack
 #endif /* !SHARED */
 
 static void
@@ -72,6 +73,9 @@  __printf_buffer_do_flush (struct __printf_buffer *buf)
       __printf_buffer_flush_fphex_to_wide
         ((struct __printf_buffer_fphex_to_wide *) buf);
       return;
+    case __printf_buffer_mode_obstack:
+      __printf_buffer_flush_obstack ((struct __printf_buffer_obstack *) buf);
+      return;
     }
   __builtin_trap ();
 }