[PATCH] Do not decorate symbols as dllexport on Cygwin

[PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

Note that dllimport/dllexport decorations are not at all required on
Cygwin for quite some time.

Worse, this breaks building DLLs and DLL import libs using libtool.

On Cygwin --export-all-symbols is default.  However, if just a single
symbol is decorated with dllexport, ld switches to exporting only these
symbols.

So, when creating DLLs and DLL import libs which are also linked against
libgnu.a, the result is that the actual DLL (i. e. cygfoo-1.dll)
contains all symbols of all object files given on the command line.
However, the DLL import lib (i. e. libfoo.dll.a) will contain only the
default symbols and the single symbol gl_get_setlocale_null_lock.

This in turn breaks linking against the just created cygfoo-1.dll.

An example of that is current recode.  Building this package with
--enable-shared is broken, unless one adds -Wl,--export-all-symbols to
the link stage command line explicitely, because of the decorated
gl_get_setlocale_null_lock variable.

Fix this by dropping the dllexport decoration when building for the
Cygwin target.

* lib/setlocale-lock.c: don't decorate gl_get_setlocale_null_lock
as dllexport on Cygwin.
* lib/setlocale_null.c: don't dllimport gl_get_setlocale_null_lock
on Cygwin.
---
 lib/setlocale-lock.c | 2 +-
 lib/setlocale_null.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/setlocale-lock.c b/lib/setlocale-lock.c
index b70ba09b0020..ab23cadd2939 100644
--- a/lib/setlocale-lock.c
+++ b/lib/setlocale-lock.c
@@ -41,7 +41,7 @@ typedef int dummy;
 #  if HAVE_VISIBILITY
   /* Override the effect of the compiler option '-fvisibility=hidden'.  */
 #   define DLL_EXPORTED __attribute__((__visibility__("default")))
-#  elif defined _WIN32 || defined __CYGWIN__
+#  elif defined _WIN32 && !defined __CYGWIN__
 #   define DLL_EXPORTED __declspec(dllexport)
 #  else
 #   define DLL_EXPORTED
diff --git a/lib/setlocale_null.c b/lib/setlocale_null.c
index 6ac563db14a9..5bf04a5ad84d 100644
--- a/lib/setlocale_null.c
+++ b/lib/setlocale_null.c
@@ -201,7 +201,7 @@ setlocale_null_with_lock (int category, char *buf, size_t bufsize)
 # elif HAVE_PTHREAD_API /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin */
 
 extern
-#  if defined _WIN32 || defined __CYGWIN__
+#  if defined _WIN32 && !defined __CYGWIN__
   __declspec(dllimport)
 #  endif
   pthread_mutex_t *gl_get_setlocale_null_lock (void);
-- 
2.39.1

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Bruno Haible]

Hi Corinna,

Thanks for the heads-up and patch.

> Note that dllimport/dllexport decorations are not at all required on
> Cygwin for quite some time.

Is this true in all circumstances? I thought that when --disable-auto-import
is in use AND the shared library wants to export variables, dllimport
and dllexport decorations are necessary.

> On Cygwin --export-all-symbols is default.  However, if just a single
> symbol is decorated with dllexport, ld switches to exporting only these
> symbols.
> ...
> An example of that is current recode.  Building this package with
> --enable-shared is broken, unless one adds -Wl,--export-all-symbols to
> the link stage command line explicitely, because of the decorated
> gl_get_setlocale_null_lock variable.

So, there are three types of packages:
  (A) Those that use --disable-auto-import (e.g. [1][2]) and provide an
      explicit list of exports for their shared libraries.
  (B) Those that don't use --disable-auto-import but want to limit
      the exposed symbols for other reasons (e.g. namespace cleanliness).
      Such as those that use the libtool options '-export-symbols' or
      '-export-symbols-regex'.
  (C) Those like 'recode', which want all symbols to be exported (e.g.
      if namespace cleanliness is not an issue for them).

For packages of type (A) or (B), the symbol gl_get_setlocale_null_lock
*MUST* be exported, otherwise there may be different get_setlocale_null_lock
functions accessed by different code, which will destroy the purpose of this
function, i.e. open the door to potential crashes due to use of setlocale().

For packages of type (C), the symbol gl_get_setlocale_null_lock *MUST NOT*
be exported, otherwise all other symbols will not be exported.

AFAIU, your patch fixes packages of type (C), while at the same time breaking
packages of type (A) or (B).

Is there an easy way to distinguish packages of type (C) from those of type
(A), (B)?

Another option — since we are talking about a single symbol and a single
platform — would be if the locking for setlocale_null were not necessary
on Cygwin in the first place. I determined that it is necessary by running
the unit test gnulib/tests/test-setlocale_null-mt-all.c [3] on Cygwin:
without the lock, it crashed within less than 1 second. Could the
implementation of setlocale() in Cygwin be changed in such a way that this
test does not crash? Then the lock would be necessary.

Bruno

[1] https://git.savannah.gnu.org/gitweb/?p=gettext.git;a=blob;f=gettext-tools/woe32dll/export.h;hb=HEAD
[2] https://haible.de/bruno/woe32dll.html
[3] https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=tests/test-setlocale_null-mt-all.c;hb=HEAD

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Bruno Haible]

