This is the mail archive of the cygwin-developers@sourceware.cygnus.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]

Re: [take 2] Re: (patch) makethread stdcall/cdecl confusion


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@cygnus.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@xraylith.wisc.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@ddi.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@cygnus.com
http://www.cygnus.com/

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