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

[Simon Richter]

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