I wrote:
> Then the lock would be necessary.

but meant: Then the lock would not be necessary.

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

On Feb  5 21:41, Bruno Haible wrote:
> Hi Corinna,
> 
> Thanks for the heads-up and patch.
> 
> > Note that dllimport/dllexport decorations are not at all required on
> > Cygwin for quite some time.
> 
> Is this true in all circumstances? I thought that when --disable-auto-import
> is in use AND the shared library wants to export variables, dllimport
> and dllexport decorations are necessary.
> 
> > On Cygwin --export-all-symbols is default.  However, if just a single
> > symbol is decorated with dllexport, ld switches to exporting only these
> > symbols.
> > ...
> > An example of that is current recode.  Building this package with
> > --enable-shared is broken, unless one adds -Wl,--export-all-symbols to
> > the link stage command line explicitely, because of the decorated
> > gl_get_setlocale_null_lock variable.
> 
> So, there are three types of packages:
>   (A) Those that use --disable-auto-import (e.g. [1][2]) and provide an
>       explicit list of exports for their shared libraries.
>   (B) Those that don't use --disable-auto-import but want to limit
>       the exposed symbols for other reasons (e.g. namespace cleanliness).
>       Such as those that use the libtool options '-export-symbols' or
>       '-export-symbols-regex'.
>   (C) Those like 'recode', which want all symbols to be exported (e.g.
>       if namespace cleanliness is not an issue for them).
> 
> For packages of type (A) or (B), the symbol gl_get_setlocale_null_lock
> *MUST* be exported, otherwise there may be different get_setlocale_null_lock
> functions accessed by different code, which will destroy the purpose of this
> function, i.e. open the door to potential crashes due to use of setlocale().
> 
> For packages of type (C), the symbol gl_get_setlocale_null_lock *MUST NOT*
> be exported, otherwise all other symbols will not be exported.
> 
> AFAIU, your patch fixes packages of type (C), while at the same time breaking
> packages of type (A) or (B).
> 
> Is there an easy way to distinguish packages of type (C) from those of type
> (A), (B)?
> 
> Another option — since we are talking about a single symbol and a single
> platform — would be if the locking for setlocale_null were not necessary
> on Cygwin in the first place. I determined that it is necessary by running
> the unit test gnulib/tests/test-setlocale_null-mt-all.c [3] on Cygwin:
> without the lock, it crashed within less than 1 second. Could the
> implementation of setlocale() in Cygwin be changed in such a way that this
> test does not crash? Then the lock would be necessary.

Well, we could do that by adding Cygwin-internal locking to setlocale
calls.  But that would only be available in the next Cygwin version
of course.

May I ask what's the idea to provide a thread-safe setlocale?  It was
never defined as thread-safe and POSIX explicitely mentions that.  Any
application expecting to call setlocale thread-safe is broken by design.
It should use the newlocale/duplocale/uselocale/freelocale API instead,
isn't it?


Corinna



> 
> Bruno
> 
> [1] https://git.savannah.gnu.org/gitweb/?p=gettext.git;a=blob;f=gettext-tools/woe32dll/export.h;hb=HEAD
> [2] https://haible.de/bruno/woe32dll.html
> [3] https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=tests/test-setlocale_null-mt-all.c;hb=HEAD
> 
> 

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

Hi Bruno,

On Feb  5 22:22, Corinna Vinschen wrote:
> On Feb  5 21:41, Bruno Haible wrote:
> > Another option — since we are talking about a single symbol and a single
> > platform — would be if the locking for setlocale_null were not necessary
> > on Cygwin in the first place. I determined that it is necessary by running
> > the unit test gnulib/tests/test-setlocale_null-mt-all.c [3] on Cygwin:
> > without the lock, it crashed within less than 1 second. Could the
> > implementation of setlocale() in Cygwin be changed in such a way that this
> > test does not crash? Then the lock would be necessary.
> 
> Well, we could do that by adding Cygwin-internal locking to setlocale
> calls.  But that would only be available in the next Cygwin version
> of course.
> 
> May I ask what's the idea to provide a thread-safe setlocale?  It was
> never defined as thread-safe and POSIX explicitely mentions that.  Any
> application expecting to call setlocale thread-safe is broken by design.
> It should use the newlocale/duplocale/uselocale/freelocale API instead,
> isn't it?

Ahhh, I finally see what's going on.  The problem is not thread-safety
as such, but thread-safety when reading the value of the LC_ALL category.

Glibc's setlocale isn't entirely thread-safe either, but there's a
difference:

- GLibc creates the global strings returned by setlocale(LC_xxx, NULL)
  at the time the locale data is changed.  All setlocale(LC_xxx, NULL)
  calls only return pointer to strings created earlier.

- Cygwin or, better, newlib, also return a pointer to global strings.
  However, while the global strings for the specific categories are
  created when the locale is changed, the string returned for LC_ALL
  gets created on the fly when setlocale(LC_ALL, NULL) is called.
  That's why test-setlocale_null-mt-all fails almost immediately.

