Re: [PATCH 1/4] dlopen: switch to new pathfinder class
On 08/31/2016 09:12 PM, Corinna Vinschen wrote: > Hi Michael, > > On Aug 31 20:07, Michael Haubenwallner wrote: >> Instead of find_exec, without changing behaviour use new pathfinder >> class with new allocator_interface around tmp_pathbuf and new vstrlist >> class. >> * pathfinder.h (pathfinder): New file. >> * vstrlist.h (allocator_interface, allocated_type, vstrlist): New file. >> * dlfcn.cc (dlopen): Avoid redundant GetModuleHandleExW with RTLD_NOLOAD >> and RTLD_NODELETE. Switch to new pathfinder class, using >> (tmp_pathbuf_allocator): New class. >> (get_full_path_of_dll): Drop. >> [...] > > Just one nit here: > >> +/* Dumb allocator using memory from tmp_pathbuf.w_get (). >> + >> + Does not reuse free'd memory areas. Instead, memory >> + is released when the tmp_pathbuf goes out of scope. >> + >> + ATTENTION: Requesting memory from an instance of tmp_pathbuf breaks >> + when another instance on a newer stack frame has provided memory. */ >> +class tmp_pathbuf_allocator >> + : public allocator_interface > > You didn't reply to > https://cygwin.com/ml/cygwin-developers/2016-08/msg00013.html > So, again, why didn't you simply integrate a tmp_pathbuf member into the > pathfinder class, rather than having to create some additional allocator > class? I'm probably not the most diligent C++ hacker, but to me this > additional allocator is a bit confusing. Sorry, seems I've failed to fully grasp your concerns firsthand in https://cygwin.com/ml/cygwin-developers/2016-08/msg00016.html Second try to answer: https://cygwin.com/ml/cygwin-developers/2016-09/msg0.html > The rest of the patch looks good. I'll look further into the patchset > later tomorrow. Thanks! /haubi/
Re: [PATCH 3/4] dlopen: on x/lib search x/bin if exe is in x/bin
Hi Michael,
On Aug 31 20:07, Michael Haubenwallner wrote:
> citing https://cygwin.com/ml/cygwin-developers/2016-08/msg00020.html
> > Consider the file /usr/bin/cygz.dll:
> > - dlopen (libz.so)success
> > - dlopen (/usr/bin/libz.so) success
> > - dlopen (/usr/lib/libz.so) fails
>
> * dlfcn.c (dlopen): For dlopen("x/lib/N"), when the application
> executable is in "x/bin/", search for "x/bin/N" before "x/lib/N".
> ---
> winsup/cygwin/dlfcn.cc | 36 +++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/winsup/cygwin/dlfcn.cc b/winsup/cygwin/dlfcn.cc
> index e592512..f8b8743 100644
> --- a/winsup/cygwin/dlfcn.cc
> +++ b/winsup/cygwin/dlfcn.cc
> @@ -153,6 +153,25 @@ collect_basenames (pathfinder::basenamelist & basenames,
>basenames.appendv (basename, baselen, ext, extlen, NULL);
> }
>
> +/* Identify dir of current executable into exedirbuf using wpathbuf buffer.
> + Return length of exedirbuf on success, or zero on error. */
> +static int
> +get_exedir (char * exedirbuf, wchar_t * wpathbuf)
> +{
> + /* Unless we have a special cygwin loader, there is no such thing like
> + DT_RUNPATH on Windows we can use to search for dlls, except for the
> + directory of the main executable. */
> + GetModuleFileNameW (NULL, wpathbuf, NT_MAX_PATH);
> + wchar_t * lastwsep = wcsrchr (wpathbuf, L'\\');
> + if (!lastwsep)
> +return 0;
> + *lastwsep = L'\0';
> + *exedirbuf = '\0';
> + if (cygwin_conv_path (CCP_WIN_W_TO_POSIX, wpathbuf, exedirbuf,
> NT_MAX_PATH))
> +return 0;
> + return strlen (exedirbuf);
> +}
You could just use the global variable program_invocation_name. If in
doubt, use the Windows path global_progname and convert it to full POSIX
via cygwin_conv_path.
> extern "C" void *
> dlopen (const char *name, int flags)
> {
> @@ -184,13 +203,28 @@ dlopen (const char *name, int flags)
>/* handle for the named library */
>path_conv real_filename;
>wchar_t *wpath = tp.w_get ();
> + char *cpath = tp.c_get ();
>
>pathfinder finder (allocator, basenames); /* eats basenames */
>
>if (have_dir)
> {
> + int dirlen = basename - 1 - name;
> +
> + /* if the specified dir is x/lib, and the current executable
> + dir is x/bin, do the /lib -> /bin mapping, which is the
> + same actually as adding the executable dir */
> + if (dirlen >= 4 && !strncmp (name + dirlen - 4, "/lib", 4))
> + {
> + int exedirlen = get_exedir (cpath, wpath);
> + if (exedirlen == dirlen &&
> + !strncmp (cpath, name, dirlen - 4) &&
> + !strcmp (cpath + dirlen - 4, "/bin"))
> + finder.add_searchdir (cpath, exedirlen);
> + }
> +
> /* search the specified dir */
> - finder.add_searchdir (name, basename - 1 - name);
> + finder.add_searchdir (name, dirlen);
> }
>else
> {
> --
> 2.7.3
Rest looks ok.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
Re: [PATCH 1/4] dlopen: switch to new pathfinder class
On Sep 1 13:05, Michael Haubenwallner wrote: > On 08/31/2016 09:12 PM, Corinna Vinschen wrote: > > Hi Michael, > > > > On Aug 31 20:07, Michael Haubenwallner wrote: > >> Instead of find_exec, without changing behaviour use new pathfinder > >> class with new allocator_interface around tmp_pathbuf and new vstrlist > >> class. > >> * pathfinder.h (pathfinder): New file. > >> * vstrlist.h (allocator_interface, allocated_type, vstrlist): New file. > >> * dlfcn.cc (dlopen): Avoid redundant GetModuleHandleExW with RTLD_NOLOAD > >> and RTLD_NODELETE. Switch to new pathfinder class, using > >> (tmp_pathbuf_allocator): New class. > >> (get_full_path_of_dll): Drop. > >> [...] > > > > Just one nit here: > > > >> +/* Dumb allocator using memory from tmp_pathbuf.w_get (). > >> + > >> + Does not reuse free'd memory areas. Instead, memory > >> + is released when the tmp_pathbuf goes out of scope. > >> + > >> + ATTENTION: Requesting memory from an instance of tmp_pathbuf breaks > >> + when another instance on a newer stack frame has provided memory. */ > >> +class tmp_pathbuf_allocator > >> + : public allocator_interface > > > > You didn't reply to > > https://cygwin.com/ml/cygwin-developers/2016-08/msg00013.html > > So, again, why didn't you simply integrate a tmp_pathbuf member into the > > pathfinder class, rather than having to create some additional allocator > > class? I'm probably not the most diligent C++ hacker, but to me this > > additional allocator is a bit confusing. > > Sorry, seems I've failed to fully grasp your concerns firsthand in > https://cygwin.com/ml/cygwin-developers/2016-08/msg00016.html > Second try to answer: > https://cygwin.com/ml/cygwin-developers/2016-09/msg0.html Ok, I see what you mean, but it doesn't make me really happy. I'm willing to take it for now but I'd rather see basenames being a member of pathfinder right from the start, so you just instantiate finder and call methods on it. Given that basenames is a member, you can do the allocator stuff completely inside the pathfinder class. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat signature.asc Description: PGP signature
Re: [PATCH 2/4] dlopen (pathfinder): try each basename per dir
On Aug 31 20:07, Michael Haubenwallner wrote:
> Rather than searching all search dirs per one basename,
> search for all basenames within per one search dir.
>
> pathfinder.h (check_path_access): Interchange dir- and basename-loops.
> ---
> winsup/cygwin/pathfinder.h | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/winsup/cygwin/pathfinder.h b/winsup/cygwin/pathfinder.h
> index bbba168..c306604 100644
> --- a/winsup/cygwin/pathfinder.h
> +++ b/winsup/cygwin/pathfinder.h
> @@ -182,12 +182,12 @@ public:
>basenamelist::member const ** found_basename = NULL)
>{
> char const * critname = criterion.name ();
> -for (basenamelist::iterator name = basenames_.begin ();
> - name != basenames_.end ();
> - ++name)
> - for (searchdirlist::iterator dir(searchdirs_.begin ());
> -dir != searchdirs_.end ();
> -++dir)
> +for (searchdirlist::iterator dir(searchdirs_.begin ());
> + dir != searchdirs_.end ();
> + ++dir)
> + for (basenamelist::iterator name = basenames_.begin ();
> +name != basenames_.end ();
> +++name)
> if (criterion.test (dir, name))
> {
> debug_printf ("(%s), take %s%s", critname,
> --
> 2.7.3
This one's fine, of course.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
Re: [PATCH 4/4] dlopen: on unspecified lib dir search exe dir
On Aug 31 20:07, Michael Haubenwallner wrote:
> Applications installed to some prefix like /opt/application do expect
> dlopen("libAPP.so") to load "/opt/application/bin/cygAPP.dll", which
> is similar to "/opt/application/lib/libAPP.so" on Linux.
>
> See also https://cygwin.com/ml/cygwin-developers/2016-08/msg00020.html
>
> * dlfcn.cc (dlopen): For dlopen("N"), search directory where the
> application executable is in.
> ---
> winsup/cygwin/dlfcn.cc | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/winsup/cygwin/dlfcn.cc b/winsup/cygwin/dlfcn.cc
> index f8b8743..974092e 100644
> --- a/winsup/cygwin/dlfcn.cc
> +++ b/winsup/cygwin/dlfcn.cc
> @@ -232,6 +232,12 @@ dlopen (const char *name, int flags)
>not use the LD_LIBRARY_PATH environment variable. */
> finder.add_envsearchpath ("LD_LIBRARY_PATH");
>
> + /* Search the current executable's directory like
> + the Windows loader does for linked dlls. */
> + int exedirlen = get_exedir (cpath, wpath);
> + if (exedirlen)
> + finder.add_searchdir (cpath, exedirlen);
> +
> /* Finally we better have some fallback. */
> finder.add_searchdir ("/usr/bin", 8);
> finder.add_searchdir ("/usr/lib", 8);
> --
> 2.7.3
Still not quite sure if that's the right thing to do...
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
