This is the mail archive of the cygwin-patches@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] pthread_fork


On Thu, Aug 15, 2002 at 08:27:38PM +0200, Thomas Pfaff wrote:
>
>This patch will fix the pthread key related problems with fork (key value
>is restored after fork) and some minor fork related fixes.
>
>Changelog:
>
>2002-08-15  Thomas Pfaff  <tpfaff@gmx.net>
>
>	* fork.cc (fork_child): fixup_after_fork call changed.
>	(fork_parent): Added call to MTinterface->fixup_before_fork to
>	save TLS values.

Sorry, but this is incorrect wording for a ChangeLog.  It should be
something like:

	* fork.cc (fork_child): Remove extra thread related fork fixup.
	(fork_parent): Add call to MTinterface->fixup_before_fork to
	save TLS values.

The tense should be "present" as in say "Add" rather than "Added". I can
see that a few incorrect usages have slipped in but I would appreciate
it if future submissions adhere to this policy.

And, FWIW, I guarantee that I will flame anyone within an inch of their
lives if they scour old ChangeLog entries trying to find occurrences
that are counter to this rule.

>diff -urp src.old/winsup/cygwin/fork.cc src/winsup/cygwin/fork.cc
>--- src.old/winsup/cygwin/fork.cc	Wed Aug 14 14:20:24 2002
>+++ src/winsup/cygwin/fork.cc	Wed Aug  7 17:14:54 2002
>@@ -652,11 +654,15 @@ fork ()
>   child_info_fork ch;
> 
>   int res = setjmp (ch.jmp);
>-
>   if (res)
>     res = fork_child (grouped.hParent, grouped.first_dll, grouped.load_dlls);
>   else
>-    res = fork_parent (grouped.hParent, grouped.first_dll, grouped.load_dlls, esp, ch);
>+    {
>+      /* Protect pthread_keys local buf from being overwritten by simultanous forks */
>+      EnterCriticalSection (&MT_INTERFACE->fork_lock);
>+      res = fork_parent (grouped.hParent, grouped.first_dll, grouped.load_dlls, esp, ch);
>+      LeaveCriticalSection (&MT_INTERFACE->fork_lock);
>+    }

Please just add additional locks close to the existing malloc lock/unlock.  It doesn't make
sense for the critical section to be the entire fork_parent function. 

>   MALLOC_CHECK;
>   syscall_printf ("%d = fork()", res);
>diff -urp src.old/winsup/cygwin/init.cc src/winsup/cygwin/init.cc
>--- src.old/winsup/cygwin/init.cc	Wed Aug 14 14:20:24 2002
>+++ src/winsup/cygwin/init.cc	Wed Aug 14 14:23:30 2002
>@@ -18,6 +18,9 @@ int NO_COPY dynamically_loaded;
> extern "C" int
> WINAPI dll_entry (HANDLE h, DWORD reason, void *static_load)
> {
>+  if (reason == DLL_THREAD_DETACH || reason == DLL_PROCESS_DETACH)
>+    MT_INTERFACE->run_key_dtors ();
>+

An if just before a switch that deals with the same variable?  Use the
switch/case, please.  You can remove the FIXME: block, if that helps.

cgf


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]