I created a patch to newlib's setlocale to tweak the LC_ALL string
each time the locale is changed, while setlocale(LC_ALL, NULL)
just returns the already prepared string.

https://cygwin.com/git/?p=newlib-cygwin.git;a=commitdiff;h=23e49b18ce39

This patch will be in the next Cygwin release 3.4.6.

I'm just a bit fuzzy what patches will be required for gnulib now...


Thanks,
Corinna

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Bruno Haible]

Hi Corinna,

Sorry, I wanted to reply sooner.

> > May I ask what's the idea to provide a thread-safe setlocale?  It was
> > never defined as thread-safe and POSIX explicitely mentions that.  Any
> > application expecting to call setlocale thread-safe is broken by design.

The 'recode' package includes a shared library 'librecode', and shared
libraries are supposed to be multithread-safe.

Multithread-safe code is supposed to be able to obey the current locale,
that is,
  - if uselocale(NULL) != LC_GLOBAL_LOCALE, uselocale()
  - if uselocale(NULL) == LC_GLOBAL_LOCALE, the global locale, that was
    last set through setlocale().

For example:
  - sprintf() needs to know the LC_CTYPE and LC_NUMERIC categories of the
    current locale, in order to transform wide strings via %ls or to
    format numbers via %f or %g.
  - iconv() needs to know the LC_CTYPE category of the current locale,
    for determining the appropriate transliterations.
  - strftime() needs to know the LC_TIME category of the current locale.
  - gettext() needs to know the LC_MESSAGES category of the current locale.
All these functions are supposed to be writable in application code
(if, for whatever reason, the sprintf, iconv, strftime, gettext functions
in libc are not considered adequate).

When POSIX specifies that applications cannot call setlocale() when there
are multiple threads, this implies that in order to *inspect* the locale
they need to do so before spawning the threads, and cache the value in a
global variable, for later use. But this is not the application architecture
that is in use when there are shared libraries. Shared libraries commonly
don't have an initialization hook by which the application informs the
library about the current locale for the rest of the execution of the
process.

So, conventional wisdom is to use setlocale(category, NULL). But this
faces MT-safety problems. On some platforms the problem is that the
setlocale(_, NULL) calls themselves will trash each other's data structures
and thus provoke a crash. On other platforms the problem is that the return
value produced in one thread is being clobbered by the second thread, and
thus the first thread has no chance to copy the return value to a safe
area in due time.

For Cygwin, the problem until yesterday was the second one, for
category == LC_ALL only.

> > It should use the newlocale/duplocale/uselocale/freelocale API instead,
> > isn't it?

Even code that pays attention to the per-thread locale has to be prepared
for the case that uselocale(NULL) returns LC_GLOBAL_LOCALE. In this case,
the caller needs to fetch the values from the global locale. There is no
locale_t object that could be queried in this case.

> Ahhh, I finally see what's going on.  The problem is not thread-safety
> as such, but thread-safety when reading the value of the LC_ALL category.

Yup, exactly.

> Glibc's setlocale isn't entirely thread-safe either, but there's a
> difference:
> 
> - GLibc creates the global strings returned by setlocale(LC_xxx, NULL)
>   at the time the locale data is changed.  All setlocale(LC_xxx, NULL)
>   calls only return pointer to strings created earlier.

Yes. Thus each thread can be sure to be able to inspect the return value.
(Assuming that there is no call to setlocale that *changes* the global
locale. This is forbidden by POSIX, and reasonable applications don't
do this.)

> - Cygwin or, better, newlib, also return a pointer to global strings.
>   However, while the global strings for the specific categories are
>   created when the locale is changed, the string returned for LC_ALL
>   gets created on the fly when setlocale(LC_ALL, NULL) is called.
... and gets overwritten by another thread, since the buffer is not
per-thread.
>   That's why test-setlocale_null-mt-all fails almost immediately.
> 
> I created a patch to newlib's setlocale to tweak the LC_ALL string
> each time the locale is changed, while setlocale(LC_ALL, NULL)
> just returns the already prepared string.
> 
> https://cygwin.com/git/?p=newlib-cygwin.git;a=commitdiff;h=23e49b18ce39
> 
> This patch will be in the next Cygwin release 3.4.6.

Thanks a lot! I'm adjusting the Gnulib code now, accordingly.

Bruno

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Bruno Haible]

Corinna Vinschen wrote:
> This patch will be in the next Cygwin release 3.4.6.

Thanks!

> I'm just a bit fuzzy what patches will be required for gnulib now...

With this patch, the setlocale_null lock should be gone in Cygwin >= 3.4.6.
I can't really test it, but I hope the patch is correct.


2023-02-06  Bruno Haible  <bruno@clisp.org>

	setlocale-null: Don't use a lock in Cygwin >= 3.4.6.
	Road paved by Corinna Vinschen <vinschen@redhat.com>.
	* m4/setlocale_null.m4 (gl_FUNC_SETLOCALE_NULL): Assume that
	setlocale (LC_ALL, NULL) is multithread-safe in Cygwin >= 3.4.6.
	* lib/setlocale_null.c: Update comments.
	* tests/test-setlocale_null-mt-all.c: Likewise.

