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

Tom Stellard tom at stellard.net
Thu Aug 29 13:11:56 PDT 2013 

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 <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
> > >
> > 
> > 
> > 
> 
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit

More information about the Piglit mailing list