Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
On Feb 17 09:30, Reuben Thomas wrote: > On Sat, 11 Feb 2023 at 12:40, Corinna Vinschen wrote: > > > On Feb 11 12:29, Reuben Thomas wrote: > > > On Sat, 11 Feb 2023 at 12:06, Corinna Vinschen > > 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
Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
On Sat, 11 Feb 2023 at 12:40, Corinna Vinschen wrote: > On Feb 11 12:29, Reuben Thomas wrote: > > On Sat, 11 Feb 2023 at 12:06, Corinna Vinschen > 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
Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
On Feb 11 12:29, Reuben Thomas wrote: > On Sat, 11 Feb 2023 at 12:06, Corinna Vinschen 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
On Sat, 11 Feb 2023 at 12:06, Corinna Vinschen 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
Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
On Feb 11 11:36, Reuben Thomas wrote: > On Fri, 10 Feb 2023 at 14:21, Bruno Haible 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 > import common > File "D:/a/recode/recode/tests/./common.py", line 10, in > 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
On Fri, 10 Feb 2023 at 14:21, Bruno Haible 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 import common File "D:/a/recode/recode/tests/./common.py", line 10, in 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
Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
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
On Tue, 7 Feb 2023 at 15:22, Corinna Vinschen 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
Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
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 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 >From 9ea601d4615d8420e7cf75659b647728774f65c9 Mon Sep 17 00:00:00 2001 From: Corinna Vinschen 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
On Feb 6 20:50, Reuben Thomas wrote: > On Mon, 6 Feb 2023 at 20:38, Bruno Haible 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
On Mon, 6 Feb 2023 at 20:38, Bruno Haible 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
Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
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
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
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 > > setlocale-null: Don't use a lock in Cygwin >= 3.4.6. > Road paved by Corinna Vinschen . > * 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 > + #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 > Haikucrash < 1 sec > -Cygwin crash < 1 sec > +Cygwin < 3.4.6 crash < 1 sec > mingwOK > MSVC OK (assuming compiler option /MD !) > */ > >
Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
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 setlocale-null: Don't use a lock in Cygwin >= 3.4.6. Road paved by Corinna Vinschen . * 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 + #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 Haikucrash < 1 sec -Cygwin crash < 1 sec +Cygwin < 3.4.6 crash < 1 sec mingwOK MSVC OK (assuming compiler option /MD !) */
Re: [PATCH] Do not decorate symbols as dllexport on Cygwin
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
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
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
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
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
[PATCH] Do not decorate symbols as dllexport on Cygwin
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
