On Mon, Aug 26, 2013 at 06:35:48PM -0700, Tom Stellard wrote: > 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. >
It seems like there is some agreement that passing a freed object to the API is an application bug and the behavior is undefined, so I think your original patch is OK. Could you resend it with the context changes removed? Thanks, Tom > > 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 _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
