[1/2] Remove _IO_MTSAFE_IO from public headers.
Commit Message
_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.
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(-)
Comments
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 }
>
>
On Thu, Apr 6, 2017 at 4:29 PM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> 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?
I think that's a desirable goal, but I don't want to do that in this
patchset because I suspect it will be messy *inside* libc. And I'd
start by getting it out of the public headers, anyway.
(I'd like to stop installing libio.h at all, too, but that's probably
going to be nastier:
https://codesearch.debian.net/search?q=libio.h&perpkg=1)
On 06/04/2017 17:41, Zack Weinberg wrote:
> On Thu, Apr 6, 2017 at 4:29 PM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> 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?
>
> I think that's a desirable goal, but I don't want to do that in this
> patchset because I suspect it will be messy *inside* libc. And I'd
> start by getting it out of the public headers, anyway.
Why do you think so? The make fragment sysdeps/pthread/Makeconfig already
sets libc-reentrant regardless, so _IO_MTSAFE_IO is already being defined
in all objects that might use it (and I suspect it has been this way for
some time already).
>
> (I'd like to stop installing libio.h at all, too, but that's probably
> going to be nastier:
> https://codesearch.debian.net/search?q=libio.h&perpkg=1)
>
Agreed.
On 04/06/2017 11:29 PM, Adhemerval Zanella wrote:
>
>
> On 06/04/2017 17:41, Zack Weinberg wrote:
>> On Thu, Apr 6, 2017 at 4:29 PM, Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>> 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?
>>
>> I think that's a desirable goal, but I don't want to do that in this
>> patchset because I suspect it will be messy *inside* libc. And I'd
>> start by getting it out of the public headers, anyway.
>
> Why do you think so? The make fragment sysdeps/pthread/Makeconfig already
> sets libc-reentrant regardless, so _IO_MTSAFE_IO is already being defined
> in all objects that might use it (and I suspect it has been this way for
> some time already).
There is some oddity going on with regards to the libio locking macros.
A previous discussion:
<https://sourceware.org/ml/libc-alpha/2016-04/msg00748.html>
(spans into next month)
Obviously, we need to clean this up, but we need to be careful to
preserve the locking behavior expected by applications.
Thanks,
Florian
On 06/04/2017 19:06, Florian Weimer wrote:
> On 04/06/2017 11:29 PM, Adhemerval Zanella wrote:
>>
>>
>> On 06/04/2017 17:41, Zack Weinberg wrote:
>>> On Thu, Apr 6, 2017 at 4:29 PM, Adhemerval Zanella
>>> <adhemerval.zanella@linaro.org> wrote:
>>>> 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?
>>>
>>> I think that's a desirable goal, but I don't want to do that in this
>>> patchset because I suspect it will be messy *inside* libc. And I'd
>>> start by getting it out of the public headers, anyway.
>>
>> Why do you think so? The make fragment sysdeps/pthread/Makeconfig already
>> sets libc-reentrant regardless, so _IO_MTSAFE_IO is already being defined
>> in all objects that might use it (and I suspect it has been this way for
>> some time already).
>
> There is some oddity going on with regards to the libio locking macros. A previous discussion:
>
> <https://sourceware.org/ml/libc-alpha/2016-04/msg00748.html>
> (spans into next month)
>
> Obviously, we need to clean this up, but we need to be careful to preserve the locking behavior expected by applications.
>
> Thanks,
> Florian
Right, removing _IO_MTSAFE_IO seems no be not a trivial issue. I think
we can push it in the future then.
@@ -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
@@ -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);
@@ -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
@@ -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 }