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

[Steve Hoelzer <shoelzer@gmail.com>]

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:
> > 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