threads / patch / 49727

patchMake compat/poll safer on Windows

Subject: [PATCH 0/1] Make compat/poll safer on Windows

## tl;dr

17 messages between Oct 31, 2018 and Nov 5, 2018. Diffs are folded; open one to read it.

replies: 16people: 7as markdown or json

Johannes Schindelin via GitGitGadget· Oct 31, 2018, 21:11 UTC · lore

This is yet another piece from the Git for Windows cake. It avoids a wrap-around in the poll emulation on Windows that occurs every 49 days.

Steve Hoelzer (1):
  poll: use GetTickCount64() to avoid wrap-around issues
 compat/poll/poll.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-64%2Fdscho%2Fmingw-safer-compat-poll-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-64/dscho/mingw-safer-compat-poll-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/64
-- 
gitgitgadget
Steve Hoelzer via GitGitGadget· Oct 31, 2018, 21:11 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

From: Steve Hoelzer <shoelzer@gmail.com>

From Visual Studio 2015 Code Analysis: Warning C28159 Consider using 'GetTickCount64' instead of 'GetTickCount'.

Reason: GetTickCount() overflows roughly every 49 days. Code that does
not take that into account can loop indefinitely. GetTickCount64()
operates on 64 bit values and does not have that problem.
Note: this patch has been carried in Git for Windows for almost two
years, but with a fallback for Windows XP, as the GetTickCount64()
function is only available on Windows Vista and later. However, in the
meantime we require Vista or later, hence we can drop that fallback.
Signed-off-by: Steve Hoelzer <shoelzer@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/poll/poll.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
Show changes to compat/poll/poll.c +7 −3
diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index ad5dcde439..4abbfcb6a4 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -18,6 +18,9 @@
    You should have received a copy of the GNU General Public License along
    with this program; if not, see <http://www.gnu.org/licenses/>.  */
 
+/* To bump the minimum Windows version to Windows Vista */
+#include "git-compat-util.h"
+
 /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
 #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
 # pragma GCC diagnostic ignored "-Wtype-limits"
@@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   static HANDLE hEvent;
   WSANETWORKEVENTS ev;
   HANDLE h, handle_array[FD_SETSIZE + 2];
-  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
+  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
+  ULONGLONG start = 0;
   fd_set rfds, wfds, xfds;
   BOOL poll_again;
   MSG msg;
@@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   if (timeout != INFTIM)
     {
       orig_timeout = timeout;
-      start = GetTickCount();
+      start = GetTickCount64();
     }
 
   if (!hEvent)
@@ -614,7 +618,7 @@ restart:
 
   if (!rc && orig_timeout && timeout != INFTIM)
     {
-      elapsed = GetTickCount() - start;
+      elapsed = (DWORD)(GetTickCount64() - start);
       timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
     }
 
-- 
gitgitgadget
Johannes Sixt· Nov 1, 2018, 10:21 UTC · re: Steve Hoelzer via GitGitGadget · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
Show 59 quoted lines
> From: Steve Hoelzer <shoelzer@gmail.com>
> 
>  From Visual Studio 2015 Code Analysis: Warning C28159 Consider using
> 'GetTickCount64' instead of 'GetTickCount'.
> 
> Reason: GetTickCount() overflows roughly every 49 days. Code that does
> not take that into account can loop indefinitely. GetTickCount64()
> operates on 64 bit values and does not have that problem.
> 
> Note: this patch has been carried in Git for Windows for almost two
> years, but with a fallback for Windows XP, as the GetTickCount64()
> function is only available on Windows Vista and later. However, in the
> meantime we require Vista or later, hence we can drop that fallback.
> 
> Signed-off-by: Steve Hoelzer <shoelzer@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/poll/poll.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> index ad5dcde439..4abbfcb6a4 100644
> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -18,6 +18,9 @@
>      You should have received a copy of the GNU General Public License along
>      with this program; if not, see <http://www.gnu.org/licenses/>.  */
>   
> +/* To bump the minimum Windows version to Windows Vista */
> +#include "git-compat-util.h"
> +
>   /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
>   #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
>   # pragma GCC diagnostic ignored "-Wtype-limits"
> @@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>     static HANDLE hEvent;
>     WSANETWORKEVENTS ev;
>     HANDLE h, handle_array[FD_SETSIZE + 2];
> -  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
> +  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
> +  ULONGLONG start = 0;
>     fd_set rfds, wfds, xfds;
>     BOOL poll_again;
>     MSG msg;
> @@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>     if (timeout != INFTIM)
>       {
>         orig_timeout = timeout;
> -      start = GetTickCount();
> +      start = GetTickCount64();
>       }
>   
>     if (!hEvent)
> @@ -614,7 +618,7 @@ restart:
>   
>     if (!rc && orig_timeout && timeout != INFTIM)
>       {
> -      elapsed = GetTickCount() - start;
> +      elapsed = (DWORD)(GetTickCount64() - start);

AFAICS, this subtraction in the old code is the correct way to take account of wrap-arounds in the tick count. The new code truncates the 64 bit difference to 32 bits; the result is exactly identical to a difference computed from truncated 32 bit values, which is what we had in the old code.

IOW, there is no change in behavior. The statement "avoid wrap-around issues" in the subject line is not correct. The patch's only effect is that it removes Warning C28159.

What is really needed is that all quantities in the calculations are promoted to ULONGLONG. Unless, of course, we agree that a timeout of more than 49 days cannot happen ;)