diff --git a/lib/setlocale_null.c b/lib/setlocale_null.c
index 6ac563db14..89c8a06598 100644
--- a/lib/setlocale_null.c
+++ b/lib/setlocale_null.c
@@ -173,7 +173,7 @@ setlocale_null_unlocked (int category, char *buf, size_t bufsize)
 #endif
 }
 
-#if !(SETLOCALE_NULL_ALL_MTSAFE && SETLOCALE_NULL_ONE_MTSAFE) /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin */
+#if !(SETLOCALE_NULL_ALL_MTSAFE && SETLOCALE_NULL_ONE_MTSAFE) /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin < 3.4.6 */
 
 /* Use a lock, so that no two threads can invoke setlocale_null_unlocked
    at the same time.  */
@@ -198,7 +198,7 @@ setlocale_null_with_lock (int category, char *buf, size_t bufsize)
   return ret;
 }
 
-# elif HAVE_PTHREAD_API /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin */
+# elif HAVE_PTHREAD_API /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin < 3.4.6 */
 
 extern
 #  if defined _WIN32 || defined __CYGWIN__
diff --git a/m4/setlocale_null.m4 b/m4/setlocale_null.m4
index dd6a5ef538..b41df499a8 100644
--- a/m4/setlocale_null.m4
+++ b/m4/setlocale_null.m4
@@ -1,4 +1,4 @@
-# setlocale_null.m4 serial 6
+# setlocale_null.m4 serial 7
 dnl Copyright (C) 2019-2023 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -13,9 +13,23 @@ AC_DEFUN([gl_FUNC_SETLOCALE_NULL],
   AC_CACHE_CHECK([whether setlocale (LC_ALL, NULL) is multithread-safe],
     [gl_cv_func_setlocale_null_all_mtsafe],
     [case "$host_os" in
-       # Guess no on musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin.
-       *-musl* | darwin* | freebsd* | midnightbsd* | netbsd* | openbsd* | aix* | haiku* | cygwin*)
+       # Guess no on musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku.
+       *-musl* | darwin* | freebsd* | midnightbsd* | netbsd* | openbsd* | aix* | haiku*)
          gl_cv_func_setlocale_null_all_mtsafe=no ;;
+       # Guess no on Cygwin < 3.4.6.
+       cygwin*)
+         AC_EGREP_CPP([Lucky user],
+           [
+#if defined __CYGWIN__
+ #include <cygwin/version.h>
+ #if CYGWIN_VERSION_DLL_COMBINED >= CYGWIN_VERSION_DLL_MAKE_COMBINED (3004, 6)
+  Lucky user
+ #endif
+#endif
+          ],
+          [gl_cv_func_setlocale_null_all_mtsafe=yes],
+          [gl_cv_func_setlocale_null_all_mtsafe=no])
+        ;;
        # Guess yes on glibc, HP-UX, IRIX, Solaris, native Windows.
        *-gnu* | gnu* | hpux* | irix* | solaris* | mingw*)
          gl_cv_func_setlocale_null_all_mtsafe=yes ;;
diff --git a/tests/test-setlocale_null-mt-all.c b/tests/test-setlocale_null-mt-all.c
index 6036c260cd..7480406639 100644
--- a/tests/test-setlocale_null-mt-all.c
+++ b/tests/test-setlocale_null-mt-all.c
@@ -166,7 +166,7 @@ Solaris 11.0         OK
 Solaris 11.4         OK
 Solaris OpenIndiana  OK
 Haiku                crash < 1 sec
-Cygwin               crash < 1 sec
+Cygwin < 3.4.6       crash < 1 sec
 mingw                OK
 MSVC                 OK (assuming compiler option /MD !)
 */

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

On Feb  6 18:37, Bruno Haible wrote:
> Corinna Vinschen wrote:
> > This patch will be in the next Cygwin release 3.4.6.
> 
> Thanks!
> 
> > I'm just a bit fuzzy what patches will be required for gnulib now...
> 
> With this patch, the setlocale_null lock should be gone in Cygwin >= 3.4.6.
> I can't really test it, but I hope the patch is correct.

I just ran the setlocale_null-mt-* tests with gnulib from git master
under Cygwin with my newlib patch, and the tests succeeded.  I checked
that configure is doing the right thing and config.h contains the right
settings:

  $ ./configure
  [...]
  checking whether setlocale (LC_ALL, NULL) is multithread-safe... yes
  checking whether setlocale (category, NULL) is multithread-safe... yes
  [...]
  $ grep SETLOCALE_NULL config.h
  #define GNULIB_TEST_SETLOCALE_NULL 1
  #define SETLOCALE_NULL_ALL_MTSAFE 1
  #define SETLOCALE_NULL_ONE_MTSAFE 1
  $ grep SETLOCALE_NULL gltests/config.h
  #define GNULIB_TEST_SETLOCALE_NULL 1
  #define SETLOCALE_NULL_ALL_MTSAFE 1
  #define SETLOCALE_NULL_ONE_MTSAFE 1


Thanks a lot,
Corinna



