On Thu, Aug 22, 2013 at 01:26:04PM +0300, Ville Korhonen wrote: > Problem with releasing already released objects originates from ICD > Loader, which requires that object is alive when calling OpenCL host > functions. Khronos ICD Loader specs > (http://www.khronos.org/registry/cl/extensions/khr/cl_khr_icd.txt) > says: > > At every OpenCL function call, the ICD Loader infers the Vendor ICD > function to call from the arguments to the function. An object is > said to be ICD compatible if it is of the following structure: > > struct _cl_<object> > { > struct _cl_icd_dispatch *dispatch; > // ... remainder of internal data > }; > > <object> is one of platform_id, device_id, context, command_queue, > mem, program, kernel, event, or sampler. > > The structure _cl_icd_dispatch is a function pointer dispatch > table which is used to direct calls to a particular vendor > implementation. All objects created from ICD compatible objects > must be ICD compatible. > > OpenCL specs says that when reference counter hits zero object is > deleted. If OpenCL API function is called for a deleted object ICD > Loader segfaults because it tries to use already freed object and > corresponding dispatch table. So if we want to return CL_INVALID_* > in these cases, objects could never be actually deleted, or we would > need some middle structure (that could never be freed) to indicate > if object is alive or not. Either way result is memory leak. > OpenCL specs does not specify what an invalid object means but > Khronos ICD specs indicates that deleted object is not to be used. >
I discussed this very same issue Blaž when he was first working on the OpenCL piglit tests. I think what the ICD loader / implementation is supposed to do in this case is to keep a hash of valid objects and use the hash to verify that an object is valid before trying to use it. > And the other thing was calling API-functions with NULL object > pointer. ICD Loader specs does not say much about it. There is only > one item in Issues section: > > 3: How will the ICD handle a NULL cl_platform_id? > RESOLVED: The NULL platform is not supported by the ICD. > > Although current ICD Loader implementation never calls any API > function if object pointer is NULL. So it is not possible to return > any error. > I think it should be returning INVALID_OBJECT for NULL in most cases. > And lastly I make own patch for create-context. > > Was this patch sent in a different email? I don't see it. -Tom > Quoting Tom Stellard <[email protected]>: > > >On Wed, Aug 21, 2013 at 04:16:47PM +0300, Ville Korhonen wrote: > >>All retain/release API tests try to test behaviour when trying to > >>release already released and destroyed object. OpenCL 1.2 Spec says > >>(considering clReleaseMemObject: > >> > >>"After the memobj reference count becomes zero and commands queued > >>for execution on a command-queue(s) that use memobj have finished, > >>the memory object is deleted. If memobj is a buffer object, memobj > >>cannot be deleted until all sub-buffer objects associated with > >>memobj are deleted." > >> > >>Calling release to a released object causes segmentation fault when > >>using ICD Loader. > > > >As I understand the spec, the tests are correct. A released object is > >invalid, so the CL_INVALID_* error should be returned. Do you see > >anything in the spec that says otherwise? > > > >> > >>Also test create-context checks error message when num_devices is > >>zero, but given num_devices was a valid number. Changed function > >>call to actually pass zero to the function. > > > >This change should be in a separate test. > > > > > >-Tom > >> > >> > >>diff --git a/tests/cl/api/create-context.c b/tests/cl/api/create-context.c > >>index 7858940..39150d0 100644 > >>--- a/tests/cl/api/create-context.c > >>+++ b/tests/cl/api/create-context.c > >>@@ -251,7 +251,7 @@ piglit_cl_test(const int argc, > >> test(context_properties, num_devices, NULL, NULL, NULL, > >> CL_INVALID_VALUE, &result, > >> "Trigger CL_INVALID_VALUE if devices is NULL"); > >>- test(context_properties, num_devices, devices, NULL, > >>&context_properties, > >>+ test(context_properties, 0, devices, NULL, &context_properties, > >> CL_INVALID_VALUE, &result, > >> "Trigger CL_INVALID_VALUE if num_devices is equal to zero"); > >> test(context_properties, 0, devices, NULL, NULL, > >>diff --git a/tests/cl/api/get-platform-info.c > >>b/tests/cl/api/get-platform-info.c > >>index c5cd4e0..cf61586 100644 > >>--- a/tests/cl/api/get-platform-info.c > >>+++ b/tests/cl/api/get-platform-info.c > >>@@ -154,21 +154,5 @@ piglit_cl_test(const int argc, > >> piglit_merge_result(&result, PIGLIT_FAIL); > >> } > >> > >>- /* > >>- * CL_INVALID_PLATFORM if platform is not a valid platform. > >>- */ > >>- errNo = clGetPlatformInfo(invalid_platform_id, > >>- CL_PLATFORM_PROFILE, > >>- 0, > >>- NULL, > >>- ¶m_value_size); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_PLATFORM)) { > >>- fprintf(stderr, > >>- "Failed (error code: %s): Trigger CL_INVALID_PLATFORM if > >>platform is not a valid platform.\n", > >>- piglit_cl_get_error_name(errNo)); > >>- piglit_merge_result(&result, PIGLIT_FAIL); > >>- } > >>- > >>- > >>- return result; > >>+ return result; > >> } > >>diff --git a/tests/cl/api/retain_release-command-queue.c > >>b/tests/cl/api/retain_release-command-queue.c > >>index a4e8ffb..3eb746c 100644 > >>--- a/tests/cl/api/retain_release-command-queue.c > >>+++ b/tests/cl/api/retain_release-command-queue.c > >>@@ -139,25 +139,5 @@ piglit_cl_test(const int argc, > >> } > >> } > >> > >>- /*** Errors ***/ > >>- > >>- /* > >>- * CL_INVALID_COMMAND_QUEUE if command_queue is not a valid > >>command-queue. > >>- */ > >>- errNo = clReleaseCommandQueue(command_queue); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_COMMAND_QUEUE)) { > >>- fprintf(stderr, > >>- "clReleaseCommandQueue: Failed (error code: > >>%s): Trigger > >>CL_INVALID_COMMAND_QUEUE if command_queue is not a valid > >>command-queue (already released).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- errNo = clReleaseCommandQueue(NULL); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_COMMAND_QUEUE)) { > >>- fprintf(stderr, > >>- "clReleaseCommandQueue: Failed (error code: > >>%s): Trigger > >>CL_INVALID_COMMAND_QUEUE if command_queue is not a valid > >>command-queue (NULL).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- > >> return PIGLIT_PASS; > >> } > >>diff --git a/tests/cl/api/retain_release-context.c > >>b/tests/cl/api/retain_release-context.c > >>index 6aaa4d9..b2bcd78 100644 > >>--- a/tests/cl/api/retain_release-context.c > >>+++ b/tests/cl/api/retain_release-context.c > >>@@ -148,25 +148,5 @@ piglit_cl_test(const int argc, > >> } > >> } > >> > >>- /*** Errors ***/ > >>- > >>- /* > >>- * CL_INVALID_CONTEXT if context is not a valid OpenCL context. > >>- */ > >>- errNo = clReleaseContext(cl_ctx); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_CONTEXT)) { > >>- fprintf(stderr, > >>- "clReleaseContext: Failed (error code: %s): > >>Trigger > >>CL_INVALID_CONTEXT if context is not a valid context (already > >>released).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- errNo = clReleaseContext(NULL); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_CONTEXT)) { > >>- fprintf(stderr, > >>- "clReleaseContext: Failed (error code: %s): > >>Trigger > >>CL_INVALID_CONTEXT if context is not a valid context (NULL).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- > >> return PIGLIT_PASS; > >> } > >>diff --git a/tests/cl/api/retain_release-event.c > >>b/tests/cl/api/retain_release-event.c > >>index bfff199..f1a8170 100644 > >>--- a/tests/cl/api/retain_release-event.c > >>+++ b/tests/cl/api/retain_release-event.c > >>@@ -149,27 +149,5 @@ piglit_cl_test(const int argc, > >> } > >> } > >> > >>- /*** Errors ***/ > >>- > >>- /* > >>- * CL_INVALID_EVENT if event is not a valid event object. > >>- */ > >>- errNo = clReleaseEvent(event); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_EVENT)) { > >>- fprintf(stderr, > >>- "clReleaseEvent: Failed (error code: %s): > >>Trigger > >>CL_INVALID_EVENT if event is not a valid event object (already > >>released).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- errNo = clReleaseEvent(NULL); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_EVENT)) { > >>- fprintf(stderr, > >>- "clReleaseEvent: Failed (error code: %s): > >>Trigger > >>CL_INVALID_EVENT if event is not a valid event object (NULL).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- > >>- clReleaseMemObject(memobj); > >>- > >> return PIGLIT_PASS; > >> } > >>diff --git a/tests/cl/api/retain_release-kernel.c > >>b/tests/cl/api/retain_release-kernel.c > >>index 94857d2..4bad2de 100644 > >>--- a/tests/cl/api/retain_release-kernel.c > >>+++ b/tests/cl/api/retain_release-kernel.c > >>@@ -137,25 +137,5 @@ piglit_cl_test(const int argc, > >> } > >> } > >> > >>- /*** Errors ***/ > >>- > >>- /* > >>- * CL_INVALID_KERNEL if kernel is not a valid kernel object. > >>- */ > >>- errNo = clReleaseKernel(kernel); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_KERNEL)) { > >>- fprintf(stderr, > >>- "clReleaseKernel: Failed (error code: %s): > >>Trigger > >>CL_INVALID_KERNEL if kernel is not a valid kernel object (already > >>released).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- errNo = clReleaseKernel(NULL); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_KERNEL)) { > >>- fprintf(stderr, > >>- "clReleaseKernel: Failed (error code: %s): > >>Trigger > >>CL_INVALID_KERNEL if kernel is not a valid kernel object (NULL).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- > >> return PIGLIT_PASS; > >> } > >>diff --git a/tests/cl/api/retain_release-mem-object.c > >>b/tests/cl/api/retain_release-mem-object.c > >>index ae4555f..286d46e 100644 > >>--- a/tests/cl/api/retain_release-mem-object.c > >>+++ b/tests/cl/api/retain_release-mem-object.c > >>@@ -138,26 +138,5 @@ piglit_cl_test(const int argc, > >> } > >> } > >> > >>- /*** Errors ***/ > >>- > >>- /* > >>- * CL_INVALID_MEM_OBJECT if mem_object is not a valid mem_object object > >>- * (buffer or image object). > >>- */ > >>- errNo = clReleaseMemObject(memobj); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_MEM_OBJECT)) { > >>- fprintf(stderr, > >>- "clReleaseMemObject: Failed (error code: %s): > >>Trigger > >>CL_INVALID_MEM_OBJECT if memOBJ is not a valid memory object > >>(already released).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- errNo = clReleaseMemObject(NULL); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_MEM_OBJECT)) { > >>- fprintf(stderr, > >>- "clReleaseMemObject: Failed (error code: %s): > >>Trigger > >>CL_INVALID_MEM_OBJECT if memobj is not a valid memory object > >>(NULL).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- > >> return PIGLIT_PASS; > >> } > >>diff --git a/tests/cl/api/retain_release-program.c > >>b/tests/cl/api/retain_release-program.c > >>index aba5426..cf20dc8 100644 > >>--- a/tests/cl/api/retain_release-program.c > >>+++ b/tests/cl/api/retain_release-program.c > >>@@ -140,25 +140,5 @@ piglit_cl_test(const int argc, > >> } > >> } > >> > >>- /*** Errors ***/ > >>- > >>- /* > >>- * CL_INVALID_PROGRAM if program is not a valid program object. > >>- */ > >>- errNo = clReleaseProgram(program); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_PROGRAM)) { > >>- fprintf(stderr, > >>- "clReleaseProgram: Failed (error code: %s): > >>Trigger > >>CL_INVALID_PROGRAM if program is not a valid program object (already > >>released).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- errNo = clReleaseProgram(NULL); > >>- if(!piglit_cl_check_error(errNo, CL_INVALID_PROGRAM)) { > >>- fprintf(stderr, > >>- "clReleaseProgram: Failed (error code: %s): > >>Trigger > >>CL_INVALID_PROGRAM if program is not a valid program object > >>(NULL).\n", > >>- piglit_cl_get_error_name(errNo)); > >>- return PIGLIT_FAIL; > >>- } > >>- > >> return PIGLIT_PASS; > >> } > >> > >> > >>_______________________________________________ > >>Piglit mailing list > >>[email protected] > >>http://lists.freedesktop.org/mailman/listinfo/piglit > > > > > _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
