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

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

[Mike Hommey]

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.

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

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

[Carlo Arenas]

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

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

[Mike Hommey]

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

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

[Mike Hommey]

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)

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

[Carlo Arenas]

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

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

[Mike Hommey]

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.

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

[Carlo Arenas]

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

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

[Mike Hommey]

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

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

[Carlo Arenas]

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

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

[Mike Hommey]

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

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

[Carlo Arenas]

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