This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2 5/5] Post-cleanup 2: minimize _G_config.h.
On 05/02/2018 21:35, Zack Weinberg wrote:
> Nearly everything in _G_config.h is either junk or more appropriately
> defined elsewhere:
>
> * _G_fpos_t, _G_fpos64_t, and _G_BUFSIZ are already completely unused.
> * All remaining uses of _G_va_list have been changed to __gnuc_va_list.
> * The definition of _G_HAVE_ST_BLKSIZE/_IO_HAVE_ST_BLKSIZE has
> been inlined into its sole use.
> * The complete definition of _G_iconv_t has been moved to libio.h and
> renamed _IO_iconv_t (all actual users used that name).
> * _G_IO_IO_FILE_VERSION is vestigial; some code cares whether
> _IO_stdin_used exists, but nothing looks at its value. I've
> preserved the value as a hardwired constant in csu/init.c.
> This means csu/init.c no longer needs to include anything.
> * Many of the headers included by _G_config.h were already being
> included directly by either either libio.h or stdio.h; the
> remaining ones were moved to libio.h.
> * _G_HAVE_MREMAP is still relevant, because mremap genuinely is a
> Linux extension; it's not in POSIX and as far as I can tell it's
> not available on the Hurd either. I also preserved _G_HAVE_MMAP,
> since it's conceivable someone would want to port glibc to a
> MMU-less, mmap-less environment in the future. Both are now always
> defined to 1/0 as is the current convention, instead of the older
> 1/undef convention. These are the only symbols still defined in
> _G_config.h.
> * The actual inclusion of _G_config.h moves from libio.h to libioP.h,
> as this is where a potential override of _G_HAVE_MMAP happens.
> * The #ifdef logic in libioP.h controlling _IO_JUMPS_OFFSET has been
> simplified.
>
> After this patch, the only surviving _G_ symbols are the struct tag
> names _G_fpos_t and _G_fpos64_t, which are preserved for the sake of
> C++ mangled names in applications, and _G_HAVE_MMAP and _G_HAVE_MREMAP,
> which do not seem worth renaming.
>
> Installed stripped libraries are unchanged by this patch.
LGTM in general with just _IO_HAVE_ST_BLKSIZE suggestion below.
>
> * bits/_G_config.h: Move back to sysdeps/generic/_G_config.h.
> Delete all contents except for definitions of _G_HAVE_MMAP and
> _G_HAVE_MREMAP. Add commentary explaining those two symbols.
> * sysdeps/unix/sysv/linux/bits/_G_config.h: Move back to
> sysdeps/unix/sysv/linux/_G_config.h. Make same content
> change as above.
>
> * libio/libio.h: Don't include bits/_G_config.h here.
> Include stddef.h with __need_wchar_t defined. Include
> bits/types/__mbstate_t.h, bits/types/wint_t.h, and gconv.h.
> Define _IO_iconv_t here, directly.
> Don't define _IO_HAVE_ST_BLKSIZE.
> * libio/libioP.h: Include _G_config.h here. Move include of
> shlib-compat.h up with rest of includes. Simplify conditionals
> controlling definition of _IO_JUMPS_OFFSET.
>
> * csu/init.c: Remove always-true #if around entire file.
> Don't include stdio.h. Set _IO_stdin_used to hardwired
> constant 0x20001, and update commentary.
> * include/stdio.h, sysdeps/ieee754/ldbl-opt/nldbl-compat.h:
> Replace all uses of _G_va_list with __gnuc_va_list.
> * libio/filedoalloc.c: Use #if defined _STATBUF_ST_BLKSIZE
> instead of #if _IO_HAVE_ST_BLKSIZE.
> * libio/fileops.c: Test _G_HAVE_MREMAP with #if, not #ifdef.
> * libio/iofdopen.c, libio/iofopen.c: Test _G_HAVE_MMAP with #if,
> not #ifdef.
> ---
> bits/_G_config.h | 54 -------------------------------
> csu/init.c | 13 +++-----
> include/stdio.h | 28 ++++++++--------
> libio/filedoalloc.c | 2 +-
> libio/fileops.c | 2 +-
> libio/iofdopen.c | 4 +--
> libio/iofopen.c | 2 +-
> libio/libio.h | 22 ++++++++++---
> libio/libioP.h | 20 ++++++------
> sysdeps/generic/_G_config.h | 15 +++++++++
> sysdeps/ieee754/ldbl-opt/nldbl-compat.h | 13 ++++----
> sysdeps/unix/sysv/linux/_G_config.h | 15 +++++++++
> sysdeps/unix/sysv/linux/bits/_G_config.h | 55 --------------------------------
> 13 files changed, 89 insertions(+), 156 deletions(-)
> delete mode 100644 bits/_G_config.h
> create mode 100644 sysdeps/generic/_G_config.h
> create mode 100644 sysdeps/unix/sysv/linux/_G_config.h
> delete mode 100644 sysdeps/unix/sysv/linux/bits/_G_config.h
>
> diff --git a/bits/_G_config.h b/bits/_G_config.h
> deleted file mode 100644
> index 8c81bc42787..00000000000
> --- a/bits/_G_config.h
> +++ /dev/null
> @@ -1,54 +0,0 @@
> -/* This file is needed by libio to define various configuration parameters.
> - These are always the same in the GNU C library. */
> -
> -#ifndef _BITS_G_CONFIG_H
> -#define _BITS_G_CONFIG_H 1
> -
> -/* Define types for libio in terms of the standard internal type names. */
> -
> -#include <bits/types.h>
> -#define __need_size_t
> -#if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
> -# define __need_wchar_t
> -#endif
> -#define __need_NULL
> -#include <stddef.h>
> -
> -#include <bits/types/__mbstate_t.h>
> -#include <bits/types/__fpos_t.h>
> -#include <bits/types/__fpos64_t.h>
> -
> -#define _G_fpos_t __fpos_t
> -#define _G_fpos64_t __fpos64_t
> -
> -#if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
> -# include <bits/types/wint_t.h>
> -#endif
> -
> -#if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
> -# include <gconv.h>
> -typedef union
> -{
> - struct __gconv_info __cd;
> - struct
> - {
> - struct __gconv_info __cd;
> - struct __gconv_step_data __data;
> - } __combined;
> -} _G_iconv_t;
> -#endif
> -
> -
> -/* These library features are always available in the GNU C library. */
> -#define _G_va_list __gnuc_va_list
> -
> -#define _G_HAVE_MMAP 1
> -
> -#define _G_IO_IO_FILE_VERSION 0x20001
> -
> -/* This is defined by <bits/stat.h> if `st_blksize' exists. */
> -#define _G_HAVE_ST_BLKSIZE defined (_STATBUF_ST_BLKSIZE)
> -
> -#define _G_BUFSIZ 8192
> -
> -#endif /* bits/_G_config.h */
Ok.
> diff --git a/csu/init.c b/csu/init.c
> index 178063b44bf..c2f978f3da5 100644
> --- a/csu/init.c
> +++ b/csu/init.c
> @@ -16,11 +16,8 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> -#if defined __GNUC__ && __GNUC__ >= 2
> -
> -#include <stdio.h>
> -
> -/* This records which stdio is linked against in the application. */
> -const int _IO_stdin_used = _G_IO_IO_FILE_VERSION;
> -
> -#endif
> +/* Vestigial libio version number. Some code in libio checks whether
> + this symbol exists in the executable, but nothing looks at its
> + value anymore; the value it was historically set to has been
> + preserved out of an abundance of caution. */
> +const int _IO_stdin_used = 0x20001;
Ok.
> diff --git a/include/stdio.h b/include/stdio.h
> index f12b281b4d6..94bc2fdc7ef 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -15,44 +15,44 @@ extern int __snprintf (char *__restrict __s, size_t __maxlen,
> __attribute__ ((__format__ (__printf__, 3, 4)));
> libc_hidden_proto (__snprintf)
> extern int __vsnprintf (char *__restrict __s, size_t __maxlen,
> - const char *__restrict __format, _G_va_list __arg)
> + const char *__restrict __format, __gnuc_va_list __arg)
> __attribute__ ((__format__ (__printf__, 3, 0)));
> extern int __vfscanf (FILE *__restrict __s,
> const char *__restrict __format,
> - _G_va_list __arg)
> + __gnuc_va_list __arg)
> __attribute__ ((__format__ (__scanf__, 2, 0)));
> libc_hidden_proto (__vfscanf)
> extern int __vscanf (const char *__restrict __format,
> - _G_va_list __arg)
> + __gnuc_va_list __arg)
> __attribute__ ((__format__ (__scanf__, 1, 0)));
> extern _IO_ssize_t __getline (char **__lineptr, size_t *__n,
> FILE *__stream) attribute_hidden;
> extern int __vsscanf (const char *__restrict __s,
> const char *__restrict __format,
> - _G_va_list __arg)
> + __gnuc_va_list __arg)
> __attribute__ ((__format__ (__scanf__, 2, 0)));
>
> extern int __sprintf_chk (char *, int, size_t, const char *, ...) __THROW;
> extern int __snprintf_chk (char *, size_t, int, size_t, const char *, ...)
> __THROW;
> extern int __vsprintf_chk (char *, int, size_t, const char *,
> - _G_va_list) __THROW;
> + __gnuc_va_list) __THROW;
> extern int __vsnprintf_chk (char *, size_t, int, size_t, const char *,
> - _G_va_list) __THROW;
> + __gnuc_va_list) __THROW;
> extern int __printf_chk (int, const char *, ...);
> extern int __fprintf_chk (FILE *, int, const char *, ...);
> -extern int __vprintf_chk (int, const char *, _G_va_list);
> -extern int __vfprintf_chk (FILE *, int, const char *, _G_va_list);
> +extern int __vprintf_chk (int, const char *, __gnuc_va_list);
> +extern int __vfprintf_chk (FILE *, int, const char *, __gnuc_va_list);
> extern char *__fgets_unlocked_chk (char *buf, size_t size, int n, FILE *fp);
> extern char *__fgets_chk (char *buf, size_t size, int n, FILE *fp);
> extern int __asprintf_chk (char **, int, const char *, ...) __THROW;
> -extern int __vasprintf_chk (char **, int, const char *, _G_va_list) __THROW;
> +extern int __vasprintf_chk (char **, int, const char *, __gnuc_va_list) __THROW;
> extern int __dprintf_chk (int, int, const char *, ...);
> -extern int __vdprintf_chk (int, int, const char *, _G_va_list);
> +extern int __vdprintf_chk (int, int, const char *, __gnuc_va_list);
> extern int __obstack_printf_chk (struct obstack *, int, const char *, ...)
> __THROW;
> extern int __obstack_vprintf_chk (struct obstack *, int, const char *,
> - _G_va_list) __THROW;
> + __gnuc_va_list) __THROW;
>
> extern int __isoc99_fscanf (FILE *__restrict __stream,
> const char *__restrict __format, ...) __wur;
> @@ -61,12 +61,12 @@ extern int __isoc99_sscanf (const char *__restrict __s,
> const char *__restrict __format, ...) __THROW;
> extern int __isoc99_vfscanf (FILE *__restrict __s,
> const char *__restrict __format,
> - _G_va_list __arg) __wur;
> + __gnuc_va_list __arg) __wur;
> extern int __isoc99_vscanf (const char *__restrict __format,
> - _G_va_list __arg) __wur;
> + __gnuc_va_list __arg) __wur;
> extern int __isoc99_vsscanf (const char *__restrict __s,
> const char *__restrict __format,
> - _G_va_list __arg) __THROW;
> + __gnuc_va_list __arg) __THROW;
> libc_hidden_proto (__isoc99_vsscanf)
> libc_hidden_proto (__isoc99_vfscanf)
>
Ok.
> diff --git a/libio/filedoalloc.c b/libio/filedoalloc.c
> index 329843827f5..d6517e493b0 100644
> --- a/libio/filedoalloc.c
> +++ b/libio/filedoalloc.c
> @@ -93,7 +93,7 @@ _IO_file_doallocate (_IO_FILE *fp)
> local_isatty (fp->_fileno))
> fp->_flags |= _IO_LINE_BUF;
> }
> -#if _IO_HAVE_ST_BLKSIZE
> +#if defined _STATBUF_ST_BLKSIZE
> if (st.st_blksize > 0 && st.st_blksize < _IO_BUFSIZ)
> size = st.st_blksize;
> #endif
I think it is more logical to have _IO_HAVE_ST_BLKSIZE defined as 0 on generic
bits/stat.h and 1 for Linux bits/stat.h (which is also the way to we are aiming
to handle these kind of macros).
> diff --git a/libio/fileops.c b/libio/fileops.c
> index 6d5393cea3f..5fd8a9647f2 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -582,7 +582,7 @@ mmap_remap_check (_IO_FILE *fp)
> {
> /* The file added some pages. We need to remap it. */
> void *p;
> -#ifdef _G_HAVE_MREMAP
> +#if _G_HAVE_MREMAP
> p = __mremap (fp->_IO_buf_base, ROUNDED (fp->_IO_buf_end
> - fp->_IO_buf_base),
> ROUNDED (st.st_size), MREMAP_MAYMOVE);
Ok.
> diff --git a/libio/iofdopen.c b/libio/iofdopen.c
> index 3546d52b4da..b6f1500dee4 100644
> --- a/libio/iofdopen.c
> +++ b/libio/iofdopen.c
> @@ -126,13 +126,13 @@ _IO_new_fdopen (int fd, const char *mode)
> new_f->fp.file._lock = &new_f->lock;
> #endif
> _IO_no_init (&new_f->fp.file, 0, 0, &new_f->wd,
> -#ifdef _G_HAVE_MMAP
> +#if _G_HAVE_MMAP
> (use_mmap && (read_write & _IO_NO_WRITES))
> ? &_IO_wfile_jumps_maybe_mmap :
> #endif
> &_IO_wfile_jumps);
> _IO_JUMPS (&new_f->fp) =
> -#ifdef _G_HAVE_MMAP
> +#if _G_HAVE_MMAP
> (use_mmap && (read_write & _IO_NO_WRITES)) ? &_IO_file_jumps_maybe_mmap :
> #endif
> &_IO_file_jumps;
Ok.
> diff --git a/libio/iofopen.c b/libio/iofopen.c
> index d6f59497b42..2ea82e20917 100644
> --- a/libio/iofopen.c
> +++ b/libio/iofopen.c
> @@ -33,7 +33,7 @@
> _IO_FILE *
> __fopen_maybe_mmap (_IO_FILE *fp)
> {
> -#ifdef _G_HAVE_MMAP
> +#if _G_HAVE_MMAP
> if ((fp->_flags2 & _IO_FLAGS2_MMAP) && (fp->_flags & _IO_NO_WRITES))
> {
> /* Since this is read-only, we might be able to mmap the contents
Ok.
> diff --git a/libio/libio.h b/libio/libio.h
> index ff67e18c182..09531202472 100644
> --- a/libio/libio.h
> +++ b/libio/libio.h
> @@ -41,8 +41,24 @@
> # error "Someone forgot to include stdio-lock.h"
> #endif
>
> -#include <bits/_G_config.h>
> -/* ALL of these should be defined in _G_config.h */
> +#define __need_wchar_t
> +#include <stddef.h>
> +
> +#include <bits/types/__mbstate_t.h>
> +#include <bits/types/wint_t.h>
> +#include <gconv.h>
> +
> +typedef union
> +{
> + struct __gconv_info __cd;
> + struct
> + {
> + struct __gconv_info __cd;
> + struct __gconv_step_data __data;
> + } __combined;
> +} _IO_iconv_t;
> +
> +/* Map the names used in libio to the names used in libc generally. */
> #define _IO_fpos_t __fpos_t
> #define _IO_fpos64_t __fpos64_t
> #define _IO_size_t size_t
Ok.
> @@ -51,8 +67,6 @@
> #define _IO_off64_t __off64_t
> #define _IO_pid_t __pid_t
> #define _IO_uid_t __uid_t
> -#define _IO_iconv_t _G_iconv_t
> -#define _IO_HAVE_ST_BLKSIZE _G_HAVE_ST_BLKSIZE
> #define _IO_BUFSIZ BUFSIZ
> #define _IO_wint_t wint_t
> #define _IO_va_list __gnuc_va_list
Ok.
> diff --git a/libio/libioP.h b/libio/libioP.h
> index 8edc207277e..58cc9de094b 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -46,6 +46,12 @@
> #include <libio/libio.h>
> #include "iolibio.h"
>
> +#include <shlib-compat.h>
> +
> +/* For historical reasons this is the name of the sysdeps header that
> + adjusts the libio configuration. */
> +#include <_G_config.h>
> +
> #ifdef __cplusplus
> extern "C" {
> #endif
> @@ -75,16 +81,10 @@ extern "C" {
> * object being acted on (i.e. the 'this' parameter).
> */
>
> -#include <shlib-compat.h>
> -#if !SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> - /* Setting this macro disables the use of the _vtable_offset bias in
> - _IO_JUMPS_FUNCS, below. That is only needed if we want to
> - support old binaries (see oldfileops.c). */
> -# define _G_IO_NO_BACKWARD_COMPAT 1
> -#endif
> -
> -#if (!defined _IO_USE_OLD_IO_FILE \
> - && (!defined _G_IO_NO_BACKWARD_COMPAT || _G_IO_NO_BACKWARD_COMPAT == 0))
> +/* Setting this macro to 1 enables the use of the _vtable_offset bias
> + in _IO_JUMPS_FUNCS, below. This is only needed for new-format
> + _IO_FILE in libc that must support old binaries (see oldfileops.c). */
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) && !defined _IO_USE_OLD_IO_FILE
> # define _IO_JUMPS_OFFSET 1
> #else
> # define _IO_JUMPS_OFFSET 0
Ok, this was indeed a convoluted check (I had to write it down to realize
_G_IO_NO_BACKWARD_COMPAT here is superflous).
> diff --git a/sysdeps/generic/_G_config.h b/sysdeps/generic/_G_config.h
> new file mode 100644
> index 00000000000..7c917bf64c4
> --- /dev/null
> +++ b/sysdeps/generic/_G_config.h
> @@ -0,0 +1,15 @@
> +/* Configuration parameters for stdio - generic version. */
> +
> +#ifndef __G_CONFIG_H
> +#define __G_CONFIG_H 1
> +
> +/* Define to 1 if the operating system supports mmap, 0 otherwise.
> + This function is required by POSIX but might still be unavailable,
> + for instance when the hardware lacks support for virtual memory. */
> +#define _G_HAVE_MMAP 1
> +
> +/* Define to 1 if the operating system supports mremap, 0 otherwise.
> + This function is currently a Linux-specific extension. */
> +#define _G_HAVE_MREMAP 0
> +
> +#endif /* _G_config.h */
Ok.
> diff --git a/sysdeps/ieee754/ldbl-opt/nldbl-compat.h b/sysdeps/ieee754/ldbl-opt/nldbl-compat.h
> index 3b3ef731a6d..d61fbb2f649 100644
> --- a/sysdeps/ieee754/ldbl-opt/nldbl-compat.h
> +++ b/sysdeps/ieee754/ldbl-opt/nldbl-compat.h
> @@ -82,22 +82,23 @@ extern ssize_t __nldbl___vstrfmon (char *, size_t, const char *, va_list)
> /* These don't use __typeof because they were not declared by the headers,
> since we don't compile with _FORTIFY_SOURCE. */
> extern int __nldbl___vfprintf_chk (FILE *__restrict, int,
> - const char *__restrict, _G_va_list);
> + const char *__restrict, __gnuc_va_list);
> extern int __nldbl___vfwprintf_chk (FILE *__restrict, int,
> const wchar_t *__restrict, __gnuc_va_list);
> extern int __nldbl___vsprintf_chk (char *__restrict, int, size_t,
> - const char *__restrict, _G_va_list) __THROW;
> + const char *__restrict, __gnuc_va_list)
> + __THROW;
> extern int __nldbl___vsnprintf_chk (char *__restrict, size_t, int, size_t,
> - const char *__restrict, _G_va_list)
> + const char *__restrict, __gnuc_va_list)
> __THROW;
> extern int __nldbl___vswprintf_chk (wchar_t *__restrict, size_t, int, size_t,
> const wchar_t *__restrict, __gnuc_va_list)
> __THROW;
> -extern int __nldbl___vasprintf_chk (char **, int, const char *, _G_va_list)
> +extern int __nldbl___vasprintf_chk (char **, int, const char *, __gnuc_va_list)
> __THROW;
> -extern int __nldbl___vdprintf_chk (int, int, const char *, _G_va_list);
> +extern int __nldbl___vdprintf_chk (int, int, const char *, __gnuc_va_list);
> extern int __nldbl___obstack_vprintf_chk (struct obstack *, int, const char *,
> - _G_va_list) __THROW;
> + __gnuc_va_list) __THROW;
> extern void __nldbl___vsyslog_chk (int, int, const char *, va_list);
>
>
Ok.
> diff --git a/sysdeps/unix/sysv/linux/_G_config.h b/sysdeps/unix/sysv/linux/_G_config.h
> new file mode 100644
> index 00000000000..04137164816
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/_G_config.h
> @@ -0,0 +1,15 @@
> +/* Configuration parameters for stdio - Linux version. */
> +
> +#ifndef __G_CONFIG_H
> +#define __G_CONFIG_H 1
> +
> +/* Define to 1 if the operating system supports mmap, 0 otherwise.
> + This function is required by POSIX but might still be unavailable,
> + for instance when the hardware lacks support for virtual memory. */
> +#define _G_HAVE_MMAP 1
> +
> +/* Define to 1 if the operating system supports mremap, 0 otherwise.
> + This function is currently a Linux-specific extension. */
> +#define _G_HAVE_MREMAP 1
> +
> +#endif /* bits/_G_config.h */
Ok.
> diff --git a/sysdeps/unix/sysv/linux/bits/_G_config.h b/sysdeps/unix/sysv/linux/bits/_G_config.h
> deleted file mode 100644
> index 05a64acb2cf..00000000000
> --- a/sysdeps/unix/sysv/linux/bits/_G_config.h
> +++ /dev/null
> @@ -1,55 +0,0 @@
> -/* This file is needed by libio to define various configuration parameters.
> - These are always the same in the GNU C library. */
> -
> -#ifndef _BITS_G_CONFIG_H
> -#define _BITS_G_CONFIG_H 1
> -
> -/* Define types for libio in terms of the standard internal type names. */
> -
> -#include <bits/types.h>
> -#define __need_size_t
> -#if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
> -# define __need_wchar_t
> -#endif
> -#define __need_NULL
> -#include <stddef.h>
> -
> -#include <bits/types/__mbstate_t.h>
> -#include <bits/types/__fpos_t.h>
> -#include <bits/types/__fpos64_t.h>
> -
> -#define _G_fpos_t __fpos_t
> -#define _G_fpos64_t __fpos64_t
> -
> -#if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
> -# include <bits/types/wint_t.h>
> -#endif
> -
> -#if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
> -# include <gconv.h>
> -typedef union
> -{
> - struct __gconv_info __cd;
> - struct
> - {
> - struct __gconv_info __cd;
> - struct __gconv_step_data __data;
> - } __combined;
> -} _G_iconv_t;
> -#endif
> -
> -
> -/* These library features are always available in the GNU C library. */
> -#define _G_va_list __gnuc_va_list
> -
> -#define _G_HAVE_MMAP 1
> -#define _G_HAVE_MREMAP 1
> -
> -#define _G_IO_IO_FILE_VERSION 0x20001
> -
> -/* This is defined by <bits/stat.h> if `st_blksize' exists. */
> -#define _G_HAVE_ST_BLKSIZE defined (_STATBUF_ST_BLKSIZE)
> -
> -#define _G_BUFSIZ 8192
> -
> -#endif /* bits/_G_config.h */
Ok.