| Message ID | 20211005063936.588874-1-mh@glandium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use mingw.h declarations for gmtime_r/localtime_r on msys2 | expand |
On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey <mh@glandium.org> wrote: > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR` > around the definitions of `gmtime_r` and `localtime_r` in > compat/mingw.c, since, after all, they are available there. something like that was merged to "main"[1] a few months ago, would that work for you? Carlo [1] https://github.com/git-for-windows/git/commit/9e52042d4a4ee2d91808dda71e7f2fdf74c83862
On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote: > On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey <mh@glandium.org> wrote: > > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE > > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR` > > around the definitions of `gmtime_r` and `localtime_r` in > > compat/mingw.c, since, after all, they are available there. > > something like that was merged to "main"[1] a few months ago, would > that work for you? This seems very close to what I was suggesting, so I would guess so :) I'm wondering if there's a reason not to set _POSIX_C_SOURCE everywhere, along the other _*_SOURCE's. Mike
On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote: > On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey <mh@glandium.org> wrote: > > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE > > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR` > > around the definitions of `gmtime_r` and `localtime_r` in > > compat/mingw.c, since, after all, they are available there. > > something like that was merged to "main"[1] a few months ago, would > that work for you? > > Carlo > > [1] https://github.com/git-for-windows/git/commit/9e52042d4a4ee2d91808dda71e7f2fdf74c83862 Since this reached 2.34, I gave it a try, and it turns out this didn't fix it: date.c:70:9: error: implicit declaration of function 'gmtime_r'; did you mean 'gmtime_s'? [-Werror=implicit-function-declaration] date.c:76:9: error: implicit declaration of function 'localtime_r'; did you mean 'localtime_s'? [-Werror=implicit-function-declaration] (presumably because _POSIX_C_SOURCE is not defined)
On Wed, Nov 17, 2021 at 7:03 PM Mike Hommey <mh@glandium.org> wrote: > > On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote: > > On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey <mh@glandium.org> wrote: > > > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE > > > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR` > > > around the definitions of `gmtime_r` and `localtime_r` in > > > compat/mingw.c, since, after all, they are available there. > > > > something like that was merged to "main"[1] a few months ago, would > > that work for you? > > > > Carlo > > > > [1] https://github.com/git-for-windows/git/commit/9e52042d4a4ee2d91808dda71e7f2fdf74c83862 > > Since this reached 2.34 It is not in 2.34; only in the git for windows fork, but agree is needed if you are building master with a newish mingw guess I got the perfect excuse to get myself approved for gitgitgadget with this PR[1] then Carlo [1] https://github.com/git/git/pull/1142
On Wed, Nov 17, 2021 at 08:51:06PM -0800, Carlo Arenas wrote: > On Wed, Nov 17, 2021 at 7:03 PM Mike Hommey <mh@glandium.org> wrote: > > > > On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote: > > > On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey <mh@glandium.org> wrote: > > > > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE > > > > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR` > > > > around the definitions of `gmtime_r` and `localtime_r` in > > > > compat/mingw.c, since, after all, they are available there. > > > > > > something like that was merged to "main"[1] a few months ago, would > > > that work for you? > > > > > > Carlo > > > > > > [1] https://github.com/git-for-windows/git/commit/9e52042d4a4ee2d91808dda71e7f2fdf74c83862 > > > > Since this reached 2.34 > > It is not in 2.34; only in the git for windows fork, but agree is > needed if you are building master with a newish mingw Err, I did mean 2.34.0.windows.1. My working workaround is to build with -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L.
On Wed, Nov 17, 2021 at 9:34 PM Mike Hommey <mh@glandium.org> wrote: > On Wed, Nov 17, 2021 at 08:51:06PM -0800, Carlo Arenas wrote: > > It is not in 2.34; only in the git for windows fork, but agree is > > needed if you are building master with a newish mingw > > Err, I did mean 2.34.0.windows.1. My working workaround is to build with > -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L. that is strange, building main/2.34.0.windows.1 works for me both in a mingw64 shell and the git for windows sdk, and the PR[1] worked as well when applied to 2.34/master that uses a git for windows sdk for building it and that would had failed without it as you reported. what version `pacman -q | grep pthread` of the winpthreads library do you have?, anything else peculiar about your build environment that you could think of? that define and the setting in git-compat-util.h should have equivalent effect in your mingw headers; what does the relevant (almost at the bottom, where the problematic functions are defined) part of /mingw64/x86_64-w64-mingw32/include/time.h say? Carlo [1] https://github.com/git/git/pull/1142
On Wed, Nov 17, 2021 at 11:58:00PM -0800, Carlo Arenas wrote: > On Wed, Nov 17, 2021 at 9:34 PM Mike Hommey <mh@glandium.org> wrote: > > On Wed, Nov 17, 2021 at 08:51:06PM -0800, Carlo Arenas wrote: > > > It is not in 2.34; only in the git for windows fork, but agree is > > > needed if you are building master with a newish mingw > > > > Err, I did mean 2.34.0.windows.1. My working workaround is to build with > > -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L. > > that is strange, building main/2.34.0.windows.1 works for me both in a > mingw64 shell and the git for windows sdk, and the PR[1] worked as > well when applied to 2.34/master that uses a git for windows sdk for > building it and that would had failed without it as you reported. > > what version `pacman -q | grep pthread` of the winpthreads library do > you have?, anything else peculiar about your build environment that > you could think of? > > that define and the setting in git-compat-util.h should have > equivalent effect in your mingw headers; what does the relevant > (almost at the bottom, where the problematic functions are defined) > part of /mingw64/x86_64-w64-mingw32/include/time.h say? Oh my bad, I overlooked an important part of the build log: it was a mingw32 build, not minwg64. Mingw64 builds fine without -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L. Mingw32 requires it (because the ifdefs are for mingw64) Mike
On Thu, Nov 18, 2021 at 1:05 AM Mike Hommey <mh@glandium.org> wrote: > Oh my bad, I overlooked an important part of the build log: it was a > mingw32 build, not minwg64. Mingw64 builds fine without > -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L. Mingw32 requires it (because > the ifdefs are for mingw64) Can you confirm the version of the winpthread library in your SDK? and output of your headers, or something that could back up that statement of "ifdefs are for mingw64"?. I definitely can't reproduce it, but I also have a freshly installed 32-bit SDK. The proposed change was meant to be backward compatible though, which is why I am holding on submitting it to git.git and even advocating throwing it away (even if it has been in use for several months) and replacing it with your original proposal, but would be good to understand why it fails, and why yours wouldn't. Carlo
On Thu, Nov 18, 2021 at 09:38:00PM -0800, Carlo Arenas wrote: > On Thu, Nov 18, 2021 at 1:05 AM Mike Hommey <mh@glandium.org> wrote: > > Oh my bad, I overlooked an important part of the build log: it was a > > mingw32 build, not minwg64. Mingw64 builds fine without > > -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L. Mingw32 requires it (because > > the ifdefs are for mingw64) > > Can you confirm the version of the winpthread library in your SDK? and > output of your headers, or something that could back up that statement > of "ifdefs are for mingw64"?. The ifdef around gmtime_r and localtime_r in mingw.h is for __MINGW64_VERSION_MAJOR. The ifdef around _POSIX_C_SOURCE in git-compat-util.h is for __MINGW64__. I'd imagine that plays a role. winpthreads version on my system is 9.0.0.6246.ae63cde27-1. The /mingw32/i686-w64-mingw32/include/time.h section related to gtime_r and localtime_r starts with: ``` #if defined(_POSIX_C_SOURCE) && !defined(_POSIX_THREAD_SAFE_FUNCTIONS) #define _POSIX_THREAD_SAFE_FUNCTIONS 200112L #endif #ifdef _POSIX_THREAD_SAFE_FUNCTIONS __forceinline struct tm *__CRTDECL localtime_r(const time_t *_Time, struct tm *_Tm) { return localtime_s(_Tm, _Time) ? NULL : _Tm; } __forceinline struct tm *__CRTDECL gmtime_r(const time_t *_Time, struct tm *_Tm) { return gmtime_s(_Tm, _Time) ? NULL : _Tm; } ``` Mike
On Thu, Nov 18, 2021 at 11:24 PM Mike Hommey <mh@glandium.org> wrote: > > The ifdef around gmtime_r and localtime_r in mingw.h is for __MINGW64_VERSION_MAJOR. > The ifdef around _POSIX_C_SOURCE in git-compat-util.h is for > __MINGW64__. > I'd imagine that plays a role. Must be, and indeed you are right that my 32-bit compiler doesn't set __MINGW64__ (only __MINGW32__) and I am not hitting that codepath, but I have no problem building and it seems it is because _POSIX_THREAD_SAFE_FUNCTIONS it is defined unconditionally at the end of pthread_unistd.h, unlike what is done for x86_64. somehow your version of headers might had that removed in both, and that was "fixed" later. I have 6346 in i386 and 6306 in x86_64. Carlo
diff --git a/compat/mingw.h b/compat/mingw.h index c9a52ad64a..4fd989980c 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -204,10 +204,8 @@ int pipe(int filedes[2]); unsigned int sleep (unsigned int seconds); int mkstemp(char *template); int gettimeofday(struct timeval *tv, void *tz); -#ifndef __MINGW64_VERSION_MAJOR struct tm *gmtime_r(const time_t *timep, struct tm *result); struct tm *localtime_r(const time_t *timep, struct tm *result); -#endif int getpagesize(void); /* defined in MinGW's libgcc.a */ struct passwd *getpwuid(uid_t uid); int setitimer(int type, struct itimerval *in, struct itimerval *out);
Older versions of msys2 had _POSIX_THREAD_SAFE_FUNCTIONS set in pthread_unistd.h, included from unistd.h. That would enable the declarations for gmtime_r and localtime_r in time.h. That's not the case anymore, and gmtime_r and localtime_r end up being undeclared, which subsequently leads to "miscompilations", for example, in datestamp(), where the result of localtime_r would be truncated and sign-extended before being passed to tm_to_time_t, leading to segfaults at runtime. Signed-off-by: Mike Hommey <mh@glandium.org> --- compat/mingw.h | 2 -- 1 file changed, 2 deletions(-) A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR` around the definitions of `gmtime_r` and `localtime_r` in compat/mingw.c, since, after all, they are available there.