Fix warnings in building libDPPLSyclInterface (#155)

diptorupd · web-flow · commit 2010cbb229e6 · 2020-10-29T09:38:15.000-05:00
This PR fixes all the currently reported warnings for the libDPPLSyclInterface and suppresses tp_print related warnings when building the Cython module.

* Enable warning flags to CXX_FLAGS.

* Add __attribute__((unused)) to wrap/unwrap functions.

    -- Wrap/unwrap functions are imported using the same macro and
       both versions are not necessarily used in the same file.
       Adding this attribute prevents a flood of warning.

* Remove unused variables.

* Initialize before use.

* Fix signed comparison warnings.

* Some formatting changes.

* fix warnings about comparing signed with unsigned

* Remove option not known to MSVC.

* Silence strncpy deprecation warning on Windows.

* Suppress tp_print deprecation warning flood on Linux.

* Update changelog.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,9 @@ All notable changes to this project will be documented in this file.
 - Device descriptors "max_compute_units", "max_work_item_dimensions", "max_work_item_sizes", "max_work_group_size", "max_num_sub_groups" and "aspects" for int64 atomics inside dpctl C API and inside the dpctl.SyclDevice class.
 - MemoryUSM* classes moved to `dpctl.memory` module, added support for aligned allocation, added support for `prefetch` and `mem_advise` (sychronous) methods, implemented `copy_to_host`, `copy_from_host` and `copy_from_device` methods, pickling support, and zero-copy interoperability with Python objects which implement `__sycl_usm_array_inerface__` protocol.
 
+### Fixed
+- Compiler warnings when building libDPPLSyclInterface and the Cython extensions.
+
 ### Removed
 - The Legacy OpenCL interface.
 
diff --git a/backends/CMakeLists.txt b/backends/CMakeLists.txt
@@ -68,18 +68,18 @@ if(WIN32)
     message(STATUS "Resetting CXX compiler to: " ${CMAKE_CXX_COMPILER})
     message(STATUS "Resetting C compiler to: " ${CMAKE_C_COMPILER})
     message(STATUS "Resetting Linker to: " ${CMAKE_LINK})
-
-    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}                                        \
-       -Wall -Wextra -Winit-self -Wuninitialized -Wmissing-declarations        \
-       -fdiagnostics-color=auto -O3                                            \
-    ")
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Qstd=c++17")
-    set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -ggdb3 -DDEBUG ")
+    set(WARNING_FLAGS "-Wall -Wextra -Winit-self -Wunused-function -Wuninitialized -Wmissing-declarations")
+    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${WARNING_FLAGS}")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${WARNING_FLAGS} -Qstd=c++17")
+    set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} ${WARNING_FLAGS} -ggdb3 -DDEBUG")
+    set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} ${WARNING_FLAGS} -ggdb3 -DDEBUG -Qstd=c++17")
 elseif(UNIX)
     set(SDL_FLAGS "-fstack-protector -fstack-protector-all -fpic -fPIC -D_FORTIFY_SOURCE=2 -Wformat -Wformat-security -fno-strict-overflow -fno-delete-null-pointer-checks")
-    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${SDL_FLAGS} -Wall -Wextra -Winit-self -Wuninitialized -Wmissing-declarations -fdiagnostics-color=auto")
-    set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -ggdb3 -DDEBUG ")
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SDL_FLAGS} -std=c++17 -fsycl")
+    set(WARNING_FLAGS "-Wall -Wextra -Winit-self -Wunused-function -Wuninitialized -Wmissing-declarations -fdiagnostics-color=auto")
+    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${WARNING_FLAGS} ${SDL_FLAGS}")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${WARNING_FLAGS} ${SDL_FLAGS} -std=c++17 -fsycl")
+    set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} ${WARNING_FLAGS} -ggdb3 -DDEBUG")
+    set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} ${WARNING_FLAGS} -ggdb3 -DDEBUG -std=c++17 -fsycl")
 else()
     message(FATAL_ERROR "Unsupported system.")
 endif()
diff --git a/backends/include/Support/CBindingWrapping.h b/backends/include/Support/CBindingWrapping.h
@@ -23,23 +23,26 @@
 ///
 //===----------------------------------------------------------------------===//
 
- #pragma once
+#pragma once
 
