Fix various memory leaks and a few errors

[Adam-]

Almost all of these are in error conditions except for:

1) a delete -> delete[] in clGetSupportedImageFormats 2) a delete in clCompileProgram

It also seems all of the existing code expects new to return NULL - however the standard form of new never returns NULL (and instead will throw a std::bad_alloc). I have changed it to instead call the nothrow version of new which will return NULL on allocation failure.

This also fixes a couple places where it will try to delete uninitialized pointers in arrays on error condtiions

[gpu]

Wow, thanks for that!

Admittedly, I assumed that new just returns NULL in the error case. I'm not sure where this assumption came from (it might have been caused by the behavior of malloc, but I think there had been more reasons behind that).

And regarding the missing delete[] calls: I have to admit that ... I didn't care. I knew that some of the previously allocated memory could have been freed in these cases, but when an allocation fails, the whole program hit such a massive brick wall that I thought that this would plainly no longer be relevant. It's "cleaner" in many ways, though, of course, and I really appreciate the effort and diligence you put into that!

(It's hard to review "each and every line" here, but from what I've read so far, it looks like it was applied systematically - did you use some tool support for that (some IDE plugin that reports these as warnings), or did you actually go manually through the whole code?)

[Adam-]

I went through all of this by hand. It may be good to switch all of these resources to use RAII at some point, since that is much less error prone.

[gpu]

There are certainly many possible "style" improvements. Although I have to admit that I'm not sure whether/how RAII should be applied systematically (one reason being that all this is targeting a C API, and the fact that it's C++ is rather for the convenience of new vs malloc and minor other details). But maybe I misunderstood this: Are you (roughly!, and only for example) referring to replacing the plain arrays with vectors? (At least, that would make much of the tedious new/delete handling obsolete...)

[Adam-]

You could replace the arrays with stl containers, but I was really referring to just the practice of deallocating resources when objects are destructed. You could eg. make an object whose destructor frees the resource such as:

template<typename T>
class Deleter {
    T *ptr;
 public:
    Deleter(T *ptr) : ptr(ptr) {}
    ~Deleter() { delete ptr; }
};

And then create one with the same lifetime as the associated ptr.

T *t = new T();
Deleter t_deleter(t);

Alternatively though, the std contains some things which can already do this such as std::unique_ptr. Here is clCreateImage3DNative using that:

JNIEXPORT jobject JNICALL Java_org_jocl_CL_clCreateImage3DNative
  (JNIEnv *env, jclass UNUSED(cls), jobject context, jlong flags, jobjectArray image_format, jlong image_width, jlong image_height, jlong image_depth, jlong image_row_pitch, jlong image_slice_pitch, jobject host_ptr, jintArray errcode_ret)
{
    Logger::log(LOG_TRACE, "Executing clCreateImage3D\n");
    if (clCreateImage3DFP == NULL)
    {
        ThrowByName(env, "java/lang/UnsupportedOperationException",
            "The function clCreateImage3D is not supported");
        return NULL;
    }

    // Native variables declaration
    cl_context nativeContext = NULL;
    cl_mem_flags nativeFlags = 0;
    std::unique_ptr<cl_image_format[]> nativeImage_format;
    size_t nativeImage_width = 0;
    size_t nativeImage_height = 0;
    size_t nativeImage_depth = 0;
    size_t nativeImage_row_pitch = 0;
    size_t nativeImage_slice_pitch = 0;
    void *nativeHost_ptr = NULL;
    cl_int nativeErrcode_ret = 0;
    cl_mem nativeMem = NULL;

    // Obtain native variable values
    if (context != NULL)
    {
        nativeContext = (cl_context)env->GetLongField(context, NativePointerObject_nativePointer);
    }
    nativeFlags = (cl_mem_flags)flags;
    if (image_format != NULL)
    {
        jsize image_formatLength = env->GetArrayLength(image_format);
        nativeImage_format = std::unique_ptr<cl_image_format[]>(new (std::nothrow) cl_image_format[(size_t)image_formatLength]);
        if (nativeImage_format == nullptr)
        {
            ThrowByName(env, "java/lang/OutOfMemoryError",
                "Out of memory during image format array creation");
            return NULL;
        }
        for (int i=0; i<image_formatLength; i++)
        {
            jobject format = env->GetObjectArrayElement(image_format, i);
            getCl_image_format(env, format, nativeImage_format[i]);
        }
    }
    nativeImage_width = (size_t)image_width;
    nativeImage_height = (size_t)image_height;
    nativeImage_depth = (size_t)image_depth;
    nativeImage_row_pitch = (size_t)image_row_pitch;
    nativeImage_slice_pitch = (size_t)image_slice_pitch;
    PointerData *host_ptrPointerData = initPointerData(env, host_ptr);
    if (host_ptrPointerData == NULL)
    {
        return NULL;
    }
    nativeHost_ptr = (void*)host_ptrPointerData->pointer;

    // TODO: Check if all flags are supported - does a global reference
    // to the host_ptr have to be created for CL_MEM_USE_HOST_PTR?
    // Otherwise, the host pointer data may be garbage collected!

    nativeMem = (clCreateImage3DFP)(nativeContext, nativeFlags, nativeImage_format.get(), nativeImage_width, nativeImage_height, nativeImage_depth, nativeImage_row_pitch, nativeImage_slice_pitch, nativeHost_ptr, &nativeErrcode_ret);

    // Write back native variable values and clean up
    if (!releasePointerData(env, host_ptrPointerData)) return NULL;
    if (!set(env, errcode_ret, 0, nativeErrcode_ret)) return NULL;

    if (nativeMem == NULL)
    {
        return NULL;
    }

    // Create and return the Java cl_mem object
    jobject mem = env->NewObject(cl_mem_Class, cl_mem_Constructor);
    if (env->ExceptionCheck())
    {
        return NULL;
    }

    setNativePointer(env, mem, (jlong)nativeMem);
    return mem;
}

It would get more tricky when you have an array of pointers which each requires freeing though - std::unique_ptr accepts in an Deleter which you can implement your own deletion logic.

[gpu]

Oh, I see. Well, wrapping some of the instances into unique_ptr or shared_ptr might simplify things a bit (but I'd probably not want to create custom allocators/deleters for the various types - that would quickly become something that would rather justify using https://github.khronos.org/OpenCL-CLHPP/ ). It's unlikely that I'll tackle this soon, but certainly worth considering.