>         timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
>       }
>   
-- Hannes
Steve Hoelzer· Nov 2, 2018, 14:47 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt <j6t@kdbg.org> wrote:
Show 75 quoted lines
>
> Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
> > From: Steve Hoelzer <shoelzer@gmail.com>
> >
> >  From Visual Studio 2015 Code Analysis: Warning C28159 Consider using
> > 'GetTickCount64' instead of 'GetTickCount'.
> >
> > Reason: GetTickCount() overflows roughly every 49 days. Code that does
> > not take that into account can loop indefinitely. GetTickCount64()
> > operates on 64 bit values and does not have that problem.
> >
> > Note: this patch has been carried in Git for Windows for almost two
> > years, but with a fallback for Windows XP, as the GetTickCount64()
> > function is only available on Windows Vista and later. However, in the
> > meantime we require Vista or later, hence we can drop that fallback.
> >
> > Signed-off-by: Steve Hoelzer <shoelzer@gmail.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >   compat/poll/poll.c | 10 +++++++---
> >   1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> > index ad5dcde439..4abbfcb6a4 100644
> > --- a/compat/poll/poll.c
> > +++ b/compat/poll/poll.c
> > @@ -18,6 +18,9 @@
> >      You should have received a copy of the GNU General Public License along
> >      with this program; if not, see <http://www.gnu.org/licenses/>.  */
> >
> > +/* To bump the minimum Windows version to Windows Vista */
> > +#include "git-compat-util.h"
> > +
> >   /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
> >   #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
> >   # pragma GCC diagnostic ignored "-Wtype-limits"
> > @@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
> >     static HANDLE hEvent;
> >     WSANETWORKEVENTS ev;
> >     HANDLE h, handle_array[FD_SETSIZE + 2];
> > -  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
> > +  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
> > +  ULONGLONG start = 0;
> >     fd_set rfds, wfds, xfds;
> >     BOOL poll_again;
> >     MSG msg;
> > @@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
> >     if (timeout != INFTIM)
> >       {
> >         orig_timeout = timeout;
> > -      start = GetTickCount();
> > +      start = GetTickCount64();
> >       }
> >
> >     if (!hEvent)
> > @@ -614,7 +618,7 @@ restart:
> >
> >     if (!rc && orig_timeout && timeout != INFTIM)
> >       {
> > -      elapsed = GetTickCount() - start;
> > +      elapsed = (DWORD)(GetTickCount64() - start);
>
> AFAICS, this subtraction in the old code is the correct way to take
> account of wrap-arounds in the tick count. The new code truncates the 64
> bit difference to 32 bits; the result is exactly identical to a
> difference computed from truncated 32 bit values, which is what we had
> in the old code.
>
> IOW, there is no change in behavior. The statement "avoid wrap-around
> issues" in the subject line is not correct. The patch's only effect is
> that it removes Warning C28159.
>
> What is really needed is that all quantities in the calculations are
> promoted to ULONGLONG. Unless, of course, we agree that a timeout of
> more than 49 days cannot happen ;)

Yep, correct on all counts. I'm in favor of changing the commit message to only say that this patch removes Warning C28159.