> 
> 
> 2023-02-06  Bruno Haible  <bruno@clisp.org>
> 
> 	setlocale-null: Don't use a lock in Cygwin >= 3.4.6.
> 	Road paved by Corinna Vinschen <vinschen@redhat.com>.
> 	* m4/setlocale_null.m4 (gl_FUNC_SETLOCALE_NULL): Assume that
> 	setlocale (LC_ALL, NULL) is multithread-safe in Cygwin >= 3.4.6.
> 	* lib/setlocale_null.c: Update comments.
> 	* tests/test-setlocale_null-mt-all.c: Likewise.
> 
> diff --git a/lib/setlocale_null.c b/lib/setlocale_null.c
> index 6ac563db14..89c8a06598 100644
> --- a/lib/setlocale_null.c
> +++ b/lib/setlocale_null.c
> @@ -173,7 +173,7 @@ setlocale_null_unlocked (int category, char *buf, size_t bufsize)
>  #endif
>  }
>  
> -#if !(SETLOCALE_NULL_ALL_MTSAFE && SETLOCALE_NULL_ONE_MTSAFE) /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin */
> +#if !(SETLOCALE_NULL_ALL_MTSAFE && SETLOCALE_NULL_ONE_MTSAFE) /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin < 3.4.6 */
>  
>  /* Use a lock, so that no two threads can invoke setlocale_null_unlocked
>     at the same time.  */
> @@ -198,7 +198,7 @@ setlocale_null_with_lock (int category, char *buf, size_t bufsize)
>    return ret;
>  }
>  
> -# elif HAVE_PTHREAD_API /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin */
> +# elif HAVE_PTHREAD_API /* musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin < 3.4.6 */
>  
>  extern
>  #  if defined _WIN32 || defined __CYGWIN__
> diff --git a/m4/setlocale_null.m4 b/m4/setlocale_null.m4
> index dd6a5ef538..b41df499a8 100644
> --- a/m4/setlocale_null.m4
> +++ b/m4/setlocale_null.m4
> @@ -1,4 +1,4 @@
> -# setlocale_null.m4 serial 6
> +# setlocale_null.m4 serial 7
>  dnl Copyright (C) 2019-2023 Free Software Foundation, Inc.
>  dnl This file is free software; the Free Software Foundation
>  dnl gives unlimited permission to copy and/or distribute it,
> @@ -13,9 +13,23 @@ AC_DEFUN([gl_FUNC_SETLOCALE_NULL],
>    AC_CACHE_CHECK([whether setlocale (LC_ALL, NULL) is multithread-safe],
>      [gl_cv_func_setlocale_null_all_mtsafe],
>      [case "$host_os" in
> -       # Guess no on musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku, Cygwin.
> -       *-musl* | darwin* | freebsd* | midnightbsd* | netbsd* | openbsd* | aix* | haiku* | cygwin*)
> +       # Guess no on musl libc, macOS, FreeBSD, NetBSD, OpenBSD, AIX, Haiku.
> +       *-musl* | darwin* | freebsd* | midnightbsd* | netbsd* | openbsd* | aix* | haiku*)
>           gl_cv_func_setlocale_null_all_mtsafe=no ;;
> +       # Guess no on Cygwin < 3.4.6.
> +       cygwin*)
> +         AC_EGREP_CPP([Lucky user],
> +           [
> +#if defined __CYGWIN__
> + #include <cygwin/version.h>
> + #if CYGWIN_VERSION_DLL_COMBINED >= CYGWIN_VERSION_DLL_MAKE_COMBINED (3004, 6)
> +  Lucky user
> + #endif
> +#endif
> +          ],
> +          [gl_cv_func_setlocale_null_all_mtsafe=yes],
> +          [gl_cv_func_setlocale_null_all_mtsafe=no])
> +        ;;
>         # Guess yes on glibc, HP-UX, IRIX, Solaris, native Windows.
>         *-gnu* | gnu* | hpux* | irix* | solaris* | mingw*)
>           gl_cv_func_setlocale_null_all_mtsafe=yes ;;
> diff --git a/tests/test-setlocale_null-mt-all.c b/tests/test-setlocale_null-mt-all.c
> index 6036c260cd..7480406639 100644
> --- a/tests/test-setlocale_null-mt-all.c
> +++ b/tests/test-setlocale_null-mt-all.c
> @@ -166,7 +166,7 @@ Solaris 11.0         OK
>  Solaris 11.4         OK
>  Solaris OpenIndiana  OK
>  Haiku                crash < 1 sec
> -Cygwin               crash < 1 sec
> +Cygwin < 3.4.6       crash < 1 sec
>  mingw                OK
>  MSVC                 OK (assuming compiler option /MD !)
>  */
> 
> 

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Bruno Haible]

