-
Notifications
You must be signed in to change notification settings - Fork 14.6k
Fix MSVC warning in benchmark #147357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix MSVC warning in benchmark #147357
Conversation
@llvm/pr-subscribers-third-party-benchmark Author: Daniel Paoliello (dpaoliello) ChangesBuilding LLVM with MSVC is raising the following warning:
This change resolves the warning by moving the Not sure how this code flows back into the original source. Full diff: https://github.com/llvm/llvm-project/pull/147357.diff 1 Files Affected:
diff --git a/third-party/benchmark/src/sysinfo.cc b/third-party/benchmark/src/sysinfo.cc
index 3993ae17f7fc4..837be8f9cf891 100644
--- a/third-party/benchmark/src/sysinfo.cc
+++ b/third-party/benchmark/src/sysinfo.cc
@@ -358,7 +358,6 @@ std::vector<CPUInfo::CacheInfo> GetCacheSizesWindows() {
C.num_sharing = static_cast<int>(b.count());
C.level = cache.Level;
C.size = cache.Size;
- C.type = "Unknown";
switch (cache.Type) {
case CacheUnified:
C.type = "Unified";
@@ -372,6 +371,9 @@ std::vector<CPUInfo::CacheInfo> GetCacheSizesWindows() {
case CacheTrace:
C.type = "Trace";
break;
+ case CacheUnknown:
+ C.type = "Unknown";
+ break;
}
res.push_back(C);
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I ran into this same issue.
Windows build bots are failing now for flang and clang |
Did this patch undergo Windows CI testing? |
I believe so: https://github.com/llvm/llvm-project/actions/runs/16124071447/job/45496681724 |
then I suspect some difference between the Windows machines doing the CI and those running the build bots, or a change to the build environments on those build bots. thanks for checking. |
The Clang MSVC build seems to be green: https://lab.llvm.org/buildbot/#/builders/63 |
Huh, that build is marked as succeeded, but it looks like nothing was actually built? @klausler What errors are you seeing? I'm guessing that your CI and my local machine have different versions of MSVC. In the meantime please revert the commit and I'll redo it. |
Filed #149154 to investigate the CI gap |
|
Ah, lovely, these values are defined in the Windows SDK, so depending on the version you're building with |
I have a fix, switched to using |
…info.cc (#149159) #147357 attempted to fix an MSVC in sysinfo.cc by adding a `case` block for a missing enum value. However, this resulted in [CI failures](llvm/llvm-project#147357 (comment)): ``` 4.170 [148/4/9] Building CXX object third-party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj FAILED: third-party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj C:\Users\tcwg\scoop\shims\ccache.exe C:\Users\tcwg\scoop\apps\llvm-arm64\current\bin\clang-cl.exe /nologo -TP -DBENCHMARK_STATIC_DEFINE -DEXPERIMENTAL_KEY_INSTRUCTIONS -DHAVE_STD_REGEX -DHAVE_STEADY_CLOCK -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/build/third-party/benchmark/src -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/third-party/benchmark/src -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/build/include -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/llvm/include -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/third-party/benchmark/include /DWIN32 /D_WINDOWS /Zc:inline /Zc:__cplusplus /Oi /Brepro /bigobj /permissive- -Werror=unguarded-availability-new -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported /Gw /W4 -EHs- -EHa- /O2 /Ob2 -std:c++14 -MD -UNDEBUG /showIncludes /Fothird-party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj /Fdthird-party\benchmark\src\CMakeFiles\benchmark.dir\benchmark.pdb -c -- C:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/third-party/benchmark/src/sysinfo.cc C:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/third-party/benchmark/src/sysinfo.cc(374,12): error: use of undeclared identifier 'CacheUnknown' 374 | case CacheUnknown: | ^ 1 error generated. ``` The root cause is that the enum being switched on is defined in the Windows SDK, so depending on which version of the SDK you are using `CacheUnknown` may or may not be defined. The correct fix here is to use a `default` block in the switch statement instead.
…ning in sysinfo.cc (#149159) #147357 attempted to fix an MSVC in sysinfo.cc by adding a `case` block for a missing enum value. However, this resulted in [CI failures](llvm/llvm-project#147357 (comment)): ``` 4.170 [148/4/9] Building CXX object third-party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj FAILED: third-party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj C:\Users\tcwg\scoop\shims\ccache.exe C:\Users\tcwg\scoop\apps\llvm-arm64\current\bin\clang-cl.exe /nologo -TP -DBENCHMARK_STATIC_DEFINE -DEXPERIMENTAL_KEY_INSTRUCTIONS -DHAVE_STD_REGEX -DHAVE_STEADY_CLOCK -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/build/third-party/benchmark/src -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/third-party/benchmark/src -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/build/include -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/llvm/include -IC:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/third-party/benchmark/include /DWIN32 /D_WINDOWS /Zc:inline /Zc:__cplusplus /Oi /Brepro /bigobj /permissive- -Werror=unguarded-availability-new -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported /Gw /W4 -EHs- -EHa- /O2 /Ob2 -std:c++14 -MD -UNDEBUG /showIncludes /Fothird-party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj /Fdthird-party\benchmark\src\CMakeFiles\benchmark.dir\benchmark.pdb -c -- C:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/third-party/benchmark/src/sysinfo.cc C:/Users/tcwg/llvm-worker/flang-arm64-windows-msvc/llvm-project/third-party/benchmark/src/sysinfo.cc(374,12): error: use of undeclared identifier 'CacheUnknown' 374 | case CacheUnknown: | ^ 1 error generated. ``` The root cause is that the enum being switched on is defined in the Windows SDK, so depending on which version of the SDK you are using `CacheUnknown` may or may not be defined. The correct fix here is to use a `default` block in the switch statement instead.
Building LLVM with MSVC is raising the following warning:
This change resolves the warning by moving the
Unknown
type into a case block forCacheUnknown
.Not sure how this code flows back into the original source.