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:02:17 -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: (patch) makethread stdcall/cdecl confusion
Message-ID: <19990916220217.A5792@cygnus.com>
Mail-Followup-To: Mumit Khan <khan AT xraylith DOT wisc DOT EDU>,
	cygwin-developers AT sourceware DOT cygnus DOT com
References: <19990916125519 DOT D655 AT cygnus DOT com> <199909161729 DOT MAA07236 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: <199909161729.MAA07236@mercury.xraylith.wisc.edu>; from Mumit Khan on Thu, Sep 16, 1999 at 12:29:42PM -0500

On Thu, Sep 16, 1999 at 12:29:42PM -0500, Mumit Khan wrote:
>Chris Faylor <cgf AT cygnus DOT com> writes:
>> On Thu, Sep 16, 1999 at 10:29:50AM -0500, Mumit Khan wrote:
>> >Another issue when you're dealing with thread start routines -- it's 
>> >almost always better to malloc the the parameter argument instead of
>> >passing the address of a stack data element. It'll work in the current
>> >usages in winsup, but this usage can lead to very subtle and hard to
>> >track errors. 
>> 
>> That's probably because, AFAICT, in every case where the argument is
>> non-NULL it *is* malloced.
>
>My mistake -- I generalized based on one particular instance, sorry. I
>was referring to the parameter passed to thread_stub, which caught my
>eye. It is harmless in this particular case since the calling thread 
>waits on the callee (synchronization acts as insurance against possible 
>stack mismatch).

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...

Nah.

cgf

>How about this:
>
>Thu Sep 16 12:31:46 1999  Mumit Khan  <khan AT xraylith DOT wisc DOT edu>
>
>	* debug.cc (stdlib.h): Unconditionally include. 
>	(thread_stub): Don't use structure copy. Deallocate memory when
>	done.
>	(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 12:29:51 1999
>@@ -9,6 +9,7 @@ details. */
> #define NO_DEBUG_DEFINES
> #include "winsup.h"
> #include "exceptions.h"
>+#include <stdlib.h>
> 
> #undef lock
> #undef unlock
>@@ -84,7 +85,7 @@ static DWORD WINAPI
> thread_stub (VOID *arg)
> {
>   exception_list except_entry;
>-  thread_start info = *((thread_start *) arg);
>+  thread_start *info = (thread_start *) 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
>@@ -95,11 +96,13 @@ thread_stub (VOID *arg)
> 	api_fatal(" Sig proc MT init failed\n");
> #endif
> 
>-  SetEvent (info.sync);
>+  SetEvent (info->sync);
> 
>   init_exceptions (&except_entry);
> 
>-  return (*info.func) (info.arg);
>+  DWORD retcode = (*info->func) (info->arg);
>+  free (info);
>+  return retcode;
> }
> 
> HANDLE
>@@ -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));
> 
>-  info.func = start;
>-  info.arg = param;
>-  info.sync = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL);
>+  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);
> 
>   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/