google / benchmark

A microbenchmark support library
Apache License 2.0
9.06k stars 1.63k forks source link

Build Error w/ msvc-141 while targeting Universal Windows Platform #661

Open natxete88 opened 6 years ago

natxete88 commented 6 years ago

Summary

While attempting to build google/benchmark with cmake for a Windows UWP (Universal Windows Platform) target the following errors can be observed.

error

These symbols are disabled/undefined by VersionHelper.h:18 -- #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) Yet required in the sysinfo.cc:526 -- #elif defined BENCHMARK_OS_WINDOWS block.

Steps to reproduce

The above can be reproduced (so long as you have VS2017 or can compile with the MSVC v141 toolchain) with the following steps:

$ git clone https://github.com/google/benchmark.git
$ cd benchmark
$ mkdir -p build && cd build
$ cmake -G "Visual Studio 15" -DBENCHMARK_ENABLE_GTEST_TESTS=OFF -DCMAKE_BUILD_TYPE=RELEASE  -DCMAKE_SYSTEM_NAME=WindowsStore -DCMAKE_SYSTEM_VERSION=10.0 ..

Note: the CMAKE_SYSTEM_NAME/VERSION settings drive the cmake config step for UWP apps

Then open the .sln in build/ and run a build.

Fix

Alternative

The frequency estimation in the block in question is sampling/inferring the MHz of the system (presumably for performance measures). As this particular block is platform(Windows Specific) wouldn't usage of the QueryPerformanceFreq() API (windows.h) be more appropriate/accurate ?

Happy to try and submit a PR for this if our thinking on this is correct and acceptable.

dmah42 commented 6 years ago

I don't know much about windows development, but from reading the API it seems reasonable that this would be a better source of data than the registry.

I also don't know what the difference is between BENCHMARK_OS_WINDOWS, which checks for _WIN32, and the WINAPI_FAMILY_PARTITION macro. If the latter is available for all the windows versions we expect to support then using that to define BENCHMARK_OS_WINDOWS in src/internal_macros.h sounds fine.

pleroy commented 6 years ago
  1. The proposed fix would work, but it dodges the issue a bit. If you are targeting UWP, then you ought to have code for the other versions of UWP (app store, xbox, etc.). Replacing _WIN32 by WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) is fine for the Windows desktop but it is not going to work the next time a poor sap wants to target, say, the Metro API.

  2. Do we have any reason to believe that QueryPerformanceFreq() is related to the CPU frequency in any way? It's not obvious from the documentation. As far as I remember, the CPU frequency is only used in an informational message somewhere; it's not clear that the performance counter frequency would be more useful.

dmah42 commented 6 years ago

@pleroy Correct, it's only for information. I'm assuming that the registry might be less correct than the windows API call.

pleroy commented 6 years ago

I don't understand why the registry would be less correct. It's actually likely to be more correct if what you are after the CPU frequency: according to MSDN:

These APIs may make use of RDTSC, but might instead make use of a timing devices on the motherboard or some other system services that provide high-quality high-resolution timing information.

So QueryPerformanceFrequency may be the CPU frequency but then again it may be the frequency of some completely unrelated timing chip on your motherboard.

dmah42 commented 3 years ago

I'd like to see a PR that correctly introduces the WINAPI_FAMILY_PARTITION macros as a subset of the BENCHMARK_OS_WINDOWS platform definition so we can correctly handle it.