threads / patch / 56647

patchUse mingw.h declarations for gmtime_r/localtime_r on msys2

Subject: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

## tl;dr

11 messages between Oct 5, 2021 and Nov 19, 2021. Diffs are folded; open one to read it.

replies: 10people: 2as markdown or json

Mike Hommey· Oct 5, 2021, 06:39 UTC · lore

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.

Show changes to compat/mingw.h +0 −2
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);

-- 
2.33.0
Carlo Arenas· Oct 5, 2021, 07:12 UTC · re: Mike Hommey · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

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
Mike Hommey· Oct 5, 2021, 08:35 UTC · re: Carlo Arenas · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote:
Show 8 quoted lines
> 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
Mike Hommey· Nov 18, 2021, 03:02 UTC · re: Carlo Arenas · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote:
Show 12 quoted lines
> 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)
Carlo Arenas· Nov 18, 2021, 04:51 UTC · re: Mike Hommey · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

On Wed, Nov 17, 2021 at 7:03 PM Mike Hommey <mh@glandium.org> wrote:
Show 16 quoted lines
>
> 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
Mike Hommey· Nov 18, 2021, 05:34 UTC · re: Carlo Arenas · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

On Wed, Nov 17, 2021 at 08:51:06PM -0800, Carlo Arenas wrote:
Show 20 quoted lines
> 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.

Carlo Arenas· Nov 18, 2021, 07:58 UTC · re: Mike Hommey · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

On Wed, Nov 17, 2021 at 9:34 PM Mike Hommey <mh@glandium.org> wrote:
Show 6 quoted lines
> 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
Mike Hommey· Nov 18, 2021, 09:05 UTC · re: Carlo Arenas · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

On Wed, Nov 17, 2021 at 11:58:00PM -0800, Carlo Arenas wrote:
Show 21 quoted lines
> 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
Carlo Arenas· Nov 19, 2021, 05:38 UTC · re: Mike Hommey · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

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
Mike Hommey· Nov 19, 2021, 07:23 UTC · re: Carlo Arenas · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

On Thu, Nov 18, 2021 at 09:38:00PM -0800, Carlo Arenas wrote:
Show 9 quoted lines
> 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
Carlo Arenas· Nov 19, 2021, 09:36 UTC · re: Mike Hommey · lore

Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2

On Thu, Nov 18, 2021 at 11:24 PM Mike Hommey <mh@glandium.org> wrote:
Show 5 quoted lines
>
> 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

← back to recent threads