Steve
Johannes Sixt· Nov 2, 2018, 16:43 UTC · re: Steve Hoelzer · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
Show 26 quoted lines
> On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt <j6t@kdbg.org> wrote:
>>
>> Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
>>> @@ -614,7 +618,7 @@ restart:
>>>
>>>      if (!rc && orig_timeout && timeout != INFTIM)
>>>        {
>>> -      elapsed = GetTickCount() - start;
>>> +      elapsed = (DWORD)(GetTickCount64() - start);
>>
>> AFAICS, this subtraction in the old code is the correct way to take
>> account of wrap-arounds in the tick count. The new code truncates the 64
>> bit difference to 32 bits; the result is exactly identical to a
>> difference computed from truncated 32 bit values, which is what we had
>> in the old code.
>>
>> IOW, there is no change in behavior. The statement "avoid wrap-around
>> issues" in the subject line is not correct. The patch's only effect is
>> that it removes Warning C28159.
>>
>> What is really needed is that all quantities in the calculations are
>> promoted to ULONGLONG. Unless, of course, we agree that a timeout of
>> more than 49 days cannot happen ;)
> 
> Yep, correct on all counts. I'm in favor of changing the commit message to
> only say that this patch removes Warning C28159.
How about this fixup instead?

---- 8< ---- squash! poll: use GetTickCount64() to avoid wrap-around issues

The value of timeout starts as an int value, and for this reason it cannot overflow unsigned long long aka ULONGLONG. The unsigned version of this initial value is available in orig_timeout. The difference (orig_timeout - elapsed) cannot wrap around because it is protected by a conditional (as can be seen in the patch text). Hence, the ULONGLONG difference can only have values that are smaller than the initial timeout value and truncation to int cannot overflow.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 compat/poll/poll.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to compat/poll/poll.c +3 −3
diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index 4abbfcb6a4..4459408c7d 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -452,7 +452,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   static HANDLE hEvent;
   WSANETWORKEVENTS ev;
   HANDLE h, handle_array[FD_SETSIZE + 2];
-  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
+  DWORD ret, wait_timeout, nhandles, orig_timeout = 0;
   ULONGLONG start = 0;
   fd_set rfds, wfds, xfds;
   BOOL poll_again;
@@ -618,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
 
   if (!rc && orig_timeout && timeout != INFTIM)
     {
-      elapsed = (DWORD)(GetTickCount64() - start);
-      timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
+      ULONGLONG elapsed = GetTickCount64() - start;
+      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
     }
 
   if (!rc && timeout)
-- 
2.19.1.406.g1aa3f475f3
Steve Hoelzer· Nov 2, 2018, 17:18 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

On Fri, Nov 2, 2018 at 11:43 AM Johannes Sixt <j6t@kdbg.org> wrote:
Show 73 quoted lines
>
> Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
> > On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt <j6t@kdbg.org> wrote:
> >>
> >> Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
> >>> @@ -614,7 +618,7 @@ restart:
> >>>
> >>>      if (!rc && orig_timeout && timeout != INFTIM)
> >>>        {
> >>> -      elapsed = GetTickCount() - start;
> >>> +      elapsed = (DWORD)(GetTickCount64() - start);
> >>
> >> AFAICS, this subtraction in the old code is the correct way to take
> >> account of wrap-arounds in the tick count. The new code truncates the 64
> >> bit difference to 32 bits; the result is exactly identical to a
> >> difference computed from truncated 32 bit values, which is what we had
> >> in the old code.
> >>
> >> IOW, there is no change in behavior. The statement "avoid wrap-around
> >> issues" in the subject line is not correct. The patch's only effect is
> >> that it removes Warning C28159.
> >>
> >> What is really needed is that all quantities in the calculations are
> >> promoted to ULONGLONG. Unless, of course, we agree that a timeout of
> >> more than 49 days cannot happen ;)
> >
> > Yep, correct on all counts. I'm in favor of changing the commit message to
> > only say that this patch removes Warning C28159.
>
> How about this fixup instead?
>
> ---- 8< ----
> squash! poll: use GetTickCount64() to avoid wrap-around issues
>
> The value of timeout starts as an int value, and for this reason it
> cannot overflow unsigned long long aka ULONGLONG. The unsigned version
> of this initial value is available in orig_timeout. The difference
> (orig_timeout - elapsed) cannot wrap around because it is protected by
> a conditional (as can be seen in the patch text). Hence, the ULONGLONG
> difference can only have values that are smaller than the initial
> timeout value and truncation to int cannot overflow.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  compat/poll/poll.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> index 4abbfcb6a4..4459408c7d 100644
> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -452,7 +452,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>    static HANDLE hEvent;
>    WSANETWORKEVENTS ev;
>    HANDLE h, handle_array[FD_SETSIZE + 2];
> -  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
> +  DWORD ret, wait_timeout, nhandles, orig_timeout = 0;
>    ULONGLONG start = 0;
>    fd_set rfds, wfds, xfds;
>    BOOL poll_again;
> @@ -618,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>
>    if (!rc && orig_timeout && timeout != INFTIM)
>      {
> -      elapsed = (DWORD)(GetTickCount64() - start);
> -      timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
> +      ULONGLONG elapsed = GetTickCount64() - start;
> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
>      }
>
>    if (!rc && timeout)
> --
> 2.19.1.406.g1aa3f475f3
I like it. This still removes the warning and avoids overflow issues.
Steve
Junio C Hamano· Nov 3, 2018, 00:39 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Johannes Sixt <j6t@kdbg.org> writes:
>> Yep, correct on all counts. I'm in favor of changing the commit message to
>> only say that this patch removes Warning C28159.
>
> How about this fixup instead?
Isn't that already in 'next'?  I didn't check, though.
Junio C Hamano· Nov 3, 2018, 00:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> Yep, correct on all counts. I'm in favor of changing the commit message to
>>> only say that this patch removes Warning C28159.
>>
>> How about this fixup instead?
>
> Isn't that already in 'next'?  I didn't check, though.

