Re: [PATCH 1/2] Remove _IO_MTSAFE_IO from public headers.

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 22/03/2017 09:55, Zack Weinberg wrote:
> _IO_MTSAFE_IO controls whether stdio is *built* with support for
> multithreading.  In the distant past it might also have worked as a
> feature selection macro, allowing library *users* to select
> thread-safe or lock-free stdio at application build time, I haven't
> done the archaeology.  Nowadays, defining _IO_MTSAFE_IO while using
> the installed headers, or in _ISOMAC mode, will cause libio.h to throw
> syntax errors.

What prevent us to just get rid of _IO_MTSAFE_IO and just build/assume
stdio with multithread support? Checking on debian code search [1], only
newlib, glibc, uclibc, hurd, and libconvert-binary-c-perl seems to
use/define it:

  * newlib have an explicit comment stating it expects multithread
    support.

newlib_2.4.0.20160527-2/newlib/libc/sys/linux/bits/libc-lock.h
#endif
#define _IO_MTSAFE_IO  /* add this as we always want this in newlib */
/* Mutex type.  */

  * uclibc is basically some old glibc code.

  * hurd also expected the same as newlib:

hurd_1:0.9.git20170310-1/libthreads/lockfile.c
   Boston, MA 02111-1307, USA.  */
#define _IO_MTSAFE_IO
#define IS_IN(lib) 0

  * libconvert-binary-c-perl just uses it on some internal testing where
    I am not sure if the correct way to use an internal glibc header.

[1] https://codesearch.debian.net/search?q=_IO_MTSAFE_IO

