{"thread":{"id":"56647","subject":"[PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","startedAt":"2021-10-05T06:55:07Z","lastAt":"2021-11-19T09:37:03Z","messageCount":11,"participants":["Mike Hommey","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437956","messageId":"20211005063936.588874-1-mh@glandium.org","threadId":"56647","inReplyTo":null,"subject":"[PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2021-10-05T06:39:36Z","receivedAt":"2021-10-05T06:55:07Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Older versions of msys2 had _POSIX_THREAD_SAFE_FUNCTIONS set in\npthread_unistd.h, included from unistd.h. That would enable the\ndeclarations for gmtime_r and localtime_r in time.h.\n\nThat's not the case anymore, and gmtime_r and localtime_r end up\nbeing undeclared, which subsequently leads to \"miscompilations\", for\nexample, in datestamp(), where the result of localtime_r would be\ntruncated and sign-extended before being passed to tm_to_time_t, leading\nto segfaults at runtime.\n\nSigned-off-by: Mike Hommey \n---\n compat/mingw.h | 2 --\n 1 file changed, 2 deletions(-)\n\nA possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE\n200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR`\naround the definitions of `gmtime_r` and `localtime_r` in\ncompat/mingw.c, since, after all, they are available there.\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex c9a52ad64a..4fd989980c 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -204,10 +204,8 @@ int pipe(int filedes[2]);\n unsigned int sleep (unsigned int seconds);\n int mkstemp(char *template);\n int gettimeofday(struct timeval *tv, void *tz);\n-#ifndef __MINGW64_VERSION_MAJOR\n struct tm *gmtime_r(const time_t *timep, struct tm *result);\n struct tm *localtime_r(const time_t *timep, struct tm *result);\n-#endif\n int getpagesize(void);\t/* defined in MinGW's libgcc.a */\n struct passwd *getpwuid(uid_t uid);\n int setitimer(int type, struct itimerval *in, struct itimerval *out);\n-- \n2.33.0\n\n"},{"id":"437957","messageId":"CAPUEspgLwLxavP3bC9OEJQTphoemQ+jxv+9Nkcvbf51uaBEpww@mail.gmail.com","threadId":"56647","inReplyTo":"20211005063936.588874-1-mh@glandium.org","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-10-05T07:12:12Z","receivedAt":"2021-10-05T07:12:26Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey wrote:\n> A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE\n> 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR`\n> around the definitions of `gmtime_r` and `localtime_r` in\n> compat/mingw.c, since, after all, they are available there.\n\nsomething like that was merged to \"main\"[1] a few months ago, would\nthat work for you?\n\nCarlo\n\n[1] https://github.com/git-for-windows/git/commit/9e52042d4a4ee2d91808dda71e7f2fdf74c83862\n"},{"id":"437962","messageId":"20211005083500.jd7byba4abupdub5@glandium.org","threadId":"56647","inReplyTo":"CAPUEspgLwLxavP3bC9OEJQTphoemQ+jxv+9Nkcvbf51uaBEpww@mail.gmail.com","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2021-10-05T08:35:00Z","receivedAt":"2021-10-05T08:35:17Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote:\n> On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey wrote:\n> > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE\n> > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR`\n> > around the definitions of `gmtime_r` and `localtime_r` in\n> > compat/mingw.c, since, after all, they are available there.\n> \n> something like that was merged to \"main\"[1] a few months ago, would\n> that work for you?\n\nThis seems very close to what I was suggesting, so I would guess so :)\nI'm wondering if there's a reason not to set _POSIX_C_SOURCE everywhere,\nalong the other _*_SOURCE's.\n\nMike\n"},{"id":"441558","messageId":"20211118030255.jscp2zda4p2ewact@glandium.org","threadId":"56647","inReplyTo":"CAPUEspgLwLxavP3bC9OEJQTphoemQ+jxv+9Nkcvbf51uaBEpww@mail.gmail.com","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2021-11-18T03:02:55Z","receivedAt":"2021-11-18T03:03:08Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote:\n> On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey wrote:\n> > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE\n> > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR`\n> > around the definitions of `gmtime_r` and `localtime_r` in\n> > compat/mingw.c, since, after all, they are available there.\n> \n> something like that was merged to \"main\"[1] a few months ago, would\n> that work for you?\n> \n> Carlo\n> \n> [1] https://github.com/git-for-windows/git/commit/9e52042d4a4ee2d91808dda71e7f2fdf74c83862\n\nSince this reached 2.34, I gave it a try, and it turns out this didn't\nfix it:\ndate.c:70:9: error: implicit declaration of function 'gmtime_r'; did you mean 'gmtime_s'? [-Werror=implicit-function-declaration]\ndate.c:76:9: error: implicit declaration of function 'localtime_r'; did you mean 'localtime_s'? [-Werror=implicit-function-declaration]\n\n(presumably because _POSIX_C_SOURCE is not defined)\n"},{"id":"441562","messageId":"CAPUEspg-5+YdfTJ6zi9hdDqF=KV2LJFCtqmECSss9Kfpn6sGrQ@mail.gmail.com","threadId":"56647","inReplyTo":"20211118030255.jscp2zda4p2ewact@glandium.org","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-18T04:51:06Z","receivedAt":"2021-11-18T04:51:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 17, 2021 at 7:03 PM Mike Hommey wrote:\n>\n> On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote:\n> > On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey wrote:\n> > > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE\n> > > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR`\n> > > around the definitions of `gmtime_r` and `localtime_r` in\n> > > compat/mingw.c, since, after all, they are available there.\n> >\n> > something like that was merged to \"main\"[1] a few months ago, would\n> > that work for you?\n> >\n> > Carlo\n> >\n> > [1] https://github.com/git-for-windows/git/commit/9e52042d4a4ee2d91808dda71e7f2fdf74c83862\n>\n> Since this reached 2.34\n\nIt is not in 2.34; only in the git for windows fork, but agree is\nneeded if you are building master with a newish mingw\n\nguess I got the perfect excuse to get myself approved for gitgitgadget\nwith this PR[1] then\n\nCarlo\n\n[1] https://github.com/git/git/pull/1142\n"},{"id":"441567","messageId":"20211118053415.4axljmr4s6kmqmms@glandium.org","threadId":"56647","inReplyTo":"CAPUEspg-5+YdfTJ6zi9hdDqF=KV2LJFCtqmECSss9Kfpn6sGrQ@mail.gmail.com","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2021-11-18T05:34:15Z","receivedAt":"2021-11-18T05:34:32Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Nov 17, 2021 at 08:51:06PM -0800, Carlo Arenas wrote:\n> On Wed, Nov 17, 2021 at 7:03 PM Mike Hommey wrote:\n> >\n> > On Tue, Oct 05, 2021 at 12:12:12AM -0700, Carlo Arenas wrote:\n> > > On Mon, Oct 4, 2021 at 11:57 PM Mike Hommey wrote:\n> > > > A possible alternative fix would be to e.g. add `#define _POSIX_C_SOURCE\n> > > > 200112L` to git-compat-util.h and add `ifndef __MINGW64_VERSION_MAJOR`\n> > > > around the definitions of `gmtime_r` and `localtime_r` in\n> > > > compat/mingw.c, since, after all, they are available there.\n> > >\n> > > something like that was merged to \"main\"[1] a few months ago, would\n> > > that work for you?\n> > >\n> > > Carlo\n> > >\n> > > [1] https://github.com/git-for-windows/git/commit/9e52042d4a4ee2d91808dda71e7f2fdf74c83862\n> >\n> > Since this reached 2.34\n> \n> It is not in 2.34; only in the git for windows fork, but agree is\n> needed if you are building master with a newish mingw\n\nErr, I did mean 2.34.0.windows.1. My working workaround is to build with\n-D_POSIX_THREAD_SAFE_FUNCTIONS=200112L.\n\n"},{"id":"441580","messageId":"CAPUEsphf0d90HGg64j=jZnt-Xuhs_bwmeOyoUnmzesp_k2c4JA@mail.gmail.com","threadId":"56647","inReplyTo":"20211118053415.4axljmr4s6kmqmms@glandium.org","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-18T07:58:00Z","receivedAt":"2021-11-18T07:58:19Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 17, 2021 at 9:34 PM Mike Hommey wrote:\n> On Wed, Nov 17, 2021 at 08:51:06PM -0800, Carlo Arenas wrote:\n> > It is not in 2.34; only in the git for windows fork, but agree is\n> > needed if you are building master with a newish mingw\n>\n> Err, I did mean 2.34.0.windows.1. My working workaround is to build with\n> -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L.\n\nthat is strange, building main/2.34.0.windows.1 works for me both in a\nmingw64 shell and the git for windows sdk, and the PR[1] worked as\nwell when applied to 2.34/master that uses a git for windows sdk for\nbuilding it and that would had failed without it as you reported.\n\nwhat version `pacman -q | grep pthread` of the winpthreads library do\nyou have?, anything else peculiar about your build environment that\nyou could think of?\n\nthat define and the setting in git-compat-util.h should have\nequivalent effect in your mingw headers; what does the relevant\n(almost at the bottom, where the problematic functions are defined)\npart of /mingw64/x86_64-w64-mingw32/include/time.h say?\n\nCarlo\n\n[1] https://github.com/git/git/pull/1142\n"},{"id":"441584","messageId":"20211118090542.rcaggue6zpd7r3ht@glandium.org","threadId":"56647","inReplyTo":"CAPUEsphf0d90HGg64j=jZnt-Xuhs_bwmeOyoUnmzesp_k2c4JA@mail.gmail.com","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2021-11-18T09:05:42Z","receivedAt":"2021-11-18T09:06:27Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Nov 17, 2021 at 11:58:00PM -0800, Carlo Arenas wrote:\n> On Wed, Nov 17, 2021 at 9:34 PM Mike Hommey wrote:\n> > On Wed, Nov 17, 2021 at 08:51:06PM -0800, Carlo Arenas wrote:\n> > > It is not in 2.34; only in the git for windows fork, but agree is\n> > > needed if you are building master with a newish mingw\n> >\n> > Err, I did mean 2.34.0.windows.1. My working workaround is to build with\n> > -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L.\n> \n> that is strange, building main/2.34.0.windows.1 works for me both in a\n> mingw64 shell and the git for windows sdk, and the PR[1] worked as\n> well when applied to 2.34/master that uses a git for windows sdk for\n> building it and that would had failed without it as you reported.\n> \n> what version `pacman -q | grep pthread` of the winpthreads library do\n> you have?, anything else peculiar about your build environment that\n> you could think of?\n> \n> that define and the setting in git-compat-util.h should have\n> equivalent effect in your mingw headers; what does the relevant\n> (almost at the bottom, where the problematic functions are defined)\n> part of /mingw64/x86_64-w64-mingw32/include/time.h say?\n\nOh my bad, I overlooked an important part of the build log: it was a\nmingw32 build, not minwg64. Mingw64 builds fine without\n-D_POSIX_THREAD_SAFE_FUNCTIONS=200112L. Mingw32 requires it (because\nthe ifdefs are for mingw64)\n\nMike\n"},{"id":"441691","messageId":"CAPUEspjZmwoOWSJHBrykOfNEv=zLi2nQLs1EkUPTPr-nSNf08Q@mail.gmail.com","threadId":"56647","inReplyTo":"20211118090542.rcaggue6zpd7r3ht@glandium.org","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-19T05:38:00Z","receivedAt":"2021-11-19T05:38:25Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Nov 18, 2021 at 1:05 AM Mike Hommey wrote:\n> Oh my bad, I overlooked an important part of the build log: it was a\n> mingw32 build, not minwg64. Mingw64 builds fine without\n> -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L. Mingw32 requires it (because\n> the ifdefs are for mingw64)\n\nCan you confirm the version of the winpthread library in your SDK? and\noutput of your headers, or something that could back up that statement\nof \"ifdefs are for mingw64\"?.\n\n I definitely can't reproduce it, but I also have a freshly installed\n32-bit SDK.\n\nThe proposed change was meant to be backward compatible though, which\nis why I am holding on submitting it to git.git and even advocating\nthrowing it away (even if it has been in use for several months) and\nreplacing it with your original proposal, but would be good to\nunderstand why it fails, and why yours wouldn't.\n\nCarlo\n"},{"id":"441702","messageId":"20211119072357.oxl5caye742blz5j@glandium.org","threadId":"56647","inReplyTo":"CAPUEspjZmwoOWSJHBrykOfNEv=zLi2nQLs1EkUPTPr-nSNf08Q@mail.gmail.com","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2021-11-19T07:23:57Z","receivedAt":"2021-11-19T07:24:09Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, Nov 18, 2021 at 09:38:00PM -0800, Carlo Arenas wrote:\n> On Thu, Nov 18, 2021 at 1:05 AM Mike Hommey wrote:\n> > Oh my bad, I overlooked an important part of the build log: it was a\n> > mingw32 build, not minwg64. Mingw64 builds fine without\n> > -D_POSIX_THREAD_SAFE_FUNCTIONS=200112L. Mingw32 requires it (because\n> > the ifdefs are for mingw64)\n> \n> Can you confirm the version of the winpthread library in your SDK? and\n> output of your headers, or something that could back up that statement\n> of \"ifdefs are for mingw64\"?.\n\nThe ifdef around gmtime_r and localtime_r in mingw.h is for __MINGW64_VERSION_MAJOR.\nThe ifdef around _POSIX_C_SOURCE in git-compat-util.h is for\n__MINGW64__.\nI'd imagine that plays a role.\n\nwinpthreads version on my system is 9.0.0.6246.ae63cde27-1.\n\nThe /mingw32/i686-w64-mingw32/include/time.h section related to gtime_r\nand localtime_r starts with:\n```\n#if defined(_POSIX_C_SOURCE) && !defined(_POSIX_THREAD_SAFE_FUNCTIONS)\n#define _POSIX_THREAD_SAFE_FUNCTIONS 200112L\n#endif\n#ifdef _POSIX_THREAD_SAFE_FUNCTIONS\n__forceinline struct tm *__CRTDECL localtime_r(const time_t *_Time, struct tm *_Tm) {\n return localtime_s(_Tm, _Time) ? NULL : _Tm;\n}\n__forceinline struct tm *__CRTDECL gmtime_r(const time_t *_Time, struct tm *_Tm) {\n return gmtime_s(_Tm, _Time) ? NULL : _Tm;\n}\n```\n\nMike\n"},{"id":"441709","messageId":"CAPUEspic-qgLWGgcJeO52A1XeUh7p+F_WKfH0NLYMmMRWL7Cnw@mail.gmail.com","threadId":"56647","inReplyTo":"20211119072357.oxl5caye742blz5j@glandium.org","subject":"Re: [PATCH] Use mingw.h declarations for gmtime_r/localtime_r on msys2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-19T09:36:49Z","receivedAt":"2021-11-19T09:37:03Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Nov 18, 2021 at 11:24 PM Mike Hommey wrote:\n>\n> The ifdef around gmtime_r and localtime_r in mingw.h is for __MINGW64_VERSION_MAJOR.\n> The ifdef around _POSIX_C_SOURCE in git-compat-util.h is for\n> __MINGW64__.\n> I'd imagine that plays a role.\n\nMust be, and indeed you are right that my 32-bit compiler doesn't set\n__MINGW64__ (only __MINGW32__) and I am not hitting that codepath, but\nI have no problem building and it seems it is because\n_POSIX_THREAD_SAFE_FUNCTIONS it is defined unconditionally at the end\nof pthread_unistd.h, unlike what is done for x86_64.\n\nsomehow your version of headers might had that removed in both, and\nthat was \"fixed\" later. I have 6346 in i386 and 6306 in x86_64.\n\nCarlo\n"}]}