This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] libio: Eliminate _IO_stdin, _IO_stdout, _IO_stderr
- From: "Gabriel F. T. Gomes" <gabriel at inconstante dot eti dot br>
- To: Florian Weimer <fweimer at redhat dot com>
- Cc: <libc-alpha at sourceware dot org>
- Date: Tue, 12 Feb 2019 14:36:24 -0200
- Subject: Re: [PATCH] libio: Eliminate _IO_stdin, _IO_stdout, _IO_stderr
- References: <87lg2x5gsf.fsf@oldenburg2.str.redhat.com>
Hi, Florian.
I have a question about the change in libio/oldfileops.c (see below).
The rest of the patch looks good to me.
On Sun, Feb 03 2019, 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.
>
> 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);
OK.
> 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))
OK.
> /* 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;
The removal. OK.
>
> 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))
> + return;
> +#endif
> + free (fp);
> +}
OK.
>
> #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))
Why did the `||' become an `&&'? Is this solving a bug?
> /* The object is dynamically allocated and large enough. Initialize
> the _mode element as well. */
> ((struct _IO_FILE_complete *) fp)->_mode = -1;
> diff --git a/libio/oldiofclose.c b/libio/oldiofclose.c
> index e4cbf88566..be5044c3bd 100644
> --- a/libio/oldiofclose.c
> +++ b/libio/oldiofclose.c
> @@ -58,12 +58,7 @@ _IO_old_fclose (FILE *fp)
> _IO_FINISH (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);
OK.
> return status;
> }
>
> diff --git a/libio/oldstdfiles.c b/libio/oldstdfiles.c
> index 524e260b7e..2b861cd754 100644
> --- a/libio/oldstdfiles.c
> +++ b/libio/oldstdfiles.c
> @@ -27,11 +27,8 @@
> #include <shlib-compat.h>
> #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
>
> -/* This file provides definitions of _IO_stdin, _IO_stdout, and _IO_stderr
> - for C code. Compare stdstreams.cc.
> - (The difference is that here the vtable field is set to 0,
> - so the objects defined are not valid C++ objects. On the other
> - hand, we don't need a C++ compiler to build this file.) */
> +/* This file provides legacy definitions of _IO_stdin_, _IO_stdout_,
> + and _IO_stderr_. See stdfiles.c for the current definitions. */
>
> #define _IO_USE_OLD_IO_FILE
> #include "libioP.h"
> @@ -78,13 +75,12 @@ _IO_check_libio (void)
> if (&_IO_stdin_used == NULL)
> {
> /* We are using the old one. */
> - _IO_stdin = stdin = (FILE *) &_IO_stdin_;
> - _IO_stdout = stdout = (FILE *) &_IO_stdout_;
> - _IO_stderr = stderr = (FILE *) &_IO_stderr_;
> + stdin = (FILE *) &_IO_stdin_;
> + stdout = (FILE *) &_IO_stdout_;
> + stderr = (FILE *) &_IO_stderr_;
_IO_std* are gone. OK.
> _IO_list_all = &_IO_stderr_;
> - _IO_stdin->_vtable_offset = _IO_stdout->_vtable_offset =
> - _IO_stderr->_vtable_offset = stdin->_vtable_offset =
> - stdout->_vtable_offset = stderr->_vtable_offset =
> + stdin->_vtable_offset = stdout->_vtable_offset
> + = stderr->_vtable_offset =
Likewise. OK.
> ((int) sizeof (struct _IO_FILE)
> - (int) sizeof (struct _IO_FILE_complete));
> }
> diff --git a/libio/stdfiles.c b/libio/stdfiles.c
> index 605e006474..9c779b47eb 100644
> --- a/libio/stdfiles.c
> +++ b/libio/stdfiles.c
> @@ -25,11 +25,10 @@
> in files containing the exception. */
>
>
> -/* This file provides definitions of _IO_stdin, _IO_stdout, and _IO_stderr
> - for C code. Compare stdstreams.cc.
> - (The difference is that here the vtable field is set to 0,
> - so the objects defined are not valid C++ objects. On the other
> - hand, we don't need a C++ compiler to build this file.) */
> +/* This file provides definitions of _IO_2_1_stdin_, _IO_2_1_stdout_,
> + and _IO_2_1_stderr_, the default values of stdin, stdout, stderr.
> + See oldstdfiles.c for glibc 2.0 legacy definitions without wide
> + character support. */
>
> #include "libioP.h"
>
> diff --git a/libio/stdio.c b/libio/stdio.c
> index 1294d2842e..522de44a27 100644
> --- a/libio/stdio.c
> +++ b/libio/stdio.c
> @@ -33,14 +33,3 @@
> FILE *stdin = (FILE *) &_IO_2_1_stdin_;
> FILE *stdout = (FILE *) &_IO_2_1_stdout_;
> FILE *stderr = (FILE *) &_IO_2_1_stderr_;
> -
> -#undef _IO_stdin
> -#undef _IO_stdout
> -#undef _IO_stderr
> -#define AL(name) AL2 (name, _IO_##name)
> -#define AL2(name, al) \
> - extern __typeof (name) al __attribute__ ((alias (#name), \
> - visibility ("hidden")))
> -AL(stdin);
> -AL(stdout);
> -AL(stderr);
Gone. OK.