From owner-freebsd-threads@FreeBSD.ORG Thu Jan 7 18:15:27 2010 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D7F51106566C; Thu, 7 Jan 2010 18:15:27 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 9BC628FC18; Thu, 7 Jan 2010 18:15:27 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 3183346B46; Thu, 7 Jan 2010 13:15:27 -0500 (EST) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 71CFB8A01D; Thu, 7 Jan 2010 13:15:26 -0500 (EST) From: John Baldwin To: Jeremy Huddleston Date: Thu, 7 Jan 2010 13:15:25 -0500 User-Agent: KMail/1.12.1 (FreeBSD/7.2-CBSD-20091103; KDE/4.3.1; amd64; ; ) References: <200912052034.nB5KYfaY000395@www.freebsd.org> <201001070956.25906.jhb@freebsd.org> <8C235DD0-74DA-4E15-A722-17772E8089DE@apple.com> In-Reply-To: <8C235DD0-74DA-4E15-A722-17772E8089DE@apple.com> MIME-Version: 1.0 Message-Id: <201001071315.26022.jhb@freebsd.org> Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Thu, 07 Jan 2010 13:15:26 -0500 (EST) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: freebsd-gnats-submit@freebsd.org, freebsd-threads@freebsd.org Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Jan 2010 18:15:27 -0000 On Thursday 07 January 2010 10:44:54 am Jeremy Huddleston wrote: > > On Jan 7, 2010, at 09:56, John Baldwin wrote: > > >> vasprintf.c.patch:+ INITEXTRA(&f); > >> vdprintf.c.patch:+ INITEXTRA(&f); > >> vfprintf.c.patch:+ INITEXTRA(&fake); > >> vfwprintf.c.patch:+ INITEXTRA(&fake); > >> vsnprintf.c.patch:+ INITEXTRA(&f); > >> vsprintf.c.patch:+ INITEXTRA(&f); > >> vsscanf.c.patch:+ INITEXTRA(&f); > >> vswprintf.c.patch:+ INITEXTRA(&f); > >> vswscanf.c.patch:+ INITEXTRA(&f); > > > > Ah, ok. In our stdio at least these are all dummy files that are passed to > > internal stdio routines that never do any locking (e.g. __vfprintf() which > > does no locking vs vfprintf() which does use the mutex locks). I'm not sure > > if that is also true for Darwin, but in theory it should be as these file > > objects are all private stack variables that no other threads can gain a > > reference to, so no locking is needed. > > Yeah, we're just being cautious with these changes. It takes one clock cycle > and maintains the old (FBSD 7?) state of initializing the mutex during > INITEXTRA in those dumies... just in case something gets added down the line > which needs it. > > If you're confident that won't be the case for FBSD, then I believe your > initial patch is sufficient. Ok. I actually think it would be nicer to provide some sort of macro to properly initialize the various 'fake' FILE objects. I will probably look at doing that in which case including an initializer for the lock should be simple. I actually don't like the amount of duplicated code to setup fake FILE objects as it is. Here is a larger patch which does this in addition to the earlier patch. Index: stdio/vsnprintf.c =================================================================== --- stdio/vsnprintf.c (revision 201516) +++ stdio/vsnprintf.c (working copy) @@ -47,7 +47,7 @@ size_t on; int ret; char dummy[2]; - FILE f; + FILE f = FAKE_FILE; on = n; if (n != 0) @@ -61,12 +61,9 @@ str = dummy; n = 1; } - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = n; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (on > 0) *f._p = '\0'; Index: stdio/vswprintf.c =================================================================== --- stdio/vswprintf.c (revision 201516) +++ stdio/vswprintf.c (working copy) @@ -45,7 +45,7 @@ { static const mbstate_t initial; mbstate_t mbs; - FILE f; + FILE f = FAKE_FILE; char *mbp; int ret, sverrno; size_t nwc; @@ -55,7 +55,6 @@ return (-1); } - f._file = -1; f._flags = __SWR | __SSTR | __SALC; f._bf._base = f._p = (unsigned char *)malloc(128); if (f._bf._base == NULL) { @@ -63,8 +62,6 @@ return (-1); } f._bf._size = f._w = 127; /* Leave room for the NUL */ - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfwprintf(&f, fmt, ap); if (ret < 0) { sverrno = errno; Index: stdio/vsscanf.c =================================================================== --- stdio/vsscanf.c (revision 201516) +++ stdio/vsscanf.c (working copy) @@ -55,16 +55,11 @@ vsscanf(const char * __restrict str, const char * __restrict fmt, __va_list ap) { - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SRD; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._r = strlen(str); f._read = eofread; - f._ub._base = NULL; - f._lb._base = NULL; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); return (__svfscanf(&f, fmt, ap)); } Index: stdio/snprintf.c =================================================================== --- stdio/snprintf.c (revision 201516) +++ stdio/snprintf.c (working copy) @@ -48,7 +48,7 @@ size_t on; int ret; va_list ap; - FILE f; + FILE f = FAKE_FILE; on = n; if (n != 0) @@ -56,12 +56,9 @@ if (n > INT_MAX) n = INT_MAX; va_start(ap, fmt); - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = n; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (on > 0) *f._p = '\0'; Index: stdio/xprintf.c =================================================================== --- stdio/xprintf.c (revision 201516) +++ stdio/xprintf.c (working copy) @@ -48,6 +48,7 @@ #include #include "un-namespace.h" +#include "local.h" #include "printf.h" #include "fvwrite.h" @@ -575,7 +576,7 @@ __v3printf(FILE *fp, const char *fmt, int pct, va_list ap) { int ret; - FILE fake; + FILE fake = FAKE_FILE; unsigned char buf[BUFSIZ]; /* copy the important variables */ Index: stdio/vdprintf.c =================================================================== --- stdio/vdprintf.c (revision 201516) +++ stdio/vdprintf.c (working copy) @@ -39,7 +39,7 @@ int vdprintf(int fd, const char * __restrict fmt, va_list ap) { - FILE f; + FILE f = FAKE_FILE; unsigned char buf[BUFSIZ]; int ret; @@ -56,8 +56,6 @@ f._write = __swrite; f._bf._base = buf; f._bf._size = sizeof(buf); - f._orientation = 0; - bzero(&f._mbstate, sizeof(f._mbstate)); if ((ret = __vfprintf(&f, fmt, ap)) < 0) return (ret); Index: stdio/vfprintf.c =================================================================== --- stdio/vfprintf.c (revision 201516) +++ stdio/vfprintf.c (working copy) @@ -169,7 +169,7 @@ __sbprintf(FILE *fp, const char *fmt, va_list ap) { int ret; - FILE fake; + FILE fake = FAKE_FILE; unsigned char buf[BUFSIZ]; /* XXX This is probably not needed. */ Index: stdio/local.h =================================================================== --- stdio/local.h (revision 201516) +++ stdio/local.h (working copy) @@ -110,6 +110,14 @@ } /* + * Structure initializations for 'fake' FILE objects. + */ +#define FAKE_FILE { \ + ._file = -1, \ + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ +} + +/* * Set the orientation for a stream. If o > 0, the stream has wide- * orientation. If o < 0, the stream has byte-orientation. */ Index: stdio/vsprintf.c =================================================================== --- stdio/vsprintf.c (revision 201516) +++ stdio/vsprintf.c (working copy) @@ -44,14 +44,11 @@ vsprintf(char * __restrict str, const char * __restrict fmt, __va_list ap) { int ret; - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = INT_MAX; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); *f._p = 0; return (ret); Index: stdio/findfp.c =================================================================== --- stdio/findfp.c (revision 201516) +++ stdio/findfp.c (working copy) @@ -61,6 +61,7 @@ ._read = __sread, \ ._seek = __sseek, \ ._write = __swrite, \ + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ } /* the usual - (stdin + stdout + stderr) */ static FILE usual[FOPEN_MAX - 3]; @@ -96,7 +97,7 @@ int n; { struct glue *g; - static FILE empty; + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; FILE *p; g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); @@ -154,7 +155,7 @@ fp->_ub._size = 0; fp->_lb._base = NULL; /* no line buffer */ fp->_lb._size = 0; -/* fp->_lock = NULL; */ /* once set always set (reused) */ +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ fp->_orientation = 0; memset(&fp->_mbstate, 0, sizeof(mbstate_t)); return (fp); Index: stdio/vasprintf.c =================================================================== --- stdio/vasprintf.c (revision 201516) +++ stdio/vasprintf.c (working copy) @@ -42,9 +42,8 @@ __va_list ap; { int ret; - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SWR | __SSTR | __SALC; f._bf._base = f._p = (unsigned char *)malloc(128); if (f._bf._base == NULL) { @@ -53,8 +52,6 @@ return (-1); } f._bf._size = f._w = 127; /* Leave room for the NUL */ - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (ret < 0) { free(f._bf._base); Index: stdio/vswscanf.c =================================================================== --- stdio/vswscanf.c (revision 201516) +++ stdio/vswscanf.c (working copy) @@ -62,7 +62,7 @@ { static const mbstate_t initial; mbstate_t mbs; - FILE f; + FILE f = FAKE_FILE; char *mbstr; size_t mlen; int r; @@ -80,15 +80,10 @@ free(mbstr); return (EOF); } - f._file = -1; f._flags = __SRD; f._bf._base = f._p = (unsigned char *)mbstr; f._bf._size = f._r = mlen; f._read = eofread; - f._ub._base = NULL; - f._lb._base = NULL; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); r = __vfwscanf(&f, fmt, ap); free(mbstr); -- John Baldwin