Fix copy and move semantics of wrapper classes (#1268)

* Remove unnecessary code

These custom equality operators are not necessary because of the
conversion operators which already allow using the standard equality
operators between two pointers.

Signed-off-by: Marco Antognini <marco.antognini@arm.com>

* Fix copy and move semantics of wrapper classes

Related to #465.

The Wrapper classes are rewritten to properly handle copy and move
semantics, while preserving the existing API and removing code
duplication.

Add error handling around clRelase* and clRetain*.

Signed-off-by: Marco Antognini <marco.antognini@arm.com>

* Address build issue on 32-bit Windows

Include linkage in RetainReleaseType function type.

Signed-off-by: Marco Antognini <marco.antognini@arm.com>

test_common/harness/typeWrappers.h

@@ -16,123 +16,135 @@
 #ifndef _typeWrappers_h
 #define _typeWrappers_h
 
-#include <stdio.h>
-#include <stdlib.h>
-
 #if !defined(_WIN32)
 #include <sys/mman.h>
 #endif
 
 #include "compat.h"
-#include <stdio.h>
 #include "mt19937.h"
 #include "errorHelpers.h"
 #include "kernelHelpers.h"
 
-/* cl_context wrapper */
+#include <cstdlib>
+#include <type_traits>
 
-class clContextWrapper {
+namespace wrapper_details {
-public:
+
-    clContextWrapper() { mContext = NULL; }
+// clRetain*() and clRelease*() functions share the same type.
-    clContextWrapper(cl_context program) { mContext = program; }
+template <typename T> // T should be cl_context, cl_program, ...
-    ~clContextWrapper()
+using RetainReleaseType = cl_int CL_API_CALL(T);
+
+// A generic wrapper class that follows OpenCL retain/release semantics.
+//
+// This Wrapper class implement copy and move semantics, which makes it
+// compatible with standard containers for example.
+//
+// Template parameters:
+//  - T is the cl_* type (e.g. cl_context, cl_program, ...)
+//  - Retain is the clRetain* function (e.g. clRetainContext, ...)
+//  - Release is the clRelease* function (e.g. clReleaseContext, ...)
+template <typename T, RetainReleaseType<T> Retain, RetainReleaseType<T> Release>
+class Wrapper {
+    static_assert(std::is_pointer<T>::value, "T should be a pointer type.");
+    T object = nullptr;
+
+    void retain()
     {
-        if (mContext != NULL) clReleaseContext(mContext);
+        if (!object) return;
+
+        auto err = Retain(object);
+        if (err != CL_SUCCESS)
+        {
+            print_error(err, "clRetain*() failed");
+            std::abort();
+        }
     }
 
-    clContextWrapper &operator=(const cl_context &rhs)
+    void release()
     {
-        mContext = rhs;
+        if (!object) return;
+
+        auto err = Release(object);
+        if (err != CL_SUCCESS)
+        {
+            print_error(err, "clRelease*() failed");
+            std::abort();
+        }
+    }
+
+public:
+    Wrapper() = default;
+
+    // On initialisation, assume the object has a refcount of one.
+    Wrapper(T object): object(object) {}
+
+    // On assignment, assume the object has a refcount of one.
+    Wrapper &operator=(T rhs)
+    {
+        reset(rhs);
         return *this;
     }
-    operator cl_context() const { return mContext; }
 
-    cl_context *operator&() { return &mContext; }
+    // Copy semantics, increase retain count.
-
+    Wrapper(Wrapper const &w) { *this = w; }
-    bool operator==(const cl_context &rhs) { return mContext == rhs; }
+    Wrapper &operator=(Wrapper const &w)
-
-protected:
-    cl_context mContext;
-};
-
-/* cl_program wrapper */
-
-class clProgramWrapper {
-public:
-    clProgramWrapper() { mProgram = NULL; }
-    clProgramWrapper(cl_program program) { mProgram = program; }
-    ~clProgramWrapper()
     {
-        if (mProgram != NULL) clReleaseProgram(mProgram);
+        reset(w.object);
-    }
+        retain();
-
-    clProgramWrapper &operator=(const cl_program &rhs)
-    {
-        mProgram = rhs;
         return *this;
     }
-    operator cl_program() const { return mProgram; }
 
-    cl_program *operator&() { return &mProgram; }
+    // Move semantics, directly take ownership.
-
+    Wrapper(Wrapper &&w) { *this = std::move(w); }
-    bool operator==(const cl_program &rhs) { return mProgram == rhs; }
+    Wrapper &operator=(Wrapper &&w)
-
-protected:
-    cl_program mProgram;
-};
-
-/* cl_kernel wrapper */
-
-class clKernelWrapper {
-public:
-    clKernelWrapper() { mKernel = NULL; }
-    clKernelWrapper(cl_kernel kernel) { mKernel = kernel; }
-    ~clKernelWrapper()
     {
-        if (mKernel != NULL) clReleaseKernel(mKernel);
+        reset(w.object);
-    }
+        w.object = nullptr;
-
-    clKernelWrapper &operator=(const cl_kernel &rhs)
-    {
-        mKernel = rhs;
         return *this;
     }
-    operator cl_kernel() const { return mKernel; }
 
-    cl_kernel *operator&() { return &mKernel; }
+    ~Wrapper() { reset(); }
 
-    bool operator==(const cl_kernel &rhs) { return mKernel == rhs; }
+    // Release the existing object, if any, and own the new one, if any.
-
+    void reset(T new_object = nullptr)
-protected:
-    cl_kernel mKernel;
-};
-
-/* cl_mem (stream) wrapper */
-
-class clMemWrapper {
-public:
-    clMemWrapper() { mMem = NULL; }
-    clMemWrapper(cl_mem mem) { mMem = mem; }
-    ~clMemWrapper()
     {
-        if (mMem != NULL) clReleaseMemObject(mMem);
+        release();
+        object = new_object;
     }
 
-    clMemWrapper &operator=(const cl_mem &rhs)
+    operator T() const { return object; }
-    {
-        mMem = rhs;
-        return *this;
-    }
-    operator cl_mem() const { return mMem; }
 
-    cl_mem *operator&() { return &mMem; }
+    // Ideally this function should not exist as it breaks encapsulation by
-
+    // allowing external mutation of the Wrapper internal state. However, too
-    bool operator==(const cl_mem &rhs) { return mMem == rhs; }
+    // much code currently relies on this. For example, instead of using T* as
-
+    // output parameters, existing code can be updated to use Wrapper& instead.
-protected:
+    T *operator&() { return &object; }
-    cl_mem mMem;
 };
 
+} // namespace wrapper_details
+
+using clContextWrapper =
+    wrapper_details::Wrapper<cl_context, clRetainContext, clReleaseContext>;
+
+using clProgramWrapper =
+    wrapper_details::Wrapper<cl_program, clRetainProgram, clReleaseProgram>;
+
+using clKernelWrapper =
+    wrapper_details::Wrapper<cl_kernel, clRetainKernel, clReleaseKernel>;
+
+using clMemWrapper =
+    wrapper_details::Wrapper<cl_mem, clRetainMemObject, clReleaseMemObject>;
+
+using clCommandQueueWrapper =
+    wrapper_details::Wrapper<cl_command_queue, clRetainCommandQueue,
+                             clReleaseCommandQueue>;
+
+using clSamplerWrapper =
+    wrapper_details::Wrapper<cl_sampler, clRetainSampler, clReleaseSampler>;
+
+using clEventWrapper =
+    wrapper_details::Wrapper<cl_event, clRetainEvent, clReleaseEvent>;
+
 class clProtectedImage {
 public:
     clProtectedImage()
@@ -183,92 +195,12 @@ public:
 
     cl_mem *operator&() { return &image; }
 
-    bool operator==(const cl_mem &rhs) { return image == rhs; }
-
 protected:
     void *backingStore;
     size_t backingStoreSize;
     cl_mem image;
 };
 
-/* cl_command_queue wrapper */
-class clCommandQueueWrapper {
-public:
-    clCommandQueueWrapper() { mMem = NULL; }
-    clCommandQueueWrapper(cl_command_queue mem) { mMem = mem; }
-    ~clCommandQueueWrapper()
-    {
-        if (mMem != NULL)
-        {
-            clReleaseCommandQueue(mMem);
-        }
-    }
-
-    clCommandQueueWrapper &operator=(const cl_command_queue &rhs)
-    {
-        mMem = rhs;
-        return *this;
-    }
-    operator cl_command_queue() const { return mMem; }
-
-    cl_command_queue *operator&() { return &mMem; }
-
-    bool operator==(const cl_command_queue &rhs) { return mMem == rhs; }
-
-protected:
-    cl_command_queue mMem;
-};
-
-/* cl_sampler wrapper */
-class clSamplerWrapper {
-public:
-    clSamplerWrapper() { mMem = NULL; }
-    clSamplerWrapper(cl_sampler mem) { mMem = mem; }
-    ~clSamplerWrapper()
-    {
-        if (mMem != NULL) clReleaseSampler(mMem);
-    }
-
-    clSamplerWrapper &operator=(const cl_sampler &rhs)
-    {
-        mMem = rhs;
-        return *this;
-    }
-    operator cl_sampler() const { return mMem; }
-
-    cl_sampler *operator&() { return &mMem; }
-
-    bool operator==(const cl_sampler &rhs) { return mMem == rhs; }
-
-protected:
-    cl_sampler mMem;
-};
-
-/* cl_event wrapper */
-class clEventWrapper {
-public:
-    clEventWrapper() { mMem = NULL; }
-    clEventWrapper(cl_event mem) { mMem = mem; }
-    ~clEventWrapper()
-    {
-        if (mMem != NULL) clReleaseEvent(mMem);
-    }
-
-    clEventWrapper &operator=(const cl_event &rhs)
-    {
-        mMem = rhs;
-        return *this;
-    }
-    operator cl_event() const { return mMem; }
-
-    cl_event *operator&() { return &mMem; }
-
-    bool operator==(const cl_event &rhs) { return mMem == rhs; }
-
-protected:
-    cl_event mMem;
-};
-
 /* Generic protected memory buffer, for verifying access within bounds */
 class clProtectedArray {
 public:

test_conformance/buffers/test_sub_buffers.cpp

@@ -39,7 +39,8 @@ public:
         region.size = mSize;
 
         cl_int error;
-        mMem = clCreateSubBuffer( mParentBuffer, flags, CL_BUFFER_CREATE_TYPE_REGION, &region, &error );
+        reset(clCreateSubBuffer(mParentBuffer, flags,
+                                CL_BUFFER_CREATE_TYPE_REGION, &region, &error));
         return error;
     }
 };