Corinna Vinschen wrote:
> I just ran the setlocale_null-mt-* tests with gnulib from git master
> under Cygwin with my newlib patch, and the tests succeeded.  I checked
> that configure is doing the right thing and config.h contains the right
> settings:
> 
>   $ ./configure
>   [...]
>   checking whether setlocale (LC_ALL, NULL) is multithread-safe... yes
>   checking whether setlocale (category, NULL) is multithread-safe... yes
>   [...]
>   $ grep SETLOCALE_NULL config.h
>   #define GNULIB_TEST_SETLOCALE_NULL 1
>   #define SETLOCALE_NULL_ALL_MTSAFE 1
>   #define SETLOCALE_NULL_ONE_MTSAFE 1
>   $ grep SETLOCALE_NULL gltests/config.h
>   #define GNULIB_TEST_SETLOCALE_NULL 1
>   #define SETLOCALE_NULL_ALL_MTSAFE 1
>   #define SETLOCALE_NULL_ONE_MTSAFE 1

Excellent! Thank you.

The lasts remaining steps, to resolve the issue, are (as far as I see it):
  1. 'recode' is updated to a current gnulib and publish a corresponding
     tarball. (Hi Reuben :) )
  2. Then the Cygwin program packagers build this tarball on a Cygwin of
     version ≥ 3.4.6.

Bruno

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

On Feb  6 21:38, Bruno Haible wrote:
> Corinna Vinschen wrote:
> > I just ran the setlocale_null-mt-* tests with gnulib from git master
> > under Cygwin with my newlib patch, and the tests succeeded.  I checked
> > that configure is doing the right thing and config.h contains the right
> > settings:
> > 
> >   $ ./configure
> >   [...]
> >   checking whether setlocale (LC_ALL, NULL) is multithread-safe... yes
> >   checking whether setlocale (category, NULL) is multithread-safe... yes
> >   [...]
> >   $ grep SETLOCALE_NULL config.h
> >   #define GNULIB_TEST_SETLOCALE_NULL 1
> >   #define SETLOCALE_NULL_ALL_MTSAFE 1
> >   #define SETLOCALE_NULL_ONE_MTSAFE 1
> >   $ grep SETLOCALE_NULL gltests/config.h
> >   #define GNULIB_TEST_SETLOCALE_NULL 1
> >   #define SETLOCALE_NULL_ALL_MTSAFE 1
> >   #define SETLOCALE_NULL_ONE_MTSAFE 1
> 
> Excellent! Thank you.
> 
> The lasts remaining steps, to resolve the issue, are (as far as I see it):
>   1. 'recode' is updated to a current gnulib and publish a corresponding
>      tarball. (Hi Reuben :) )
>   2. Then the Cygwin program packagers build this tarball on a Cygwin of
>      version ≥ 3.4.6.

Incidentally I'm the maintainer of the recode package in the Cygwin distro
and I built 3.7.12 (as in Fedora 37) with -Wl,--export-all-symbols, which
also does the trick for now.


Thanks,
Corinna

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Reuben Thomas]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

On Mon, 6 Feb 2023 at 20:38, Bruno Haible <bruno@clisp.org> wrote:

>   1. 'recode' is updated to a current gnulib and publish a corresponding
>      tarball. (Hi Reuben :) )
>

I've updated gnulib in recode git; I'd be grateful if I could have a report
that it's doing what's needed here before I make a release, if possible!

Git at: https://github.com/rrthomas/recode.git

-- 
https://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 1348 bytes --]

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

On Feb  6 20:50, Reuben Thomas wrote:
> On Mon, 6 Feb 2023 at 20:38, Bruno Haible <bruno@clisp.org> wrote:
> 
> >   1. 'recode' is updated to a current gnulib and publish a corresponding
> >      tarball. (Hi Reuben :) )
> >
> 
> I've updated gnulib in recode git; I'd be grateful if I could have a report
> that it's doing what's needed here before I make a release, if possible!
> 
> Git at: https://github.com/rrthomas/recode.git

Working on it, but this may take a while.  Stay tuned, please.


Thanks,
Corinna

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

Hi Reuben,

On Feb  7 14:22, Corinna Vinschen wrote:
> On Feb  6 20:50, Reuben Thomas wrote:
> > On Mon, 6 Feb 2023 at 20:38, Bruno Haible <bruno@clisp.org> wrote:
> > 
> > >   1. 'recode' is updated to a current gnulib and publish a corresponding
> > >      tarball. (Hi Reuben :) )
> > >
> > 
> > I've updated gnulib in recode git; I'd be grateful if I could have a report
> > that it's doing what's needed here before I make a release, if possible!
> > 
> > Git at: https://github.com/rrthomas/recode.git
> 
> Working on it, but this may take a while.  Stay tuned, please.

The new recode gnulib build works as desired, thank you.

However, I'm trying to build recode from the Cygwin-specific build
script, and running it with default settings I get an error before
recode even got built:

    help2man: can't get `--help' info from ./recode.exe
    Try `--no-discard-stderr' if option outputs to stderr
    WARNING: '/usr/bin/help2man' is missing on your system.
	     [...]

So even with help2man installed, it doesn't work because recode.exe
didn't get build yet.

The Cygwin build system prefers parallel make.  And that's the problem.
I think commit dcdd5d26c0c2 is wrong:

    -recode.1: main.c $(top_srcdir)/configure.ac recode$(EXEEXT)
    +recode.1: main.c $(top_srcdir)/configure.ac

This means that recode.1 doesn't depend on the built binary anymore even
though it *needs* said binary to create the man page from it.  This
breaks dependency tracking in make and so make happily runs the
recode.1: rule before recode has been built, just depending on the
existence of main.c and configure.ac.

For cross-checking I tried a Linux build with `make -j8' and help2man
gets called before recode even got built as well.

