Re: Improvements to fork handling (2/5)
On May 22 14:42, Ryan Johnson wrote:
> On 21/05/2011 9:44 PM, Christopher Faylor wrote:
> >On Wed, May 11, 2011 at 02:31:37PM -0400, Ryan Johnson wrote:
> >>Hi all,
> >>
> >>This patch has the parent sort its dll list topologically by
> >>dependencies. Previously, attempts to load a DLL_LOAD dll risked pulling
> >>in dependencies automatically, and the latter would then not benefit
> >>from the code which "encourages" them to land in the right places. The
> >>dependency tracking is achieved using a simple class which allows to
> >>introspect a mapped dll image and pull out the dependencies it lists.
> >>The code currently rebuilds the dependency list at every fork rather
> >>than attempt to update it properly as modules are loaded and unloaded.
> >>Note that the topsort optimization affects only cygwin dlls, so any
> >>windows dlls which are pulled in dynamically (directly or indirectly)
> >>will still impose the usual risk of address space clobbers.
> >This seems CPU and memory intensive during a time for which we already
> >know is very slow. Is the benefit really worth it? How much more robust
> >does it make forking?
> Topological sorting is O(n), so there's no asymptotic change in
> performance. Looking up dependencies inside a dll is *very* cheap
> (2-3 pointer dereferences per dep), and all of this only happens for
> dynamically-loaded dlls. Given the number of calls to
> Virtual{Alloc,Query,Free} and LoadDynamicLibraryEx which we make, I
> would be surprised if the topsort even registered. That said, it is
> extra work and will slow down fork.
>
> I have not been able to test how much it helps, but it should help
> with the test case Jon Turney reported with python a while back [1].
> In fact, it was that example which made me aware of the potential
> need for a topsort in the first place.
>
> In theory, this should completely eliminate the case where us
> loading one DLL pulls in dependencies automatically (= uncontrolled
> and at Windows' whim). The problem would manifest as a DLL which
> "loads" in the same wrong place repeatedly when given the choice,
> and for which we would be unable to VirtualAlloc the offending spot
> (because the dll in question has non-zero refcount even after we
> unload it, due to the dll(s) that depend on it.
There might be a way around this. It seems to be possible to tweak
the module list the PEB points to so that you can unload a library
even though it has dependencies. Then you can block the unwanted
space and call LoadLibrary again. See (*) for a discussion how
you can unload the exe itself to reload another one. Maybe that's
something we can look into as well. ObNote: Of course, if we
could influnce the address at which a DLL gets loaded right from the
start, it would be the preferrable solution.
Corinna
(*) http://www.blizzhackers.cc/viewtopic.php?p=4332690
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Project Co-Leader cygwin AT cygwin DOT com
Red Hat
Re: __xpg_strerror_r should not clobber strerror buffer
On 05/21/2011 07:35 PM, Christopher Faylor wrote: > On Sat, May 21, 2011 at 07:26:37PM -0600, Eric Blake wrote: >> POSIX says that no other function in the standard should clobber the >> strerror buffer. Our strerror_r is a GNU extension, so it can get away >> with clobbering the buffer (but if we wanted to fix it, we would have to >> separate _my_tls.locals.strerror_buf into two different buffers). >> perror() is still broken, but that needs to be fixed in newlib. But >> __xpg_strerror_r, which is our POSIX strerror_r variant, has to be fixed >> in cygwin. >> >> Meanwhile, glibc just patched strerror this week to print negative >> errnum as a negative 32-bit int, rather than as a positive unsigned >> long; cygwin should do likewise. >> >> 2011-05-21 Eric Blake >> >> * errno.cc (strerror): Print unknown errno as int. >> (__xpg_strerror_r): Likewise, and don't clobber strerror buffer. > > Looks good. Please check in. Pushed. Just for the record, I'm having a problem self-building cygwin right now, from what looks like mingw issues: /home/eblake/src/winsup/utils/mingw gcc-4 -B./ -shared -Wl,--image-base,0x6FBC -Wl,--entry,_DllMainCRTStartup@12 mthr.o mthr_init.o mingwthrd.def -Lmingwex -o mingwm10.dll mingwex/libmingwex.a(strtodnrp.o): In function `strtod': /home/eblake/src/build/i686-pc-cygwin/winsup/mingw/mingwex/../../../../../winsup/mingw/include/stdlib.h:315: multiple definition of `_strtod' ... /usr/lib/gcc/i686-pc-cygwin/4.3.4/../../../../i686-pc-cygwin/bin/ld: warning: cannot find entry symbol _DllMainCRTStartup@12; defaulting to 6fbc1000 ertr01.o:(.rdata+0x0): undefined reference to `__pei386_runtime_relocator' collect2: ld returned 1 exit status make[3]: *** [mingwm10.dll] Error 1 make[3]: Leaving directory `/home/eblake/src/build/i686-pc-cygwin/winsup/mingw' How do we go about getting that resolved? -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: __xpg_strerror_r should not clobber strerror buffer
On 05/23/2011 02:45 PM, Eric Blake wrote:
> On 05/21/2011 07:35 PM, Christopher Faylor wrote:
>> On Sat, May 21, 2011 at 07:26:37PM -0600, Eric Blake wrote:
>>> POSIX says that no other function in the standard should clobber the
>>> strerror buffer. Our strerror_r is a GNU extension, so it can get away
>>> with clobbering the buffer (but if we wanted to fix it, we would have to
>>> separate _my_tls.locals.strerror_buf into two different buffers).
Shoot. This introduced an off-by-one buffer overrun. I'm pushing this
followup. Meanwhile, do we want a second buffer, so that the GNU
strerror_r won't clobber the strerror buffer?
+++ b/winsup/cygwin/ChangeLog
@@ -2,6 +2,7 @@
* errno.cc (strerror): Print unknown errno as int.
(__xpg_strerror_r): Likewise, and don't clobber strerror buffer.
+ * cygtls.h (strerror_buf): Resize to allow '-'.
2011-05-23 Corinna Vinschen
diff --git a/winsup/cygwin/cygtls.h b/winsup/cygwin/cygtls.h
index 4d4306b..f911a6c 100644
--- a/winsup/cygwin/cygtls.h
+++ b/winsup/cygwin/cygtls.h
@@ -109,7 +109,7 @@ struct _local_storage
} select;
/* strerror */
- char strerror_buf[sizeof ("Unknown error 4294967295")];
+ char strerror_buf[sizeof ("Unknown error -2147483648")];
/* times.cc */
char timezone_buf[20];
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: __xpg_strerror_r should not clobber strerror buffer
On Mon, May 23, 2011 at 02:45:43PM -0600, Eric Blake wrote: >Just for the record, I'm having a problem self-building cygwin right >now, from what looks like mingw issues: > >/home/eblake/src/winsup/utils/mingw gcc-4 -B./ -shared >-Wl,--image-base,0x6FBC -Wl,--entry,_DllMainCRTStartup@12 mthr.o >mthr_init.o mingwthrd.def -Lmingwex -o mingwm10.dll >mingwex/libmingwex.a(strtodnrp.o): In function `strtod': >/home/eblake/src/build/i686-pc-cygwin/winsup/mingw/mingwex/../../../../../winsup/mingw/include/stdlib.h:315: >multiple definition of `_strtod' >... >/usr/lib/gcc/i686-pc-cygwin/4.3.4/../../../../i686-pc-cygwin/bin/ld: >warning: cannot find entry symbol _DllMainCRTStartup@12; defaulting to >6fbc1000 >ertr01.o:(.rdata+0x0): undefined reference to >`__pei386_runtime_relocator' >collect2: ld returned 1 exit status >make[3]: *** [mingwm10.dll] Error 1 >make[3]: Leaving directory >`/home/eblake/src/build/i686-pc-cygwin/winsup/mingw' > >How do we go about getting that resolved? I sent email to ironh34d regarding the strtod error after you mentioned this on irc. I added an "extern" in front of the strtod definition in stdlib.h and that allowed mingw to build but I don't know if it is the right fix or not. cgf
Re: __xpg_strerror_r should not clobber strerror buffer
On Mon, May 23, 2011 at 02:52:12PM -0600, Eric Blake wrote: >On 05/23/2011 02:45 PM, Eric Blake wrote: >> On 05/21/2011 07:35 PM, Christopher Faylor wrote: >>> On Sat, May 21, 2011 at 07:26:37PM -0600, Eric Blake wrote: POSIX says that no other function in the standard should clobber the strerror buffer. Our strerror_r is a GNU extension, so it can get away with clobbering the buffer (but if we wanted to fix it, we would have to separate _my_tls.locals.strerror_buf into two different buffers). > >Shoot. This introduced an off-by-one buffer overrun. I'm pushing this >followup. Meanwhile, do we want a second buffer, so that the GNU >strerror_r won't clobber the strerror buffer? > >+++ b/winsup/cygwin/ChangeLog >@@ -2,6 +2,7 @@ > > * errno.cc (strerror): Print unknown errno as int. > (__xpg_strerror_r): Likewise, and don't clobber strerror buffer. >+ * cygtls.h (strerror_buf): Resize to allow '-'. Please don't send ChangeLog diffs. The patch is approved. I don't know if I care whether strerror_r clobbers strerror. Thanks. cgf
fix perror POSIX compliance
This depends on the newlib patch:
http://sourceware.org/ml/newlib/2011/msg00215.html
In fact, if that patch goes in, then this one is required to avoid a
link failure; this one can probably go in first but makes no difference
to perror without the newlib patch.
winsup/cygwin/ChangeLog|6 +++
winsup/cygwin/cygtls.h |1 +
winsup/cygwin/errno.cc | 43 ---
winsup/cygwin/tlsoffsets.h | 82
++--
4 files changed, 78 insertions(+), 54 deletions(-)
2011-05-23 Eric Blake
* cygtls.h (strerror_r_buf): New buffer.
* errno.cc (strerror): Move guts...
(_strerror_r): ...to new function demanded by newlib.
(strerror_r): Don't clobber strerror buffer.
(_user_strerror): Drop unused declaration.
* tlsoffsets.h: Regenerate.
diff --git a/winsup/cygwin/cygtls.h b/winsup/cygwin/cygtls.h
index 6359e7c..a1642b1 100644
--- a/winsup/cygwin/cygtls.h
+++ b/winsup/cygwin/cygtls.h
@@ -110,6 +110,7 @@ struct _local_storage
/* strerror errno.cc */
char strerror_buf[sizeof ("Unknown error -2147483648")];
+ char strerror_r_buf[sizeof ("Unknown error -2147483648")];
/* times.cc */
char timezone_buf[20];
diff --git a/winsup/cygwin/errno.cc b/winsup/cygwin/errno.cc
index aa311b7..f3b925e 100644
--- a/winsup/cygwin/errno.cc
+++ b/winsup/cygwin/errno.cc
@@ -360,8 +360,6 @@ seterrno (const char *file, int line)
seterrno_from_win_error (file, line, GetLastError ());
}
-extern char *_user_strerror _PARAMS ((int));
-
static char *
strerror_worker (int errnum)
{
@@ -373,22 +371,38 @@ strerror_worker (int errnum)
return res;
}
-/* strerror: convert from errno values to error strings. Newlib's
- strerror_r returns "" for unknown values, so we override it to
- provide a nicer thread-safe result string and set errno. */
+/* Newlib requires this override for perror and friends to avoid
+ clobbering strerror() buffer, without having to differentiate
+ between strerror_r signatures. This function is intentionally not
+ exported, so that only newlib can use it. */
extern "C" char *
-strerror (int errnum)
+_strerror_r (struct _reent *, int errnum, int internal, int *errptr)
{
char *errstr = strerror_worker (errnum);
if (!errstr)
{
- __small_sprintf (errstr = _my_tls.locals.strerror_buf, "Unknown
error %d",
- errnum);
- errno = _impure_ptr->_errno = EINVAL;
+ errstr = internal ? _my_tls.locals.strerror_r_buf
+: _my_tls.locals.strerror_buf;
+ __small_sprintf (errstr, "Unknown error %d", errnum);
+ if (errptr)
+*errptr = EINVAL;
}
return errstr;
}
+/* strerror: convert from errno values to error strings. Newlib's
+ strerror_r returns "" for unknown values, so we override it to
+ provide a nicer thread-safe result string and set errno. */
+extern "C" char *
+strerror (int errnum)
+{
+ int error = 0;
+ char *result = _strerror_r (NULL, errnum, 0, &error);
+ if (error)
+set_errno (error);
+ return result;
+}
+
/* Newlib's provides declarations for two strerror_r
variants, according to preprocessor feature macros. However, it
returns "" instead of "Unknown error ...", so we override both
@@ -396,10 +410,13 @@ strerror (int errnum)
extern "C" char *
strerror_r (int errnum, char *buf, size_t n)
{
- char *error = strerror (errnum);
- if (strlen (error) >= n)
-return error;
- return strcpy (buf, error);
+ int error = 0;
+ char *errstr = _strerror_r (NULL, errnum, 1, &error);
+ if (error)
+set_errno (error);
+ if (strlen (errstr) >= n)
+return errstr;
+ return strcpy (buf, errstr);
}
extern "C" int
diff --git a/winsup/cygwin/tlsoffsets.h b/winsup/cygwin/tlsoffsets.h
index 4e459df..3ef7963 100644
--- a/winsup/cygwin/tlsoffsets.h
+++ b/winsup/cygwin/tlsoffsets.h
@@ -1,6 +1,6 @@
//;# autogenerated: Do not edit.
-//; $tls::sizeof__cygtls = 3984;
+//; $tls::sizeof__cygtls = 4012;
... // rest of generated file patch elided
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