Well, it turnsout that I already prepared one but not pushed it out yet. I'll eject this topic and rebuild the integration branches, and wait until Dscho says something, to avoid having to redo the integration cycle again.

Thanks.
Carlo Arenas· Nov 3, 2018, 08:14 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
nitpick: cast to DWORD instead of int
Carlo
Johannes Sixt· Nov 3, 2018, 14:05 UTC · re: Carlo Arenas · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Am 03.11.18 um 09:14 schrieb Carlo Arenas:
Show 5 quoted lines
> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt <j6t@kdbg.org> wrote:
>>
>> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
> 
> nitpick: cast to DWORD instead of int

No; timeout is of type int; after an explicit type cast we don't want to have another implicit conversion.

-- Hannes
Junio C Hamano· Nov 4, 2018, 23:26 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Johannes Sixt <j6t@kdbg.org> writes:
Show 11 quoted lines
> Am 03.11.18 um 09:14 schrieb Carlo Arenas:
>> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt <j6t@kdbg.org> wrote:
>>>
>>> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
>>
>> nitpick: cast to DWORD instead of int
>
> No; timeout is of type int; after an explicit type cast we don't want
> to have another implicit conversion.
>
> -- Hannes

OK, thanks. It seems that the relative silence after this message is a sign that the resulting patch after squashing is what everybody is happey with?

-- >8 --
From: Steve Hoelzer <shoelzer@gmail.com>
Date: Wed, 31 Oct 2018 14:11:36 -0700
Subject: [PATCH] poll: use GetTickCount64() to avoid wrap-around issues

The value of timeout starts as an int value, and for this reason it cannot overflow unsigned long long aka ULONGLONG. The unsigned version of this initial value is available in orig_timeout. The difference (orig_timeout - elapsed) cannot wrap around because it is protected by a conditional (as can be seen in the patch text). Hence, the ULONGLONG difference can only have values that are smaller than the initial timeout value and truncation to int cannot overflow.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Acked-by: Steve Hoelzer <shoelzer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/poll/poll.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Show changes to compat/poll/poll.c +8 −4
diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index ad5dcde439..4459408c7d 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -18,6 +18,9 @@
    You should have received a copy of the GNU General Public License along
    with this program; if not, see <http://www.gnu.org/licenses/>.  */
 
+/* To bump the minimum Windows version to Windows Vista */
+#include "git-compat-util.h"
+
 /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
 #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
 # pragma GCC diagnostic ignored "-Wtype-limits"
@@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   static HANDLE hEvent;
   WSANETWORKEVENTS ev;
   HANDLE h, handle_array[FD_SETSIZE + 2];
