Re: About the dll search algorithm of dlopen (patch-r2)

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Michael,

On Aug 22 20:37, Corinna Vinschen wrote:
> On Aug 22 18:02, Michael Haubenwallner wrote:
> > diff --git a/winsup/cygwin/vstrlist.h b/winsup/cygwin/vstrlist.h
> > new file mode 100644
> > index 0000000..ecbdc64
> > --- /dev/null
> > +++ b/winsup/cygwin/vstrlist.h
> > [...]
> > +    void * operator new (size_t class_size, char const * part0, va_list parts)
> > +    {
> > +      char const * part = part0;
> > +      va_list partsdup;
> > +      va_copy (partsdup, parts);
> > +      size_t bufferlength = 0;
> > +      while (part)
> > +	{
> > +	  int partlen = va_arg (partsdup, int);
> > +	  if (partlen < 0)
> > +	    partlen = strlen (part);
> > +	  bufferlength += partlen;
> > +	  part = va_arg (partsdup, char const *);
> > +	}
> > +      va_end (partsdup);
> > +
> > +      return cmalloc_abort (HEAP_STR, class_size + bufferlength);
> > +    }
> 
> Uhm... I don't think this is correct.  Using cmalloc and friends has a
> specific purpose.  Only internal datastructures which are inherited by
> child processes via fork/exec are supposed to be allocated on the
> cygheap.  The cygheap really shouldn't be used freely for other
> purposes.  You could leak it to child processes if another thread forks
> during your thread's call to dlopen.  Also, especially on 32 bit the
> pressure on the cygheap is not insignificant.  Please use malloc/free
> instead.

After a night's sleep something else occured to me.  The old, existing
code didn't have to use {c}malloc/{c}free at all (ignoring called
functions).  By using the pathfinder method in its current state
you now allocate/free multiple blocks, one for each basename and one for
each search dir.  And at the end of the function you free the space
again.

Malloc/free is a costly operation which also requires locking and using
pathfinder as is adds that multiple times for each call of dlopen.
Every time we can get rid of malloc/free is a win.  So, here's a
question: What if you only use a single tmp_pathbuf for the basenames
and a single tmp_pathbuf of 64K for the paths.  That should be
sufficient for this functionality and the necessary buffers are already
allocated.  Alternatively you could use alloca for the basenames, they
are light on space anyway.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature