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:[email protected]] On Behalf Of > Simon Richter > Sent: Friday, December 9, 2016 17:25 > To: [email protected] > 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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/beignet _______________________________________________ Beignet mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/beignet