So the above change breaks parallel make.

Not sure how to fix this correctly, but I have another patch, which
would be nice to have:

In Cygwin projects using libtool, we always have to add -no-undefined
iLDFLAGS.  This is some old safe-guard in libtool to remind developers
that when creating Windows DLLs, all external symbols must be resolved.

Fortunately, libtool allows this flags also on other platforms which
don't require its usage.

Patch is attached.  It would be nice if that's ok for inclusion.


Thanks,
Corinna

[-- Attachment #2: 0001-src-Makefile.am-librecode_la_LDFLAGS-add-no-undefine.patch --]
[-- Type: text/plain, Size: 1085 bytes --]

From 9ea601d4615d8420e7cf75659b647728774f65c9 Mon Sep 17 00:00:00 2001
From: Corinna Vinschen <vinschen@redhat.com>
Date: Tue, 7 Feb 2023 16:09:29 +0100
Subject: [PATCH] src/Makefile.am: librecode_la_LDFLAGS: add -no-undefined

-no-undefined is a libtool requirement to build shared libs on
Windows-based platforms like Cygwin.
---
 src/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 93d3ed3634b1..427cc8c97fe7 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -52,7 +52,7 @@ recode_LDADD = librecode.la
 librecode_la_SOURCES = charname.c combine.c fr-charname.c iconv.c \
 names.c outer.c recode.c request.c strip-pool.c task.c $(ALL_STEPS) \
 $(include_HEADERS) $(noinst_HEADERS) $(H_STEPS)
-librecode_la_LDFLAGS = -version-info $(VERSION_INFO) $(LTLIBINTL) \
+librecode_la_LDFLAGS = -no-undefined -version-info $(VERSION_INFO) $(LTLIBINTL) \
 	$(LIB_CLOCK_GETTIME) $(LIB_GETRANDOM) $(LIB_HARD_LOCALE) $(LIB_MBRTOWC) $(LIB_SETLOCALE_NULL)
 librecode_la_LIBADD = ../lib/libgnu.la libmerged.la
 
-- 
2.39.1

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Reuben Thomas]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On Tue, 7 Feb 2023 at 15:22, Corinna Vinschen <vinschen@redhat.com> wrote:

> In Cygwin projects using libtool, we always have to add -no-undefined
> iLDFLAGS.  This is some old safe-guard in libtool to remind developers
> that when creating Windows DLLs, all external symbols must be resolved.
>
> Fortunately, libtool allows this flags also on other platforms which
> don't require its usage.
>
> Patch is attached.  It would be nice if that's ok for inclusion.


I did apply this patch, but just noticed that it broke the Mingwin build;
see:

https://github.com/rrthomas/recode/actions/runs/4115515520/jobs/7104330023

I haven't had time to look into it yet; help welcome!

-- 
https://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 1858 bytes --]

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Bruno Haible]

Corinna Vinschen wrote:
> > In Cygwin projects using libtool, we always have to add -no-undefined
> > in LDFLAGS.

Likewise for AIX, IIRC.

Reuben Thomas wrote:
> > Patch is attached.  It would be nice if that's ok for inclusion.
> 
> 
> I did apply this patch, but just noticed that it broke the Mingwin build;
> see:
> 
> https://github.com/rrthomas/recode/actions/runs/4115515520/jobs/7104330023
> 
> I haven't had time to look into it yet; help welcome!

It complains about the symbols defined in libiconv. This means, you need
to invoke the Gnulib module 'iconv' and add $(LIBICONV) or $(LTLIBICONV)
to the LDFLAGS.

Bruno

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Reuben Thomas]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

On Fri, 10 Feb 2023 at 14:21, Bruno Haible <bruno@clisp.org> wrote:

> It complains about the symbols defined in libiconv. This means, you need
> to invoke the Gnulib module 'iconv' and add $(LIBICONV) or $(LTLIBICONV)
> to the LDFLAGS.
>

Bruno to the rescue again! Many thanks.

Having added the iconv gnulib module, the Windows build works, but the
tests, which use a Cython module, are now broken. The actual error is:

Traceback (most recent call last):
File "D:\a\recode\recode\tests\pytest", line 135, in main
module = __import__(base[:-3])
File "D:/a/recode/recode/tests/./t21_names.py", line 2, in <module>
import common
File "D:/a/recode/recode/tests/./common.py", line 10, in <module>
import Recode
ImportError: DLL load failed while importing Recode: The specified module
could not be found.

I assume that the "DLL load failed" indicates a link error. Obviously my
first thought was that something extra was needed for iconv; however, this
appears already to be covered (in setup.py.in). I add LIBICONV there to the
"extra_libs" argument. I had a look at the way the Python extension is
compiled and compared it with the last Windows build that passed, and they
seem to be identical; in both, libiconv.dll.a is added to the link line by
cython.

