Skip to content

util: use 64 bit timer on Windows#5054

Merged
ethomson merged 3 commits intolibgit2:masterfrom
tniessen:util-use-64-bit-timer
Aug 23, 2019
Merged

util: use 64 bit timer on Windows#5054
ethomson merged 3 commits intolibgit2:masterfrom
tniessen:util-use-64-bit-timer

Conversation

@tniessen
Copy link
Copy Markdown
Contributor

git__timer was originally implemented using a 32 bit timer since Windows XP did not support GetTickCount64. Windows XP was discontinued five years ago, so it should be safe to use the new API.

As a benefit, we do not need to care about overflows for the next 585 million years.

@tniessen
Copy link
Copy Markdown
Contributor Author

I am not sure why one of the MinGW builds fails. This API was introduced more than 10 years ago and only requires _WIN32_WINNT >= 0x0600, so MinGW should support it.

@ethomson
Copy link
Copy Markdown
Member

I'm not either, that's pretty surprising. But IIRC mingw often makes use of its own headers, so it may simply be something that we need to declare explicitly on that platform.

@tniessen
Copy link
Copy Markdown
Contributor Author

I am not sure what the default _WIN32_WINNT value is in current MinGW releases, but you explicitely changed it to 0x0600 in b8bdffb last year, so it should work. I don't know why this specific platform does not seem to pick it up.

@tniessen
Copy link
Copy Markdown
Contributor Author

@ethomson Any ideas or updates on this?

@ethomson
Copy link
Copy Markdown
Member

No, sorry, I'm not very familiar with mingw.

tniessen added 2 commits July 29, 2019 18:06
git__timer was originally implemented using a 32 bit timer since
Windows XP did not support GetTickCount64. Windows XP was discontinued
five years ago, so it should be safe to use the new API.

As a benefit, we do not need to care about overflows for the next 585
million years.
@tniessen
Copy link
Copy Markdown
Contributor Author

Took me a while to set up MinGW locally and figure this out. The problem is that git__timer is an inline function which is also compiled in test/ and fuzzers/, which don't explicitly set _WIN32_WINNT.

This should be good to go now.

Comment thread fuzzers/CMakeLists.txt Outdated
@@ -1,3 +1,8 @@
# Ensure that MinGW provides the correct header files.
IF (WIN32 AND NOT CYGWIN)
ADD_DEFINITIONS(-DWIN32 -D_WIN32_WINNT=0x0600)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just move this into the top-level CMakeLists.txt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor difference is that this is now tucked inside an ELSE block so it will only execute if we're not building with MSVC.

I think that we want to continue setting this value on all platforms, when we're using MSVC, MinGW or Cygwin... Does that sound correct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it out of the ELSE block for MSVC (not sure if it makes a difference on MSVC). As before, _WIN32_WINNT is not set on Cygwin, I believe Cygwin doesn't provide Windows header files anyway.

@ethomson
Copy link
Copy Markdown
Member

Thanks for all the work on this @tniessen! 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants