Message ID | 20170322125504.4863-2-zackw@panix.com |
---|---|
State | New |
Headers | show |
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.
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 }