[Piglit] [PATCH] cl: From API clRetain* tests, removed attempt to release already released and destroyed objects

Tom Stellard tom at stellard.net
Mon Aug 26 18:35:48 PDT 2013 

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 <tom at stellard.net>:
> 
> >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,
> >>-	                          &param_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
> >>Piglit at lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/piglit
> >
> 
> 
>

More information about the Piglit mailing list