[PATCH] Preserve order of dlopen'd modules in dll_list::topsort

[Corinna Vinschen]

Hi David,

thanks for the new patch.

On Feb 27 17:13, David Allsopp wrote:
> Corinna Vinschen wrote:
> > On Feb 25 16:27, David Allsopp wrote:
> > > This patch (below - I hope I have managed to format this email
> > > correctly) alters the behaviour of dll_list::topsort to preserve the
> > > order of dlopen'd units.
> > > [...]
> > > This patch is licensed under 2-clause BSD as per winsup/CONTRIBUTORS,
> > > Copyright (c) 2017, MetaStack Solutions Ltd.

Do you really want to make it (c) MetaStack?

I'm asking because this way I can't add you personally as contributor to
the CONTRIBUTORS file and you will have to continue to add per-patch
copyright.  The idea of the CONTRIBUTORS file was to claim BSD 2-clause
for your first and all subsequent patches you provide to Cygwin, so you
never have to think about the copyright stuff again.  Your choice.

> > - While you're at it, please reformat your patch so the line length
> >   is not longer than 80 chars.
> 
> Done - sorry, I'd inferred a longer length from a few other longer lines!

Yeah, the surrounding codes has a few minor formatting issues, in fact.

> > - Last but not least.  You add code to topsort so the loaded DLLs
> >   are handled first.  The subsequent code is untouched.  However,
> >   shouldn't the next loop then restrict calling populate_deps to the
> >   linked DLLs only, at least for performance?
> 
> Oops :$ That's an artefact of the "story" of the patch's development.
> As it happens, the first dlopen'd DLL would have been initialised in
> the second loop, not the first, but the presence of two loops like
> that was indeed mostly inefficient. I've kept the original one as a
> "fast path" for the case of no dlopen'd DLLs, though I don't know if
> that's a worthwhile optimisation. 

Well, interesting point.  Basically your new code is a drop-in
replacement, except for the fact that it always calls an extra
cmalloc/cfree.  However, this is only required if loaded_dlls > 0 so I
think we may get away with removing the old loop with a simple tweak to
your new one:

  dll** dlopen_deps = NULL;
  if (loaded_dlls > 0)
    dlopen_deps = (dll**) cmalloc (HEAP_2_DLL, maxdeps*sizeof (dll*));
  while ((d = d->next))
    {
      [...]
    }
  if (dlopen_deps)
    cfree (dlopen_deps);

Do you want to tweak your patch accordingly?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20170228/8ce52621/attachment.sig>