Mailing-List: contact cygwin-developers-help AT sourceware DOT cygnus DOT com; run by ezmlm
List-Unsubscribe: <mailto:cygwin-developers-unsubscribe-archive-cygwin-developers=delorie DOT com AT sourceware DOT cygnus DOT com>
List-Subscribe: <mailto:cygwin-developers-subscribe AT sourceware DOT cygnus DOT com>
List-Archive: <http://sourceware.cygnus.com/ml/cygwin-developers/>
List-Post: <mailto:cygwin-developers AT sourceware DOT cygnus DOT com>
List-Help: <mailto:cygwin-developers-help AT sourceware DOT cygnus DOT com>, <http://sourceware.cygnus.com/ml/#faqs>
Sender: cygwin-developers-owner AT sourceware DOT cygnus DOT com
Delivered-To: mailing list cygwin-developers AT sourceware DOT cygnus DOT com
Date: Thu, 16 Sep 1999 22:43:34 -0400
From: Chris Faylor <cgf AT cygnus DOT com>
To: Mumit Khan <khan AT xraylith DOT wisc DOT EDU>
Cc: cygwin-developers AT sourceware DOT cygnus DOT com
Subject: Re: [take 2] Re: (patch) makethread stdcall/cdecl confusion
Message-ID: <19990916224334.A5928@cygnus.com>
Mail-Followup-To: Mumit Khan <khan AT xraylith DOT wisc DOT EDU>,
	cygwin-developers AT sourceware DOT cygnus DOT com
References: <19990916220217 DOT A5792 AT cygnus DOT com> <199909170112 DOT UAA12971 AT mercury DOT xraylith DOT wisc DOT edu>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
X-Mailer: Mutt 0.95.6i
In-Reply-To: <199909170112.UAA12971@mercury.xraylith.wisc.edu>; from Mumit Khan on Thu, Sep 16, 1999 at 08:12:47PM -0500

I still don't understand.  You seem to be going with the malloc()
mechanism because using the stack is "bad".  No matter how you
slice it, your version is slower and it doesn't have any advantage
over the current one.

In fact, malloc() uses its own synchronization mechanism so even if you
remove the event synchronization you could still end up with as much or
more overhead than the current method.  So, you could eliminate the
event signalling with the use of malloc(), but I'm not 100% certain
that it would buy you anything.

As I mentioned, the reason for the synchronization is to *ensure* that
the stack in the initiating thread is in a known state.  It's the whole
reason for the synchronization.

I'm not sure why you're mentioning sig_send in this context.  Maybe
you meant wait_sig.  sig_send will wait for wait_sig if it wait_sig
has not finished initializing.

-chris

On Thu, Sep 16, 1999 at 08:12:47PM -0500, Mumit Khan wrote:
>Chris Faylor <cgf AT cygnus DOT com> writes:
>> 
>> Ah.  I see where you were coming from, then.  The synchronization is
>> there specifically to work around this particular problem.
>> 
>> I don't see any reason to avoid using the stack since the
>> synchronization is there to ensure that it's ok for the first thread to
>> return (guess how long it took me to realize that I needed this).  I'd
>> assumed that it is considerably less expensive to copy a few elements
>> from the stack than it is to malloc something.  Maybe this is moot if we
>> can actually eliminate the CreateEvent/SetEvent/WaitForSingleObject but
>> your patch doesn't do that.  It does potentially use up a block of
>> memory in the heap which could be in use for a long period of time.
>> 
>> Maybe what is really needed here are some comments...
>
>My oversight. Trivially fixed by going back to structure copy and 
>free'ing the passed in pointer right away. Patch below.
>
>Frankly, I don't quite understand why the synchronization (which is
>always more expensive not doing it ;-) is needed. The part that I
>have not looked at in detail is what may happen in sig_send if the
>signal thread hasn't gotten there quite yet. With the change below,
>either way will be safe at least from the parameter lifetime issue.
>
>Thu Sep 16 20:04:16 1999  Mumit Khan  <khan AT xraylith DOT wisc DOT edu>
>
>	* debug.cc (stdlib.h): Unconditionally include. 
>	(thread_stub): Deallocate passed parameter.
>	(makethread): Don't pass stack data to thread start routine.
>
>--- debug.cc.~1	Thu Sep 16 12:14:44 1999
>+++ debug.cc	Thu Sep 16 20:03:33 1999
>@@ -9,6 +9,7 @@ details. */
> #define NO_DEBUG_DEFINES
> #include "winsup.h"
> #include "exceptions.h"
>+#include <stdlib.h>
> 
> #undef lock
> #undef unlock
>@@ -86,6 +87,8 @@ thread_stub (VOID *arg)
>   exception_list except_entry;
>   thread_start info = *((thread_start *) arg);
> 
>+  free (arg);
>+
>   /* marco AT ddi DOT nl: Needed for the reent's  of this local dll thread
>      I assume that the local threads are using the reent structure of
>      the main thread
>@@ -109,11 +112,18 @@ makethread (DWORD (*start) (void *), LPV
>   DWORD tid;
>   HANDLE h;
>   SECURITY_ATTRIBUTES *sa;
>-  thread_start info;
>+  // This is deallocated by thread_stub.
>+  thread_start *info = (thread_start *) malloc (sizeof (thread_start));
>+
>+  if (! info)
>+    {
>+      debug_printf ("malloc failed, %E");
>+      return INVALID_HANDLE_VALUE;
>+    }
> 
>-  info.func = start;
>-  info.arg = param;
>-  info.sync = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL);
>+  info->func = start;
>+  info->arg = param;
>+  info->sync = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL);
> 
>   if (*name != '+')
>     sa = &sec_none_nih;
>@@ -123,12 +133,12 @@ makethread (DWORD (*start) (void *), LPV
>       sa = &sec_none;
>     }
> 
>-  if ((h = CreateThread (sa, 0, thread_stub, (VOID *)&info, flags, &tid)))
>+  if ((h = CreateThread (sa, 0, thread_stub, (VOID *)info, flags, &tid)))
>     {
>       regthread (name, tid);
>-      WaitForSingleObject (info.sync, INFINITE);
>+      WaitForSingleObject (info->sync, INFINITE);
>     }
>-  CloseHandle (info.sync);
>+  CloseHandle (info->sync);
> 
>   return h;
> }
>@@ -164,7 +174,6 @@ threadname (DWORD tid, int lockit)
> }
> 
> #ifdef DEBUGGING
>-#include <stdlib.h>
> 
> typedef struct _h
>   {
>
>Regards,
>Mumit
>

-- 
cgf AT cygnus DOT com
http://www.cygnus.com/