> 
> This patch removes _IO_MTSAFE_IO from the public headers
> (specifically, from libio/libio.h).  The most important thing it
> controlled in there was whether libio.h defines _IO_lock_t itself or
> expects stdio-lock.h to have done it, and we do still need a
> inter-header communication macro for that, because stdio-lock.h can
> only define _IO_lock_t as a typedef.  I've invented
> _IO_lock_t_defined, which is defined by both versions of stdio-lock.h.
> 
> _IO_MTSAFE_IO also controlled the definitions of a handful of macros
> that _might_ count as part of the public libio.h interface.  They are
> now unconditionally given their non-_IO_MTSAFE_IO definition in
> libio/libio.h, and include/libio.h redefines them with the
> _IO_MTSAFE_IO definition.  This should minimize the odds of breaking
> old software that actually uses those macros.
> 
> I suspect that this entire mechanism is vestigial, and that glibc
> won't build anymore if you *don't* define _IO_MTSAFE_IO, but that's
> another patchset.  The bulk of libio.h is internal-use-only stuff that
> no longer makes sense to expose (libstdc++ gave up on making a FILE
> the same object as a C++ filebuf *decades* ago) but that, too, is
> another patchset.
> 
> 	* libio/libio.h: Condition dummy definition of _IO_lock_t on
> 	_IO_lock_t_defined, not _IO_MTSAFE_IO. Unconditionally use the
> 	non-_IO_MTSAFE_IO definitions for _IO_peekc, _IO_flockfile,
> 	_IO_funlockfile, and _IO_ftrylockfile.  Only define
> 	_IO_cleanup_region_start and _IO_cleanup_region_end if not
> 	already defined.
> 	* include/libio.h: If _IO_MTSAFE_IO is defined, redefine
>         _IO_peekc, _IO_flockfile, _IO_funlockfile, and _IO_ftrylockfile
>         appropriately.
> 	* sysdeps/generic/stdio-lock.h, sysdeps/nptl/stdio-lock.h:
> 	Define _IO_lock_t_defined after defining _IO_lock_t.
> ---
>  include/libio.h              | 25 +++++++++++++++++--------
>  libio/libio.h                | 31 +++++++++++++------------------
>  sysdeps/generic/stdio-lock.h |  1 +
>  sysdeps/nptl/stdio-lock.h    |  1 +
>  4 files changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/include/libio.h b/include/libio.h
> index 97fc5b548b..d2fa796758 100644
> --- a/include/libio.h
> +++ b/include/libio.h
> @@ -21,16 +21,25 @@ libc_hidden_proto (_IO_sgetn)
>  libc_hidden_proto (_IO_vfprintf)
>  libc_hidden_proto (_IO_vfscanf)
>  
> -#if defined _IO_MTSAFE_IO && _IO_lock_inexpensive
> +#ifdef _IO_MTSAFE_IO
> +# undef _IO_peekc
>  # undef _IO_flockfile
> -# define _IO_flockfile(_fp) \
> -  if (((_fp)->_flags & _IO_USER_LOCK) == 0)				      \
> -     _IO_lock_lock (*(_fp)->_lock)
>  # undef _IO_funlockfile
> -# define _IO_funlockfile(_fp) \
> -  if (((_fp)->_flags & _IO_USER_LOCK) == 0)				      \
> -    _IO_lock_unlock (*(_fp)->_lock)
> -#endif
> +# undef _IO_ftrylockfile
> +
> +# define _IO_peekc(_fp) _IO_peekc_locked (_fp)
> +# if _IO_lock_inexpensive
> +#  define _IO_flockfile(_fp) \
> +  if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_lock (*(_fp)->_lock)
> +#  define _IO_funlockfile(_fp) \
> +  if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_unlock (*(_fp)->_lock)
> +# else
> +#  define _IO_flockfile(_fp) \
> +  if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_flockfile (_fp)
> +#  define _IO_funlockfile(_fp) \
> +  if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_funlockfile (_fp)
> +# endif
> +#endif /* _IO_MTSAFE_IO */
>  
>  #endif
>  #endif
> diff --git a/libio/libio.h b/libio/libio.h
> index 2241c1471d..518ffd8e44 100644
> --- a/libio/libio.h
> +++ b/libio/libio.h
> @@ -143,10 +143,9 @@
>  
>  struct _IO_jump_t;  struct _IO_FILE;
>  
> -/* Handle lock.  */
> -#ifdef _IO_MTSAFE_IO
> -/* _IO_lock_t defined in internal headers during the glibc build.  */
> -#else
> +/* During the build of glibc itself, _IO_lock_t will already have been
> +   defined by internal headers.  */
> +#ifndef _IO_lock_t_defined
>  typedef void _IO_lock_t;
>  #endif
>  
> @@ -441,20 +440,16 @@ extern void _IO_flockfile (_IO_FILE *) __THROW;
>  extern void _IO_funlockfile (_IO_FILE *) __THROW;
>  extern int _IO_ftrylockfile (_IO_FILE *) __THROW;
>  
> -#ifdef _IO_MTSAFE_IO
> -# define _IO_peekc(_fp) _IO_peekc_locked (_fp)
> -# define _IO_flockfile(_fp) \
> -  if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_flockfile (_fp)
> -# define _IO_funlockfile(_fp) \
> -  if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_funlockfile (_fp)
> -#else
> -# define _IO_peekc(_fp) _IO_peekc_unlocked (_fp)
> -# define _IO_flockfile(_fp) /**/
> -# define _IO_funlockfile(_fp) /**/
> -# define _IO_ftrylockfile(_fp) /**/
> -# define _IO_cleanup_region_start(_fct, _fp) /**/
> -# define _IO_cleanup_region_end(_Doit) /**/
> -#endif /* !_IO_MTSAFE_IO */
> +#define _IO_peekc(_fp) _IO_peekc_unlocked (_fp)
> +#define _IO_flockfile(_fp) /**/
> +#define _IO_funlockfile(_fp) /**/
> +#define _IO_ftrylockfile(_fp) /**/
> +#ifndef _IO_cleanup_region_start
> +#define _IO_cleanup_region_start(_fct, _fp) /**/
> +#endif
> +#ifndef _IO_cleanup_region_end
> +#define _IO_cleanup_region_end(_Doit) /**/
> +#endif
>  
>  extern int _IO_vfscanf (_IO_FILE * __restrict, const char * __restrict,
>  			_IO_va_list, int *__restrict);
> diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h
> index 532bb5f23f..763c7d465a 100644
> --- a/sysdeps/generic/stdio-lock.h
> +++ b/sysdeps/generic/stdio-lock.h
> @@ -22,6 +22,7 @@
>  #include <libc-lock.h>
>  
>  __libc_lock_define_recursive (typedef, _IO_lock_t)
> +#define _IO_lock_t_defined 1
>  
>  /* We need recursive (counting) mutexes.  */
>  #ifdef _LIBC_LOCK_RECURSIVE_INITIALIZER
> diff --git a/sysdeps/nptl/stdio-lock.h b/sysdeps/nptl/stdio-lock.h
> index 35d6ce0a29..df749062ea 100644
> --- a/sysdeps/nptl/stdio-lock.h
> +++ b/sysdeps/nptl/stdio-lock.h
> @@ -27,6 +27,7 @@
>  #define _IO_lock_inexpensive	1
>  
>  typedef struct { int lock; int cnt; void *owner; } _IO_lock_t;
> +#define _IO_lock_t_defined 1
>  
>  #define _IO_lock_initializer { LLL_LOCK_INITIALIZER, 0, NULL }
>  
>