- #define DEFINE_SIMPLE_CONVERSION_FUNCTIONS(ty, ref)     \
-   inline ty *unwrap(ref P) {                            \
-     return reinterpret_cast<ty*>(P);                    \
-   }                                                     \
-                                                         \
-   inline ref wrap(const ty *P) {                        \
-     return reinterpret_cast<ref>(const_cast<ty*>(P));   \
-   }
+#define DEFINE_SIMPLE_CONVERSION_FUNCTIONS(ty, ref)                            \
+    __attribute__((unused)) inline ty *unwrap(ref P)                           \
+    {                                                                          \
+        return reinterpret_cast<ty*>(P);                                       \
+    }                                                                          \
+                                                                               \
+    __attribute__((unused)) inline ref wrap(const ty *P)                       \
+    {                                                                          \
+            return reinterpret_cast<ref>(const_cast<ty*>(P));                  \
+    }
 
- #define DEFINE_STDCXX_CONVERSION_FUNCTIONS(ty, ref)     \
-   DEFINE_SIMPLE_CONVERSION_FUNCTIONS(ty, ref)           \
-                                                         \
-   template<typename T>                                  \
-   inline T *unwrap(ref P) {                             \
-     T *Q = (T*)unwrap(P);                               \
-     assert(Q && "Invalid cast!");                       \
-     return Q;                                           \
-   }
+ #define DEFINE_STDCXX_CONVERSION_FUNCTIONS(ty, ref)                           \
+    DEFINE_SIMPLE_CONVERSION_FUNCTIONS(ty, ref)                                \
+                                                                               \
+    template<typename T>                                                       \
+    __attribute__((unused)) inline T *unwrap(ref P)                            \
+    {                                                                          \
+        T *Q = (T*)unwrap(P);                                                  \
+        assert(Q && "Invalid cast!");                                          \
+        return Q;                                                              \
+    }
diff --git a/backends/source/dppl_sycl_device_interface.cpp b/backends/source/dppl_sycl_device_interface.cpp
@@ -28,6 +28,7 @@
 #include "Support/CBindingWrapping.h"
 #include <iomanip>
 #include <iostream>
+#include <cstring>
 #include <CL/sycl.hpp>                /* SYCL headers   */
 
 using namespace cl::sycl;
@@ -220,8 +221,13 @@ DPPLDevice_GetName (__dppl_keep const DPPLSyclDeviceRef DRef)
     auto D = unwrap(DRef);
     if (D) {
         auto name = D->get_info<info::device::name>();
-        auto cstr_name = new char [name.length()+1];
-        std::strcpy (cstr_name, name.c_str());
+        auto cstr_len = name.length()+1;
+        auto cstr_name = new char[cstr_len];
+#ifdef _WIN32
+        strncpy_s(cstr_name, cstr_len, name.c_str(), cstr_len);
+#else
+        std::strncpy(cstr_name, name.c_str(), cstr_len);
+#endif
         return cstr_name;
     }
     return nullptr;
@@ -233,8 +239,13 @@ DPPLDevice_GetVendorName (__dppl_keep const DPPLSyclDeviceRef DRef)
     auto D = unwrap(DRef);
     if (D) {
         auto vendor = D->get_info<info::device::vendor>();
-        auto cstr_vendor = new char [vendor.length()+1];
-        std::strcpy (cstr_vendor, vendor.c_str());
+        auto cstr_len = vendor.length()+1;
+        auto cstr_vendor = new char[cstr_len];
+#ifdef _WIN32
+        strncpy_s(cstr_vendor, cstr_len, vendor.c_str(), cstr_len);
+#else
+        std::strncpy(cstr_vendor, vendor.c_str(), cstr_len);
+#endif
         return cstr_vendor;
     }
     return nullptr;
@@ -246,8 +257,13 @@ DPPLDevice_GetDriverInfo (__dppl_keep const DPPLSyclDeviceRef DRef)
     auto D = unwrap(DRef);
     if (D) {
         auto driver = D->get_info<info::device::driver_version>();
-        auto cstr_driver = new char [driver.length()+1];
-        std::strcpy (cstr_driver, driver.c_str());
+        auto cstr_len = driver.length()+1;
+        auto cstr_driver = new char[cstr_len];
+#ifdef _WIN32
+        strncpy_s(cstr_driver, cstr_len, driver.c_str(), cstr_len);
+#else
+        std::strncpy(cstr_driver, driver.c_str(), cstr_len);
+#endif
         return cstr_driver;
     }
     return nullptr;
