Re: [PATCH] libio: Eliminate _IO_stdin, _IO_stdout, _IO_stderr

["Dmitry V. Levin" <ldv at altlinux dot org>]

Hi,

On Sun, Feb 03, 2019 at 11:04:48AM +0100, Florian Weimer wrote:
> These variables are only used to determine if a stdio stream is
> a pre-allocated stream, but it is possible to do so by comparing
> a FILE * to all pre-allocated stream objects.  As a result, it is
> not necessary to keep those pointers in separate variables.
> 
> Behavior with symbol interposition is unchanged because _IO_stdin_,
> _IO_stdout_, _IO_stderr_ are exported, and refer to objects outside of
> libc if symbol interposition or copy relocations are involved.
> 
> 2019-02-03  Florian Weimer  <fweimer@redhat.com>
> 
> 	* libio/libio.h (_IO_stdin, _IO_stdout, _IO_stderr): Remove
> 	declaration.
> 	* libio/stdio.c (AL, AL2, _IO_stdin, _IO_stdout, _IO_stderr):
> 	Remove definitions.
> 	* libio/stdfiles.c: Update comment.
> 	* libio/oldstdfiles.c (_IO_check_libio): Update comment.  Do not
> 	set _IO_stdin, _IO_stdout, _IO_stderr.
> 	* libio/libioP.h (_IO_fake_stdiobuf): Remove unused declaration.
> 	[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)] (_IO_legacy_file): New
> 	inline function.
> 	(_IO_deallocate_file): New inline function.
> 	* libio/iolibio.h (_IO_vprintf): Remove definition.
> 	* libio/iofclose.c (_IO_new_fclose): Use _IO_deallocate_file.
> 	* libio/oldiofclose.c (_IO_old_fclose): Likewise.
> 	* libio/iofwide.c (_IO_fwide): Use __glibc_unlikely and
> 	_IO_legacy_file.
> 	* libio/oldfileops.c (_IO_old_file_init_internal): Remove
> 	__builtin_expect.  Use _IO_legacy_file.

Looks like _IO_legacy_file makes sense only when &_IO_stdin_used == NULL.
If the check was moved inside _IO_legacy_file, then ...

> diff --git a/libio/iofclose.c b/libio/iofclose.c
> index 9b39a6cc4e..8a80dd0b78 100644
> --- a/libio/iofclose.c
> +++ b/libio/iofclose.c
> @@ -71,12 +71,7 @@ _IO_new_fclose (FILE *fp)
>        if (_IO_have_backup (fp))
>  	_IO_free_backup_area (fp);
>      }
> -  if (fp != _IO_stdin && fp != _IO_stdout && fp != _IO_stderr)
> -    {
> -      fp->_flags = 0;
> -      free(fp);
> -    }
> -
> +  _IO_deallocate_file (fp);
>    return status;
>  }
>  
> diff --git a/libio/iofwide.c b/libio/iofwide.c
> index 6676ad5e53..247cfde3d0 100644
> --- a/libio/iofwide.c
> +++ b/libio/iofwide.c
> @@ -87,8 +87,7 @@ _IO_fwide (FILE *fp, int mode)
>    mode = mode < 0 ? -1 : (mode == 0 ? 0 : 1);
>  
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> -  if (__builtin_expect (&_IO_stdin_used == NULL, 0)
> -      && (fp == _IO_stdin || fp == _IO_stdout || fp == _IO_stderr))
> +  if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp))

... this would become
	if (_IO_legacy_file (fp)))

