Re: Relocation patch for cygwin

[Charles Wilson]

On 10/3/2011 7:26 AM, Bruno Haible wrote:
> I'm applying this change to gnulib, from where it will propagate to libiconv
> and libintl.

I really don't think this is the right approach. See below.

> Only it's a pity that you gave me the advice to use Cygwin's /proc/self/maps,
> which I did [2], and now we discover that it is much slower than the Win32
> API calls that I had originally.

True -- but IF someone WERE to build some package that incorporated
gnulib relocation support, with --enable-relocation turned on, then it
is appropriate on cygwin to use cygwin features, even if it IS slower.

The problem is, it can really cause issues for "the rest of cygwin" if
an app uses the Win32 API "behind cygwin's back".  Given cygwin's
existing code and algorithm structure, the Win32 calls that
old-relocation was using were not (and are not) problematic AFAICT (*)
-- but that could change in the future, in unpredictable ways.  It would
be bad if cygwin found itself constrained in its ongoing development,
because some important libraries were using Win32 calls in ways that
prevented cygwin from making some optimization or other.


(*) Well, except that -- IIRC -- cygwin processes no longer guarantee
that the win32 "idea" of what is the CWD, matches the posix "idea" --
when the full path is longer than 260 characters or contains unicode
characters that cannot be represented in the current codepage.  I can
see where THAT little issue might cause problems even with
old-relocation's win32 calls...  Corinna can correct me on this if I am
misremembering the details.


> [1] http://cygwin.com/ml/cygwin/2011-01/msg00410.html
> [2] 
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=b8d53f77acf1ed08ba808014e534e07c9dd40daf
> 
> 
> 2011-10-03  Bruno Haible  <address@hidden>
> 
>       relocatable-lib[-lgpl]: Avoid expensive /proc access on Linux, Cygwin.
>       * lib/relocatable.c (ENABLE_COSTLY_RELOCATABLE): New macro.
>       (compute_curr_prefix, shared_library_fullname,
>       find_shared_library_fullname, get_shared_library_fullname, relocate):
>       Use it together with PIC && INSTALLDIR.
>       Reported by <address@hidden>
>       via Charles Wilson <address@hidden>.

Basically, this patch means that even if I **DO** configure with
--enable-relocation, I won't actually get it.  That's not the right fix.
 If I do --enable-relocatable, then by george I ought to get it, slow
5ms operation and all.

The problem jojolino discovered was that ENABLE_RELOCATABLE was
*hardcoded* to 1 inside libintl's Makefile.in, whether you specify
-enable-relocation OR --disable-relocation (or nothing).  This meant
that the slow operations always occurred -- and always did precisely
nothing when --disable-relocatable, because the compiled-in prefix was
/usr, and the installation directory was under /usr, so it was a no-op.
A very slow no-op.

I understand that you want to ensure that the API of (e.g.) libintl
doesn't change, regardless of --enable|--disable-relocatable; so some
function named "libintl_relocate()" (or relocate(), or whatever the heck
it eventually gets #defined to be) must exist in the final libintl .dll.
 It should just be a *fast* no-op in the --disable-relocation case.

I think the right fix is that, for __CYGWIN__, UNLESS
--enable-relocation, the "guts" of the relocate() function should just
return the input argument (e.g. a fast no-op).  However, if
--enable-relocation, then relocate() should operate just as it did last
week [slow /proc/maps/exe and everything].

Maybe this could be accomplished by, along with your patch, adding a
simple extra modification to relocatable-lib.m4:

diff --git a/m4/relocatable-lib.m4 b/m4/relocatable-lib.m4
index aa798b9..b413b3c 100644
--- a/m4/relocatable-lib.m4
+++ b/m4/relocatable-lib.m4
@@ -26,6 +26,10 @@ AC_DEFUN([gl_RELOCATABLE_LIBRARY_BODY],
     AC_DEFINE([ENABLE_RELOCATABLE], [1],
       [Define to 1 if the package shall run at any location in the file
        system.])
+    AC_DEFINE([ENABLE_COSTLY_RELOCATABLE], [1],
+      [Special support for expensive relocation in libintl on certain
+       platforms, since libintl Makefile.in always defines
ENABLE_RELOCATABLE
+       for ABI stability reasons.])
   fi
 ])


It seems that libintl, at least, needs two relocation-related "knobs" --
one that specifies whether *relocate() is declared/defined, and another
that specifies whether it actually DOES anything.  On cygwin (at least
for libintl), the first knob is explicitly and permanently set to "yes"
in Makefile.in, regardless of the options passed to ./configure -- and
we need the /other/ knob's value to be affected by
--enable|--disable-relocatable.

--
Chuck