diff --git a/backends/source/dppl_sycl_kernel_interface.cpp b/backends/source/dppl_sycl_kernel_interface.cpp
@@ -50,8 +50,13 @@ DPPLKernel_GetFunctionName (__dppl_keep const DPPLSyclKernelRef Kernel)
     auto kernel_name = SyclKernel->get_info<info::kernel::function_name>();
     if(kernel_name.empty())
         return nullptr;
-    auto cstr_name = new char [kernel_name.length()+1];
-    std::strcpy (cstr_name, kernel_name.c_str());
+    auto cstr_len = kernel_name.length()+1;
+    auto cstr_name = new char[cstr_len];
+#ifdef _WIN32
+    strncpy_s(cstr_name, cstr_len, kernel_name.c_str(), cstr_len);
+#else
+    std::strncpy (cstr_name, kernel_name.c_str(), cstr_len);
+#endif
     return cstr_name;
 }
 
diff --git a/backends/source/dppl_sycl_program_interface.cpp b/backends/source/dppl_sycl_program_interface.cpp
@@ -96,7 +96,6 @@ DPPLProgram_CreateFromOCLSource (__dppl_keep const DPPLSyclContextRef Ctx,
                                  __dppl_keep const char *Source,
                                  __dppl_keep const char *CompileOpts)
 {
-    cl_int err;
     std::string compileOpts;
     context *SyclCtx = nullptr;
     program *SyclProgram = nullptr;
diff --git a/backends/tests/test_sycl_kernel_interface.cpp b/backends/tests/test_sycl_kernel_interface.cpp
@@ -99,8 +99,8 @@ TEST_F (TestDPPLSyclKernelInterface, CheckGetNumArgs)
     auto AddKernel = DPPLProgram_GetKernel(PRef, "add");
     auto AxpyKernel = DPPLProgram_GetKernel(PRef, "axpy");
 
-    ASSERT_EQ(DPPLKernel_GetNumArgs(AddKernel), 3);
-    ASSERT_EQ(DPPLKernel_GetNumArgs(AxpyKernel), 4);
+    ASSERT_EQ(DPPLKernel_GetNumArgs(AddKernel), 3ul);
+    ASSERT_EQ(DPPLKernel_GetNumArgs(AxpyKernel), 4ul);
 
     DPPLQueue_Delete(QueueRef);
     DPPLContext_Delete(CtxRef);
diff --git a/backends/tests/test_sycl_platform_interface.cpp b/backends/tests/test_sycl_platform_interface.cpp
@@ -32,13 +32,13 @@ struct TestDPPLSyclPlatformInterface : public ::testing::Test
 TEST_F (TestDPPLSyclPlatformInterface, CheckGetNumPlatforms)
 {
     auto nplatforms = DPPLPlatform_GetNumNonHostPlatforms();
-    EXPECT_GE(nplatforms, 0);
+    EXPECT_GE(nplatforms, 0ul);
 }
 
 TEST_F (TestDPPLSyclPlatformInterface, GetNumBackends)
 {
     auto nbackends = DPPLPlatform_GetNumNonHostBackends();
-    EXPECT_GE(nbackends, 0);
+    EXPECT_GE(nbackends, 0ul);
 }
 
 TEST_F (TestDPPLSyclPlatformInterface, GetListOfBackends)
diff --git a/backends/tests/test_sycl_program_interface.cpp b/backends/tests/test_sycl_program_interface.cpp
@@ -40,74 +40,75 @@ using namespace cl::sycl;
 
 namespace
 {
-    const size_t SIZE = 1024;
+const int SIZE = 1024;
 
-    void add_kernel_checker (queue *syclQueue, DPPLSyclKernelRef AddKernel)
-    {
-        range<1> a_size{SIZE};
-        std::array<int, SIZE> a, b, c;
+void add_kernel_checker (queue *syclQueue, DPPLSyclKernelRef AddKernel)
+{
+    range<1> a_size{SIZE};
+    std::array<int, SIZE> a, b, c;
 
-        for (int i = 0; i<SIZE; ++i) {
-            a[i] = i;
-            b[i] = i;
-            c[i] = 0;
-        }
+    for (int i = 0; i < SIZE; ++i) {
+        a[i] = i;
+        b[i] = i;
+        c[i] = 0;
+    }
 
-        {
-            buffer<int, 1> a_device(a.data(), a_size);
-            buffer<int, 1> b_device(b.data(), a_size);
-            buffer<int, 1> c_device(c.data(), a_size);
-            buffer<int, 1> buffs[3] = {a_device, b_device, c_device};
-            syclQueue->submit([&](handler& cgh) {
-                for (auto buff : buffs) {
-                    auto arg = buff.get_access<access::mode::read_write>(cgh);
-                    cgh.set_args(arg);
-                }
-                auto syclKernel = reinterpret_cast<kernel*>(AddKernel);
-                cgh.parallel_for(range<1>{SIZE}, *syclKernel);
-            });
-        }
+    {
+        buffer<int, 1> a_device(a.data(), a_size);
+        buffer<int, 1> b_device(b.data(), a_size);
+        buffer<int, 1> c_device(c.data(), a_size);
+        buffer<int, 1> buffs[3] = {a_device, b_device, c_device};
+        syclQueue->submit([&](handler& cgh) {
+            for (auto buff : buffs) {
+                auto arg = buff.get_access<access::mode::read_write>(cgh);
+                cgh.set_args(arg);
+            }
+            auto syclKernel = reinterpret_cast<kernel*>(AddKernel);
+            cgh.parallel_for(range<1>{SIZE}, *syclKernel);
+        });
+    }
 
-        // Validate the data
-        for(auto i = 0ul; i < SIZE; ++i) {
-            EXPECT_EQ(c[i], i + i);
-        }
+    // Validate the data
+    for(int i = 0; i < SIZE; ++i) {
+        EXPECT_EQ(c[i], i + i);
     }
+}
 
-    void axpy_kernel_checker (queue *syclQueue, DPPLSyclKernelRef AxpyKernel)
-    {
-        range<1> a_size{SIZE};
-        std::array<int, SIZE> a, b, c;
+void axpy_kernel_checker (queue *syclQueue, DPPLSyclKernelRef AxpyKernel)
+{
+    range<1> a_size{SIZE};
+    std::array<int, SIZE> a, b, c;
 
-        for (int i = 0; i<SIZE; ++i) {
-            a[i] = i;
-            b[i] = i;
-            c[i] = 0;
-        }
-        int d = 10;
-        {
-            buffer<int, 1> a_device(a.data(), a_size);
-            buffer<int, 1> b_device(b.data(), a_size);
-            buffer<int, 1> c_device(c.data(), a_size);
-            buffer<int, 1> buffs[3] = {a_device, b_device, c_device};
-            syclQueue->submit([&](handler& cgh) {
-                for (auto i = 0ul; i < 3; ++i) {
-                    auto arg = buffs[i].get_access<access::mode::read_write>(cgh);
-                    cgh.set_arg(i, arg);
-                }
-                cgh.set_arg(3, d);
-                auto syclKernel = reinterpret_cast<kernel*>(AxpyKernel);
-                cgh.parallel_for(range<1>{SIZE}, *syclKernel);
-            });
-        }
+    for (int i = 0; i < SIZE; ++i) {
+        a[i] = i;
+        b[i] = i;
+        c[i] = 0;
+    }
+    int d = 10;
+    {
+        buffer<int, 1> a_device(a.data(), a_size);
+        buffer<int, 1> b_device(b.data(), a_size);
+        buffer<int, 1> c_device(c.data(), a_size);
+        buffer<int, 1> buffs[3] = {a_device, b_device, c_device};
+        syclQueue->submit([&](handler& cgh) {
+            for (auto i = 0ul; i < 3; ++i) {
+                auto arg = buffs[i].get_access<access::mode::read_write>(cgh);
+                cgh.set_arg(i, arg);
+            }
+            cgh.set_arg(3, d);
+            auto syclKernel = reinterpret_cast<kernel*>(AxpyKernel);
+            cgh.parallel_for(range<1>{SIZE}, *syclKernel);
+        });
+    }
 
-        // Validate the data
-        for(auto i = 0ul; i < SIZE; ++i) {
-            EXPECT_EQ(c[i], i + d*i);
-        }
+    // Validate the data
+    for(int i = 0; i < SIZE; ++i) {
+        EXPECT_EQ(c[i], i + d*i);
     }
 }
 
+} /* end of anonymous namespace */
+
 struct TestDPPLSyclProgramInterface : public ::testing::Test
 {
     const char *CLProgramStr = R"CLC(
@@ -128,10 +129,10 @@ struct TestDPPLSyclProgramInterface : public ::testing::Test
     size_t nOpenCLGpuQ = 0;
 
     TestDPPLSyclProgramInterface () :
-        nOpenCLGpuQ(DPPLQueueMgr_GetNumQueues(DPPL_OPENCL, DPPL_GPU)),
         spirvFile{"./multi_kernel.spv", std::ios::binary | std::ios::ate},
         spirvFileSize(std::filesystem::file_size("./multi_kernel.spv")),
-        spirvBuffer(spirvFileSize)
+        spirvBuffer(spirvFileSize),
+        nOpenCLGpuQ(DPPLQueueMgr_GetNumQueues(DPPL_OPENCL, DPPL_GPU))
     {
         spirvFile.seekg(0, std::ios::beg);
         spirvFile.read(spirvBuffer.data(), spirvFileSize);
diff --git a/backends/tests/test_sycl_queue_manager.cpp b/backends/tests/test_sycl_queue_manager.cpp
@@ -83,7 +83,7 @@ TEST_F (TestDPPLSyclQueueManager, CheckDPPLGetCurrentQueue)
     if(!has_devices())
         GTEST_SKIP_("Skipping: No Sycl devices.\n");
 
-    DPPLSyclQueueRef q;
+    DPPLSyclQueueRef q = nullptr;
     ASSERT_NO_THROW(q = DPPLQueueMgr_GetCurrentQueue());
     ASSERT_TRUE(q != nullptr);
 }
@@ -170,7 +170,6 @@ TEST_F (TestDPPLSyclQueueManager, CheckGetNumActivatedQueues)
 
     auto nOpenCLCpuQ = DPPLQueueMgr_GetNumQueues(DPPL_OPENCL, DPPL_CPU);
     auto nOpenCLGpuQ = DPPLQueueMgr_GetNumQueues(DPPL_OPENCL, DPPL_GPU);
-    auto nL0GpuQ     = DPPLQueueMgr_GetNumQueues(DPPL_LEVEL_ZERO, DPPL_GPU);
 
     // Add a queue to main thread
     if(!nOpenCLCpuQ || !nOpenCLGpuQ)
@@ -192,10 +191,10 @@ TEST_F (TestDPPLSyclQueueManager, CheckGetNumActivatedQueues)
 
     // Verify what the expected number of activated queues each time a thread
     // called getNumActivatedQueues.
-    EXPECT_EQ(num0, 1);
-    EXPECT_EQ(num1, 2);
-    EXPECT_EQ(num2, 1);
-    EXPECT_EQ(num4, 0);
+    EXPECT_EQ(num0, 1ul);
+    EXPECT_EQ(num1, 2ul);
+    EXPECT_EQ(num2, 1ul);
+    EXPECT_EQ(num4, 0ul);
 
     DPPLQueue_Delete(q);
 }
diff --git a/backends/tests/test_sycl_usm_interface.cpp b/backends/tests/test_sycl_usm_interface.cpp
diff --git a/setup.py b/setup.py

Original file line number	Diff line number	Diff line change
`@@ -96,7 +96,6 @@ DPPLProgram_CreateFromOCLSource (__dppl_keep const DPPLSyclContextRef Ctx,`
`96`	`96`	`__dppl_keep const char *Source,`
`97`	`97`	`__dppl_keep const char *CompileOpts)`
`98`	`98`	`{`
`99`		`- cl_int err;`
`100`	`99`	`std::string compileOpts;`
`101`	`100`	`context *SyclCtx = nullptr;`
`102`	`101`	`program *SyclProgram = nullptr;`
Original file line number	Diff line number	Diff line change
`@@ -32,13 +32,13 @@ struct TestDPPLSyclPlatformInterface : public ::testing::Test`
`32`	`32`	`TEST_F (TestDPPLSyclPlatformInterface, CheckGetNumPlatforms)`
`33`	`33`	`{`
`34`	`34`	`auto nplatforms = DPPLPlatform_GetNumNonHostPlatforms();`
`35`		`- EXPECT_GE(nplatforms, 0);`
	`35`	`+ EXPECT_GE(nplatforms, 0ul);`
`36`	`36`	`}`
`37`	`37`
`38`	`38`	`TEST_F (TestDPPLSyclPlatformInterface, GetNumBackends)`
`39`	`39`	`{`
`40`	`40`	`auto nbackends = DPPLPlatform_GetNumNonHostBackends();`
`41`		`- EXPECT_GE(nbackends, 0);`
	`41`	`+ EXPECT_GE(nbackends, 0ul);`
`42`	`42`	`}`
`43`	`43`
`44`	`44`	`TEST_F (TestDPPLSyclPlatformInterface, GetListOfBackends)`