>      /* This is for a stream in the glibc 2.0 format.  */
>      return -1;
>  #endif
> diff --git a/libio/iolibio.h b/libio/iolibio.h
> index 2642d71e4f..9561833655 100644
> --- a/libio/iolibio.h
> +++ b/libio/iolibio.h
> @@ -58,8 +58,6 @@ extern int _IO_vsscanf (const char *, const char *, __gnuc_va_list) __THROW;
>     == _IO_pos_BAD ? EOF : 0)
>  #define _IO_rewind(FILE) \
>    (void) _IO_seekoff_unlocked (FILE, 0, 0, _IOS_INPUT|_IOS_OUTPUT)
> -#define _IO_vprintf(FORMAT, ARGS) \
> -  _IO_vfprintf (_IO_stdout, FORMAT, ARGS)
>  #define _IO_freopen(FILENAME, MODE, FP) \
>    (_IO_file_close_it (FP), \
>     _IO_file_fopen (FP, FILENAME, MODE, 1))
> diff --git a/libio/libio.h b/libio/libio.h
> index d21162aab0..872574cfb7 100644
> --- a/libio/libio.h
> +++ b/libio/libio.h
> @@ -185,9 +185,6 @@ struct _IO_FILE_plus;
>  extern struct _IO_FILE_plus _IO_2_1_stdin_;
>  extern struct _IO_FILE_plus _IO_2_1_stdout_;
>  extern struct _IO_FILE_plus _IO_2_1_stderr_;
> -extern FILE *_IO_stdin attribute_hidden;
> -extern FILE *_IO_stdout attribute_hidden;
> -extern FILE *_IO_stderr attribute_hidden;
>  
>  struct _IO_cookie_file;
>  
> diff --git a/libio/libioP.h b/libio/libioP.h
> index 8c75f15167..1c434ec3a1 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -817,7 +817,35 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>  # endif
>  #endif
>  
> -extern struct _IO_fake_stdiobuf _IO_stdin_buf, _IO_stdout_buf, _IO_stderr_buf;
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +/* See oldstdfiles.c.  These are the old stream variables.  */
> +extern struct _IO_FILE_plus _IO_stdin_;
> +extern struct _IO_FILE_plus _IO_stdout_;
> +extern struct _IO_FILE_plus _IO_stderr_;
> +
> +static inline bool
> +_IO_legacy_file (FILE *fp)
> +{
> +  return fp == (FILE *) &_IO_stdin_ || fp == (FILE *) &_IO_stdout_
> +    || fp == (FILE *) &_IO_stderr_;
> +}
> +#endif
> +
> +/* Deallocate a stream if it is heap-allocated.  Preallocated
> +   stdin/stdout/stderr streams are not deallocated. */
> +static inline void
> +_IO_deallocate_file (FILE *fp)
> +{
> +  /* The current stream variables.  */
> +  if (fp == (FILE *) &_IO_2_1_stdin_ || fp == (FILE *) &_IO_2_1_stdout_
> +      || fp == (FILE *) &_IO_2_1_stderr_)
> +    return;
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +  if (_IO_legacy_file (fp))

... this would save us from 3 extra checks for _IO_stdin_, _IO_stdout_,
and _IO_stderr_, and ...

> +    return;
> +#endif
> +  free (fp);
> +}
>  
>  #ifdef IO_DEBUG
>  # define CHECK_FILE(FILE, RET) do {			\
> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
> index 4d6c5e3fe7..10f2205e8a 100644
> --- a/libio/oldfileops.c
> +++ b/libio/oldfileops.c
> @@ -109,10 +109,7 @@ _IO_old_file_init_internal (struct _IO_FILE_plus *fp)
>  			     - (int) sizeof (struct _IO_FILE_complete));
>    fp->file._fileno = -1;
>  
> -  if (__builtin_expect (&_IO_stdin_used != NULL, 1)
> -      || (fp != (struct _IO_FILE_plus *) _IO_stdin
> -	  && fp != (struct _IO_FILE_plus *) _IO_stdout
> -	  && fp != (struct _IO_FILE_plus *) _IO_stderr))
> +  if (&_IO_stdin_used != NULL && !_IO_legacy_file ((FILE *) fp))

... and this would become
	if (!_IO_legacy_file ((FILE *) fp))

Overall a bit cleaner and a bit less error-prone.


-- 
ldv

Attachment: signature.asc
Description: PGP signature