[Beignet] [PATCH] Runtime: Use cl_ulong as CL_DEVICE_MAX_MEM_ALLOC_SIZE's return type.

[Yang, Rong R]

Yes, yor are right, beignet only touch param_value_size, so should not clear the other bits' value.
If application don’t check the param_value_size and fail, it is application's issue.

Thanks for your comments, I will send a new patch.

> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> Simon Richter
> Sent: Friday, December 9, 2016 17:25
> To: beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH] Runtime: Use cl_ulong as
> CL_DEVICE_MAX_MEM_ALLOC_SIZE's return type.
> 
> Hi,
> 
> On Fri, Dec 09, 2016 at 06:49:56AM +0000, Yang, Rong R wrote:
> 
> > The change is for the case that param_value_size > sizeof(device->FIELD).
> > For example:
> >   cl_ulong max_alloc_size;
> >   clGetDeviceInfo(device, CL_DEVICE_MAX_MEM_ALLOC_SIZE,
> > sizeof(max_alloc_size), &max_alloc_size, NULL);
> 
> > param_value_size is 8, if beignet's device->max_alloc_size is size_t,
> sizeof(device->max_alloc_size) is 4 in i386 systems.
> 
> Yes, but this has been fixed now.
> 
> > Because max_alloc_size hasn't been initialized, is garbage, and
> > memcpy(param_value, &device->FIELD, sizeof device->FIELD); only copy
> the low 4 bytes, the high 4 bytes is still garbage.
> > For example max_alloc_size is 0xdeaddeaddeaddead before call
> > clGetDeviceInfo, and device-> max_alloc_size is 0x40000000, After
> clGetDeviceInfo, max_alloc_size's value is 0xdeaddead40000000.
> 
> Right, but the returned size value is 4, so the program should know that
> there was a communication error, and bail out anyway, because if Beignet
> doesn't return sizeof(cl_ulong) there, then obviously it hasn't stored a
> cl_ulong, so the result cannot be interpreted that way.
> 
> If a program doesn't do that check, it is broken. I cannot expect
> 
>   __uint128_t max_alloc_size;
>   clGetDeviceInfo(device, CL_DEVICE_MAX_MEM_ALLOC_SIZE,
> sizeof(max_alloc_size), &max_alloc_size, NULL);
> 
> to work either. People need to write this out completely, as
> 
>   __uint128_t max_alloc_size;
>   cl_uint max_alloc_size_size;
>   int error = clGetDeviceInfo(device, CL_DEVICE_MAX_MEM_ALLOC_SIZE,
> sizeof max_alloc_size, &max_alloc_size, &max_alloc_size_size);
>   if(error != CL_SUCCESS)
>     goto error;
>   if(max_alloc_size_size != sizeof max_alloc_size)
>     goto error;
> 
> and if they don't, I want valgrind to say "btw, they are using a value with
> uninitialized bits here."
> 
> > So add memset(param_value, 0, param_value_size) to clear param_value,
> the param_value's size is param_value_size, I think it is safe.
> 
> Safe, yes, because the spec doesn't say what should happen to the rest of
> the buffer (so overwriting with zeros is fine), but also unnecessary.
> 
>    Simon
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/beignet