-  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
+  DWORD ret, wait_timeout, nhandles, orig_timeout = 0;
+  ULONGLONG start = 0;
   fd_set rfds, wfds, xfds;
   BOOL poll_again;
   MSG msg;
@@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   if (timeout != INFTIM)
     {
       orig_timeout = timeout;
-      start = GetTickCount();
+      start = GetTickCount64();
     }
 
   if (!hEvent)
@@ -614,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
 
   if (!rc && orig_timeout && timeout != INFTIM)
     {
-      elapsed = GetTickCount() - start;
-      timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
+      ULONGLONG elapsed = GetTickCount64() - start;
+      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
     }
 
   if (!rc && timeout)
-- 
2.19.1-816-gcd69ec8cde
Randall S. Becker· Nov 4, 2018, 23:44 UTC · re: Junio C Hamano · lore

RE: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

On November 4, 2018 6:26 PM, Junio C Hamano, wrote:
Show 16 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Am 03.11.18 um 09:14 schrieb Carlo Arenas:
> >> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt <j6t@kdbg.org> wrote:
> >>>
> >>> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout -
> >>> + elapsed);
> >>
> >> nitpick: cast to DWORD instead of int
> >
> > No; timeout is of type int; after an explicit type cast we don't want
> > to have another implicit conversion.
> >
> > -- Hannes
> 
> OK, thanks.  It seems that the relative silence after this message is a
sign that
> the resulting patch after squashing is what everybody is happey with?

On my platform (HPE NonStop), DWORD is being defined as unsigned int (32-bit) rather than unsigned long long (64 bit). The definition comes through the odbc/windows.h include, not the compiler or any core definition. It's only a nano-quibble (if even that), because GetTickCount64 is not defined on the platform anyway, so this is probably not a big deal.

Cheers, Randall

Eric Sunshine· Nov 5, 2018, 03:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

On Sun, Nov 4, 2018 at 6:26 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> OK, thanks.  It seems that the relative silence after this message
> is a sign that the resulting patch after squashing is what everybody
> is happey with?
>
> -- >8 --
> From: Steve Hoelzer <shoelzer@gmail.com>
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Acked-by: Steve Hoelzer <shoelzer@gmail.com>
It's not clear from this who the author is.
Junio C Hamano· Nov 5, 2018, 04:02 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 12 quoted lines
> On Sun, Nov 4, 2018 at 6:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>> OK, thanks.  It seems that the relative silence after this message
>> is a sign that the resulting patch after squashing is what everybody
>> is happey with?
>>
>> -- >8 --
>> From: Steve Hoelzer <shoelzer@gmail.com>
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> Acked-by: Steve Hoelzer <shoelzer@gmail.com>
>
> It's not clear from this who the author is.

Right. The latter should be s-o-b and the order swapped, and probably say "Steve wrote the original version, Johannes extended it" in the text to match.

THanks.
Johannes Sixt· Nov 5, 2018, 07:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Am 05.11.18 um 00:26 schrieb Junio C Hamano:
> OK, thanks.  It seems that the relative silence after this message
> is a sign that the resulting patch after squashing is what everybody
> is happey with?

I'm not 100% happy. I'll resend a squashed patch, but it has to wait as I have to catch a train now.

Appologies for the sub-optimal submission process.
-- Hannes
Johannes Sixt· Nov 5, 2018, 20:28 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Am 05.11.18 um 08:01 schrieb Johannes Sixt:
Show 6 quoted lines
> Am 05.11.18 um 00:26 schrieb Junio C Hamano:
>> OK, thanks.  It seems that the relative silence after this message
>> is a sign that the resulting patch after squashing is what everybody
>> is happey with?
> 
> I'm not 100% happy.

I see the patch is already in next. Never mind. The patch text is fine, I just wanted to modify the commit message a bit.

Thanks, -- Hannes

Johannes Schindelin· Nov 5, 2018, 22:05 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

Hi Hannes,
On Mon, 5 Nov 2018, Johannes Sixt wrote:
Show 7 quoted lines
> Am 05.11.18 um 00:26 schrieb Junio C Hamano:
> > OK, thanks.  It seems that the relative silence after this message is
> > a sign that the resulting patch after squashing is what everybody is
> > happey with?
> 
> I'm not 100% happy. I'll resend a squashed patch, but it has to wait as
> I have to catch a train now.
Thank you for running with this.

Ciao, Dscho

← back to recent threads