Compare the working version:
https://github.com/rrthomas/recode/actions/runs/4115503607/jobs/7104302788#step:9:2302
to the non-working:
https://github.com/rrthomas/recode/actions/runs/4150792693/jobs/7180677411#step:9:2306

They appear to be identical.

Is there some other casualty (other than iconv) of -no-undefined here?

I've just booted up my Windows VM to have a look myself, but, aside from it
being extremely slow, I would again welcome any insights you might have!

-- 
https://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 4632 bytes --]

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

On Feb 11 11:36, Reuben Thomas wrote:
> On Fri, 10 Feb 2023 at 14:21, Bruno Haible <bruno@clisp.org> wrote:
> 
> > It complains about the symbols defined in libiconv. This means, you need
> > to invoke the Gnulib module 'iconv' and add $(LIBICONV) or $(LTLIBICONV)
> > to the LDFLAGS.
> >
> 
> Bruno to the rescue again! Many thanks.
> 
> Having added the iconv gnulib module, the Windows build works, but the
> tests, which use a Cython module, are now broken. The actual error is:
> 
> Traceback (most recent call last):
> File "D:\a\recode\recode\tests\pytest", line 135, in main
> module = __import__(base[:-3])
> File "D:/a/recode/recode/tests/./t21_names.py", line 2, in <module>
> import common
> File "D:/a/recode/recode/tests/./common.py", line 10, in <module>
> import Recode
> ImportError: DLL load failed while importing Recode: The specified module
> could not be found.
> 
> I assume that the "DLL load failed" indicates a link error.

If you're sure that the native recode.dll has been built, is it possible
that it's just not found, because it's not in Windows' DLL search path?

Keep in mind that Windows searches DLLs in the OS default paths, in
$PATH, or in the same dir the executable has been started from.
https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order

So at a guess, you either just have to add the directory where
recode.dll resides to $PATH, or you have to copy the DLL into a path
already in $PATH.


Corinna

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Reuben Thomas]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On Sat, 11 Feb 2023 at 12:06, Corinna Vinschen <vinschen@redhat.com> wrote:

>
> If you're sure that the native recode.dll has been built, is it possible
> that it's just not found, because it's not in Windows' DLL search path?
>
>
Yes, because it was working fine before, and I've not changed anything
about where it is built.

-- 
https://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 1017 bytes --]

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

On Feb 11 12:29, Reuben Thomas wrote:
> On Sat, 11 Feb 2023 at 12:06, Corinna Vinschen <vinschen@redhat.com> wrote:
> 
> >
> > If you're sure that the native recode.dll has been built, is it possible
> > that it's just not found, because it's not in Windows' DLL search path?
> >
> >
> Yes, because it was working fine before, and I've not changed anything
> about where it is built.

Uhm, ok, but... given you didn't build with -no-undefined yet, I'm
pretty sure libtool only built a static librecode.a, so there's a good
chance cython was linked statrically against the recode lib, no?


Corinna

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Reuben Thomas]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Sat, 11 Feb 2023 at 12:40, Corinna Vinschen <vinschen@redhat.com> wrote:

> On Feb 11 12:29, Reuben Thomas wrote:
> > On Sat, 11 Feb 2023 at 12:06, Corinna Vinschen <vinschen@redhat.com>
> wrote:
> >
> > >
> > > If you're sure that the native recode.dll has been built, is it
> possible
> > > that it's just not found, because it's not in Windows' DLL search path?
> > >
> > >
> > Yes, because it was working fine before, and I've not changed anything
> > about where it is built.
>
> Uhm, ok, but... given you didn't build with -no-undefined yet, I'm
> pretty sure libtool only built a static librecode.a, so there's a good
> chance cython was linked statrically against the recode lib, no?
>

Thanks, that was the answer; adding the DLL's build directory to PATH on
Windows fixes the tests.

I have some issues with building dependencies of Recode on Debian to
investigate, and then I'll make another release.

-- 
https://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 1872 bytes --]

Re: [PATCH] Do not decorate symbols as dllexport on Cygwin

[Corinna Vinschen]

On Feb 17 09:30, Reuben Thomas wrote:
> On Sat, 11 Feb 2023 at 12:40, Corinna Vinschen <vinschen@redhat.com> wrote:
> 
> > On Feb 11 12:29, Reuben Thomas wrote:
> > > On Sat, 11 Feb 2023 at 12:06, Corinna Vinschen <vinschen@redhat.com>
> > wrote:
> > >
> > > >
> > > > If you're sure that the native recode.dll has been built, is it
> > possible
> > > > that it's just not found, because it's not in Windows' DLL search path?
> > > >
> > > >
> > > Yes, because it was working fine before, and I've not changed anything
> > > about where it is built.
> >
> > Uhm, ok, but... given you didn't build with -no-undefined yet, I'm
> > pretty sure libtool only built a static librecode.a, so there's a good
> > chance cython was linked statrically against the recode lib, no?
> >
> 
> Thanks, that was the answer; adding the DLL's build directory to PATH on
> Windows fixes the tests.

I'm glad I could help.

> I have some issues with building dependencies of Recode on Debian to
> investigate, and then I'll make another release.

Great, thanks!


Corinna