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

Ville Korhonen ville.t.korhonen at tut.fi
Thu Aug 22 03:26:04 PDT 2013 

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.

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.

And lastly I make own patch for create-context.


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