Raise MSVC warning level from /W3 to /W4

kbenzie · kbenzie · commit 6a0b9b3c2f6e · 2024-09-24T11:57:08.000+01:00
This patch increases the warning level when using the MSVC compiler from
`/W3` to `/W4` and fixes the issues found. Four warnings introduced by
`/W4` are disabled, all related to variable name shadowing, as they
overly prescriptive to valid code.
diff --git a/cmake/helpers.cmake b/cmake/helpers.cmake
@@ -82,7 +82,11 @@ function(add_ur_target_compile_options name)
     elseif(MSVC)
         target_compile_options(${name} PRIVATE
             $<$<CXX_COMPILER_ID:MSVC>:/MP>  # clang-cl.exe does not support /MP
-            /W3
+            /W4
+            /wd4456  # Disable: declaration of 'identifier' hides previous local declaration
+            /wd4457  # Disable: declaration of 'identifier' hides function parameter
+            /wd4458  # Disable: declaration of 'identifier' hides class member
+            /wd4459  # Disable: declaration of 'identifier' hides global declaration
             /MD$<$<CONFIG:Debug>:d>
             /GS
             /DWIN32_LEAN_AND_MEAN
diff --git a/examples/collector/collector.cpp b/examples/collector/collector.cpp
@@ -25,7 +25,14 @@
 #include <string_view>
 
 #include "ur_api.h"
+
+#ifdef _MSC_VER
+#pragma warning(disable : 4245)
+#endif
 #include "xpti/xpti_trace_framework.h"
+#ifdef _MSC_VER
+#pragma warning(default : 4245)
+#endif
 
 constexpr uint16_t TRACE_FN_BEGIN =
     static_cast<uint16_t>(xpti::trace_point_type_t::function_with_args_begin);
diff --git a/include/ur_api.h b/include/ur_api.h
@@ -422,7 +422,7 @@ typedef struct ur_physical_mem_handle_t_ *ur_physical_mem_handle_t;
 ///////////////////////////////////////////////////////////////////////////////
 #ifndef UR_BIT
 /// @brief Generic macro for enumerator bit masks
-#define UR_BIT(_i) (1 << _i)
+#define UR_BIT(_i) (1U << _i)
 #endif // UR_BIT
 
 ///////////////////////////////////////////////////////////////////////////////
diff --git a/scripts/core/common.yml b/scripts/core/common.yml
@@ -134,7 +134,7 @@ name: "$x_physical_mem_handle_t"
 type: macro
 desc: "Generic macro for enumerator bit masks"
 name: "$X_BIT( _i )"
-value: "( 1 << _i )"
+value: "( 1U << _i )"
 --- #--------------------------------------------------------------------------
 type: enum
 desc: "Defines Return/Error codes"
diff --git a/source/adapters/level_zero/CMakeLists.txt b/source/adapters/level_zero/CMakeLists.txt
@@ -65,7 +65,7 @@ if (NOT DEFINED LEVEL_ZERO_LIBRARY OR NOT DEFINED LEVEL_ZERO_INCLUDE_DIR)
 
     target_compile_options(ze_loader PRIVATE
         $<$<IN_LIST:$<CXX_COMPILER_ID>,GNU;Clang;Intel;IntelLLVM>:-Wno-error>
-        $<$<CXX_COMPILER_ID:MSVC>:/WX- /UUNICODE>
+        $<$<CXX_COMPILER_ID:MSVC>:/UUNICODE>
     )
 
     set(LEVEL_ZERO_LIBRARY ze_loader)
@@ -256,7 +256,7 @@ if(UR_BUILD_ADAPTER_L0_V2)
 
     # TODO: fix level_zero adapter conversion warnings
     target_compile_options(ur_adapter_level_zero_v2 PRIVATE
-        $<$<CXX_COMPILER_ID:MSVC>:/wd4805 /wd4244>
+        $<$<CXX_COMPILER_ID:MSVC>:/wd4805 /wd4244 /wd4100>
     )
 
     set_target_properties(ur_adapter_level_zero_v2 PROPERTIES
diff --git a/source/adapters/level_zero/context.cpp b/source/adapters/level_zero/context.cpp
@@ -756,7 +756,7 @@ ur_result_t ur_context_handle_t_::getAvailableCommandList(
         // queue's map to hold the fence and other associated command
         // list information.
         auto &QGroup = Queue->getQueueGroup(UseCopyEngine);
-        uint32_t QueueGroupOrdinal;
+        uint32_t QueueGroupOrdinal = 0;
         auto &ZeCommandQueue = ForcedCmdQueue
                                    ? *ForcedCmdQueue
                                    : QGroup.getZeQueue(&QueueGroupOrdinal);
diff --git a/source/adapters/level_zero/queue.cpp b/source/adapters/level_zero/queue.cpp
@@ -2263,7 +2263,7 @@ ur_result_t ur_queue_handle_t_::createCommandList(
   ZeStruct<ze_fence_desc_t> ZeFenceDesc;
   ze_command_list_handle_t ZeCommandList;
 
-  uint32_t QueueGroupOrdinal;
+  uint32_t QueueGroupOrdinal = 0;
   auto &QGroup = getQueueGroup(UseCopyEngine);
   auto &ZeCommandQueue =
       ForcedCmdQueue ? *ForcedCmdQueue : QGroup.getZeQueue(&QueueGroupOrdinal);
diff --git a/source/adapters/opencl/adapter.cpp b/source/adapters/opencl/adapter.cpp
@@ -98,6 +98,4 @@ UR_APIEXPORT ur_result_t UR_APICALL urAdapterGetInfo(ur_adapter_handle_t,
   default:
     return UR_RESULT_ERROR_INVALID_ENUMERATION;
   }
-
-  return UR_RESULT_SUCCESS;
 }
diff --git a/source/adapters/opencl/event.cpp b/source/adapters/opencl/event.cpp
@@ -10,6 +10,7 @@
 
 #include "common.hpp"
 
+#include <limits>
 #include <mutex>
 #include <set>
 #include <unordered_map>
@@ -32,8 +33,7 @@ cl_event_info convertUREventInfoToCL(const ur_event_info_t PropName) {
     return CL_EVENT_REFERENCE_COUNT;
     break;
   default:
-    return -1;
-    break;
+    return std::numeric_limits<cl_event_info>::max();
   }
 }
 
@@ -51,7 +51,7 @@ convertURProfilingInfoToCL(const ur_profiling_info_t PropName) {
   case UR_PROFILING_INFO_COMMAND_END:
     return CL_PROFILING_COMMAND_END;
   default:
-    return -1;
+    return std::numeric_limits<cl_profiling_info>::max();
   }
 }
 
diff --git a/source/adapters/opencl/memory.cpp b/source/adapters/opencl/memory.cpp
@@ -9,6 +9,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "common.hpp"
+#include <limits>
 
 cl_image_format mapURImageFormatToCL(const ur_image_format_t *PImageFormat) {
   cl_image_format CLImageFormat;
@@ -59,7 +60,8 @@ cl_image_format mapURImageFormatToCL(const ur_image_format_t *PImageFormat) {
     CLImageFormat.image_channel_order = CL_sRGBA;
     break;
   default:
-    CLImageFormat.image_channel_order = -1;
+    CLImageFormat.image_channel_order =
+        std::numeric_limits<cl_channel_order>::max();
     break;
   }
 
@@ -110,7 +112,8 @@ cl_image_format mapURImageFormatToCL(const ur_image_format_t *PImageFormat) {
     CLImageFormat.image_channel_data_type = CL_FLOAT;
     break;
   default:
-    CLImageFormat.image_channel_data_type = -1;
+    CLImageFormat.image_channel_data_type =
+        std::numeric_limits<cl_channel_type>::max();
     break;
   }
 
@@ -139,7 +142,7 @@ cl_image_desc mapURImageDescToCL(const ur_image_desc_t *PImageDesc) {
     CLImageDesc.image_type = CL_MEM_OBJECT_IMAGE1D_ARRAY;
     break;
   default:
-    CLImageDesc.image_type = -1;
+    CLImageDesc.image_type = std::numeric_limits<cl_mem_object_type>::max();
     break;
   }
 
diff --git a/source/adapters/opencl/queue.cpp b/source/adapters/opencl/queue.cpp
@@ -8,9 +8,9 @@
 
 #include "common.hpp"
 #include "platform.hpp"
+#include <limits>
 
 cl_command_queue_info mapURQueueInfoToCL(const ur_queue_info_t PropName) {
-
   switch (PropName) {
   case UR_QUEUE_INFO_CONTEXT:
     return CL_QUEUE_CONTEXT;
@@ -25,7 +25,7 @@ cl_command_queue_info mapURQueueInfoToCL(const ur_queue_info_t PropName) {
   case UR_QUEUE_INFO_SIZE:
     return CL_QUEUE_SIZE;
   default:
-    return -1;
+    return std::numeric_limits<cl_command_queue_info>::max();
   }
 }
 
diff --git a/source/adapters/opencl/usm_p2p.cpp b/source/adapters/opencl/usm_p2p.cpp
@@ -8,22 +8,20 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "common.hpp"
+#include "logger/ur_logger.hpp"
 
 UR_APIEXPORT ur_result_t UR_APICALL
 urUsmP2PEnablePeerAccessExp([[maybe_unused]] ur_device_handle_t commandDevice,
                             [[maybe_unused]] ur_device_handle_t peerDevice) {
-
-  cl_adapter::die(
+  logger::warning(
       "Experimental P2P feature is not implemented for OpenCL adapter.");
   return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }
 
 UR_APIEXPORT ur_result_t UR_APICALL
 urUsmP2PDisablePeerAccessExp([[maybe_unused]] ur_device_handle_t commandDevice,
                              [[maybe_unused]] ur_device_handle_t peerDevice) {
-
-  cl_adapter::die(
+  logger::warning(
       "Experimental P2P feature is not implemented for OpenCL adapter.");
   return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }
@@ -34,8 +32,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urUsmP2PPeerAccessGetInfoExp(
     [[maybe_unused]] ur_exp_peer_info_t propName,
     [[maybe_unused]] size_t propSize, [[maybe_unused]] void *pPropValue,
     [[maybe_unused]] size_t *pPropSizeRet) {
-
-  cl_adapter::die(
+  logger::warning(
       "Experimental P2P feature is not implemented for OpenCL adapter.");
   return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }
diff --git a/source/common/logger/ur_logger.hpp b/source/common/logger/ur_logger.hpp
@@ -116,8 +116,9 @@ template <typename T> inline std::string toHex(T t) {
 inline Logger create_logger(std::string logger_name, bool skip_prefix,
                             bool skip_linebreak,
                             logger::Level default_log_level) {
-    std::transform(logger_name.begin(), logger_name.end(), logger_name.begin(),
-                   ::toupper);
+    std::transform(
+        logger_name.begin(), logger_name.end(), logger_name.begin(),
+        [](char c) -> char { return static_cast<char>(::toupper(c)); });
     std::stringstream env_var_name;
     const auto default_flush_level = logger::Level::ERR;
     const std::string default_output = "stderr";
diff --git a/source/common/ur_util.cpp b/source/common/ur_util.cpp
@@ -15,7 +15,7 @@
 #include <windows.h>
 int ur_getpid(void) { return static_cast<int>(GetCurrentProcessId()); }
 
-int ur_close_fd(int fd) { return -1; }
+int ur_close_fd(int fd [[maybe_unused]]) { return -1; }
 
 int ur_duplicate_fd(int pid, int fd_in) {
     // TODO: find another way to obtain a duplicate of another process's file descriptor
diff --git a/source/common/ur_util.hpp b/source/common/ur_util.hpp
@@ -98,8 +98,9 @@ std::optional<std::string> ur_getenv(const char *name);
 
 inline bool getenv_tobool(const char *name, bool def = false) {
     if (auto env = ur_getenv(name); env) {
-        std::transform(env->begin(), env->end(), env->begin(),
-                       [](unsigned char c) { return std::tolower(c); });
+        std::transform(env->begin(), env->end(), env->begin(), [](char c) {
+            return static_cast<char>(std::tolower(c));
+        });
         auto true_str = {"y", "yes", "t", "true", "1"};
         return std::find(true_str.begin(), true_str.end(), *env) !=
                true_str.end();
diff --git a/source/loader/layers/tracing/ur_tracing_layer.cpp b/source/loader/layers/tracing/ur_tracing_layer.cpp
@@ -12,8 +12,16 @@
 #include "ur_tracing_layer.hpp"
 #include "ur_api.h"
 #include "ur_util.hpp"
+
+#ifdef _MSC_VER
+#pragma warning(disable : 4245)
+#endif
 #include "xpti/xpti_data_types.h"
 #include "xpti/xpti_trace_framework.h"
+#ifdef _MSC_VER
+#pragma warning(default : 4245)
+#endif
+
 #include <atomic>
 #include <optional>
 #include <sstream>
diff --git a/source/loader/ur_adapter_registry.hpp b/source/loader/ur_adapter_registry.hpp
@@ -225,12 +225,13 @@ class AdapterRegistry {
             }
 
             // case-insensitive comparison by converting both tolower
-            std::transform(platformBackendName.begin(),
-                           platformBackendName.end(),
-                           platformBackendName.begin(),
-                           [](unsigned char c) { return std::tolower(c); });
-            std::transform(backend.begin(), backend.end(), backend.begin(),
-                           [](unsigned char c) { return std::tolower(c); });
+            std::transform(
+                platformBackendName.begin(), platformBackendName.end(),
+                platformBackendName.begin(),
+                [](char c) { return static_cast<char>(std::tolower(c)); });
+            std::transform(
+                backend.begin(), backend.end(), backend.begin(),
+                [](char c) { return static_cast<char>(std::tolower(c)); });
             std::size_t nameFound = platformBackendName.find(backend);
 
             bool backendFound = nameFound != std::string::npos;
diff --git a/source/loader/ur_lib.cpp b/source/loader/ur_lib.cpp
@@ -17,6 +17,7 @@
 #define NOMINMAX
 #include "ur_api.h"
 #include "ur_ldrddi.hpp"
+#include <limits>
 #endif // !NOMINMAX
 
 #include "logger/ur_logger.hpp"
@@ -413,7 +414,7 @@ ur_result_t urDeviceGetSelected(ur_platform_handle_t hPlatform,
 
     using DeviceIdType = unsigned long;
     constexpr DeviceIdType DeviceIdTypeALL =
-        -1; // ULONG_MAX but without #include <climits>
+        std::numeric_limits<DeviceIdType>::max();
 
     struct DeviceSpec {
         DevicePartLevel level;
@@ -427,8 +428,9 @@ ur_result_t urDeviceGetSelected(ur_platform_handle_t hPlatform,
     auto getRootHardwareType =
         [](const std::string &input) -> DeviceHardwareType {
         std::string lowerInput(input);
-        std::transform(lowerInput.cbegin(), lowerInput.cend(),
-                       lowerInput.begin(), ::tolower);
+        std::transform(
+            lowerInput.cbegin(), lowerInput.cend(), lowerInput.begin(),
+            [](char c) { return static_cast<char>(std::tolower(c)); });
         if (lowerInput == "cpu") {
             return ::UR_DEVICE_TYPE_CPU;
         }
@@ -483,9 +485,8 @@ ur_result_t urDeviceGetSelected(ur_platform_handle_t hPlatform,
                         platformBackendName.cend(), backend.cbegin(),
                         backend.cend(), [](const auto &a, const auto &b) {
                             // case-insensitive comparison by converting both tolower
-                            return std::tolower(
-                                       static_cast<unsigned char>(a)) ==
-                                   std::tolower(static_cast<unsigned char>(b));
+                            return std::tolower(static_cast<char>(a)) ==
+                                   std::tolower(static_cast<char>(b));
                         })) {
             // irrelevant term for current request: different backend -- silently ignore
             logger::error("unrecognised backend '{}'", backend);
diff --git a/source/loader/windows/adapter_search.cpp b/source/loader/windows/adapter_search.cpp
@@ -40,7 +40,8 @@ std::optional<fs::path> getLoaderLibPath() {
     return std::nullopt;
 }
 
-std::optional<fs::path> getAdapterNameAsPath(std::string adapterName) {
+std::optional<fs::path> getAdapterNameAsPath(std::string adapterName
+                                             [[maybe_unused]]) {
     return std::nullopt;
 }
 
diff --git a/test/conformance/device/urDevicePartition.cpp b/test/conformance/device/urDevicePartition.cpp
@@ -138,7 +138,7 @@ TEST_F(urDevicePartitionTest, PartitionByCounts) {
             uint32_t sum = 0;
             for (auto sub_device : sub_devices) {
                 ASSERT_NE(sub_device, nullptr);
-                uint32_t n_cu_in_sub_device;
+                uint32_t n_cu_in_sub_device = 0;
                 ASSERT_NO_FATAL_FAILURE(
                     getNumberComputeUnits(sub_device, n_cu_in_sub_device));
                 sum += n_cu_in_sub_device;
diff --git a/test/conformance/exp_command_buffer/buffer_fill_kernel_update.cpp b/test/conformance/exp_command_buffer/buffer_fill_kernel_update.cpp
@@ -329,7 +329,7 @@ TEST_P(BufferFillCommandTest, OverrideUpdate) {
     ASSERT_SUCCESS(urCommandBufferUpdateKernelLaunchExp(command_handle,
                                                         &first_update_desc));
 
-    uint32_t second_val = -99;
+    uint32_t second_val = 0xffffff9d; // -99
     ur_exp_command_buffer_update_value_arg_desc_t second_input_desc = {
         UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_UPDATE_VALUE_ARG_DESC, // stype
         nullptr,                                                    // pNext
@@ -385,7 +385,7 @@ TEST_P(BufferFillCommandTest, OverrideArgList) {
         &first_val, // hArgValue
     };
 
-    uint32_t second_val = -99;
+    uint32_t second_val = 0xffffff9d; // -99
     input_descs[1] = {
         UR_STRUCTURE_TYPE_EXP_COMMAND_BUFFER_UPDATE_VALUE_ARG_DESC, // stype
         nullptr,                                                    // pNext
diff --git a/test/conformance/integration/QueueBuffer.cpp b/test/conformance/integration/QueueBuffer.cpp
@@ -47,8 +47,8 @@ TEST_P(QueueBufferTestWithParam, QueueBufferTest) {
     std::vector<ur_event_handle_t> EventsFill;
     ur_event_handle_t Event;
 
-    size_t Buffer1Index;
-    size_t Buffer2Index;
+    size_t Buffer1Index = 0;
+    size_t Buffer2Index = 0;
     ASSERT_NO_FATAL_FAILURE(
         AddBuffer1DArg(ArraySize * sizeof(uint32_t), &Buffer1, &Buffer1Index));
     ASSERT_NO_FATAL_FAILURE(
diff --git a/test/conformance/testing/include/uur/utils.h b/test/conformance/testing/include/uur/utils.h
@@ -176,8 +176,6 @@ ur_result_t GetObjectReferenceCount(T object, uint32_t &out_ref_count) {
             object, UR_EXP_COMMAND_BUFFER_COMMAND_INFO_REFERENCE_COUNT,
             out_ref_count);
     }
-
-    return UR_RESULT_ERROR_INVALID_VALUE;
 }
 
 inline std::string GetPlatformName(ur_platform_handle_t hPlatform) {
diff --git a/test/conformance/usm/urUSMDeviceAlloc.cpp b/test/conformance/usm/urUSMDeviceAlloc.cpp
@@ -110,9 +110,10 @@ TEST_P(urUSMDeviceAllocTest, InvalidNullPtrResult) {
 
 TEST_P(urUSMDeviceAllocTest, InvalidUSMSize) {
     void *ptr = nullptr;
-    ASSERT_EQ_RESULT(
-        UR_RESULT_ERROR_INVALID_USM_SIZE,
-        urUSMDeviceAlloc(context, device, nullptr, pool, -1, &ptr));
+    ASSERT_EQ_RESULT(UR_RESULT_ERROR_INVALID_USM_SIZE,
+                     urUSMDeviceAlloc(context, device, nullptr, pool,
+                                      std::numeric_limits<size_t>::max(),
+                                      &ptr));
 }
 
 TEST_P(urUSMDeviceAllocTest, InvalidValueAlignPowerOfTwo) {
diff --git a/test/conformance/usm/urUSMHostAlloc.cpp b/test/conformance/usm/urUSMHostAlloc.cpp
diff --git a/test/conformance/usm/urUSMSharedAlloc.cpp b/test/conformance/usm/urUSMSharedAlloc.cpp
diff --git a/test/layers/tracing/test_collector.cpp b/test/layers/tracing/test_collector.cpp
diff --git a/tools/urinfo/utils.hpp b/tools/urinfo/utils.hpp
diff --git a/tools/urtrace/collector.cpp b/tools/urtrace/collector.cpp

Original file line number	Diff line number	Diff line change
`@@ -65,7 +65,7 @@ if (NOT DEFINED LEVEL_ZERO_LIBRARY OR NOT DEFINED LEVEL_ZERO_INCLUDE_DIR)`
`65`	`65`
`66`	`66`	`target_compile_options(ze_loader PRIVATE`
`67`	`67`	`$<$<IN_LIST:$<CXX_COMPILER_ID>,GNU;Clang;Intel;IntelLLVM>:-Wno-error>`
`68`		`- $<$<CXX_COMPILER_ID:MSVC>:/WX- /UUNICODE>`
	`68`	`+ $<$<CXX_COMPILER_ID:MSVC>:/UUNICODE>`
`69`	`69`	`)`
`70`	`70`
`71`	`71`	`set(LEVEL_ZERO_LIBRARY ze_loader)`
`@@ -256,7 +256,7 @@ if(UR_BUILD_ADAPTER_L0_V2)`
`256`	`256`
`257`	`257`	`# TODO: fix level_zero adapter conversion warnings`
`258`	`258`	`target_compile_options(ur_adapter_level_zero_v2 PRIVATE`
`259`		`- $<$<CXX_COMPILER_ID:MSVC>:/wd4805 /wd4244>`
	`259`	`+ $<$<CXX_COMPILER_ID:MSVC>:/wd4805 /wd4244 /wd4100>`
`260`	`260`	`)`
`261`	`261`
`262`	`262`	`set_target_properties(ur_adapter_level_zero_v2 PROPERTIES`
Original file line number	Diff line number	Diff line change
`@@ -98,6 +98,4 @@ UR_APIEXPORT ur_result_t UR_APICALL urAdapterGetInfo(ur_adapter_handle_t,`
`98`	`98`	`default:`
`99`	`99`	`return UR_RESULT_ERROR_INVALID_ENUMERATION;`
`100`	`100`	`}`
`101`		`-`
`102`		`- return UR_RESULT_SUCCESS;`
`103`	`101`	`}`
Original file line number	Diff line number	Diff line change
`@@ -10,6 +10,7 @@`
`10`	`10`
`11`	`11`	`#include "common.hpp"`
`12`	`12`
	`13`	`+#include <limits>`
`13`	`14`	`#include <mutex>`
`14`	`15`	`#include <set>`
`15`	`16`	`#include <unordered_map>`
`@@ -32,8 +33,7 @@ cl_event_info convertUREventInfoToCL(const ur_event_info_t PropName) {`
`32`	`33`	`return CL_EVENT_REFERENCE_COUNT;`
`33`	`34`	`break;`
`34`	`35`	`default:`
`35`		`- return -1;`
`36`		`- break;`
	`36`	`+ return std::numeric_limits<cl_event_info>::max();`
`37`	`37`	`}`
`38`	`38`	`}`
`39`	`39`
`@@ -51,7 +51,7 @@ convertURProfilingInfoToCL(const ur_profiling_info_t PropName) {`
`51`	`51`	`case UR_PROFILING_INFO_COMMAND_END:`
`52`	`52`	`return CL_PROFILING_COMMAND_END;`
`53`	`53`	`default:`
`54`		`- return -1;`
	`54`	`+ return std::numeric_limits<cl_profiling_info>::max();`
`55`	`55`	`}`
`56`	`56`	`}`
`57`	`57`