[Mesa-dev] [PATCH v2 RFC 1/1] clover: Don't crash on NULL global buffer objects.

[Francisco Jerez]

Jan Vesely <jan.vesely at rutgers.edu> writes:

> Specs say "If the argument is a buffer object, the arg_value
> pointer can be NULL or point to a NULL value in which case a NULL
> value will be used as the value for the argument declared as a
> pointer to __global or __constant memory in the kernel."
>
> So don't crash when somebody does that.
>
> v2: Insert NULL into input buffer instead of buffer handle pair
>     Fix constant_argument too
>     Drop r600 driver changes
>
> Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
> ---
> Hi Francisco,
>
Hi Jan,

> I could not find much info about how ctx.input is supposed to be used.
> It looks like it stores begin-end intervals of parameters so NULL, NULL
> seemed appropriate.
>

It's just an array of bytes that is passed to the driver as input for
the kernel.  Some suggestions more below.

> Is this closer to what you had in mind? This patch fixes my issue
> even without the r600 changes.
>
> regards,
> Jan
>
>  src/gallium/state_trackers/clover/core/kernel.cpp | 34 +++++++++++++----------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp b/src/gallium/state_trackers/clover/core/kernel.cpp
> index 58780d6..d4942b3 100644
> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
> @@ -327,16 +327,19 @@ kernel::global_argument::set(size_t size, const void *value) {
>     if (size != sizeof(cl_mem))
>        throw error(CL_INVALID_ARG_SIZE);
>  
> -   buf = &obj<buffer>(*(cl_mem *)value);
> +   buf = pobj<buffer>(value ? *(cl_mem *)value : NULL);
>     _set = true;
>  }
>  
>  void
>  kernel::global_argument::bind(exec_context &ctx,
>                                const module::argument &marg) {
> -   align(ctx.input, marg.target_align);

As null pointers have the same alignment restrictions as normal pointers,
you should keep this line here before the 'if (buf)' conditional.

> -   ctx.g_handles.push_back(allocate(ctx.input, marg.target_size));
> -   ctx.g_buffers.push_back(buf->resource(*ctx.q).pipe);
> +   if (buf) {
> +      align(ctx.input, marg.target_align);
> +      ctx.g_handles.push_back(allocate(ctx.input, marg.target_size));
> +      ctx.g_buffers.push_back(buf->resource(*ctx.q).pipe);
> +   } else

For consistency use braces around the else block of an if block that
uses braces.

> +      insert(ctx.input, {NULL, NULL});

I don't think this does what you want.  The easiest way to append zeroes
to the input buffer is:

|    } else {
|       // Null global pointer.
|       allocate(ctx.input, marg.target_size);
|    }

>  }
>  
>  void
> @@ -379,22 +382,25 @@ kernel::constant_argument::set(size_t size, const void *value) {
>     if (size != sizeof(cl_mem))
>        throw error(CL_INVALID_ARG_SIZE);
>  
> -   buf = &obj<buffer>(*(cl_mem *)value);
> +   buf = pobj<buffer>(value ? *(cl_mem *)value : NULL);
>     _set = true;
>  }
>  
>  void
>  kernel::constant_argument::bind(exec_context &ctx,
>                                  const module::argument &marg) {
> -   auto v = bytes(ctx.resources.size() << 24);
> -
> -   extend(v, module::argument::zero_ext, marg.target_size);
> -   byteswap(v, ctx.q->dev.endianness());
> -   align(ctx.input, marg.target_align);
> -   insert(ctx.input, v);
> -
> -   st = buf->resource(*ctx.q).bind_surface(*ctx.q, false);
> -   ctx.resources.push_back(st);
> +   if (buf) {
> +      auto v = bytes(ctx.resources.size() << 24);
> +
> +      extend(v, module::argument::zero_ext, marg.target_size);
> +      byteswap(v, ctx.q->dev.endianness());
> +      align(ctx.input, marg.target_align);

This align() call should be kept outside the conditional too.

> +      insert(ctx.input, v);
> +
> +      st = buf->resource(*ctx.q).bind_surface(*ctx.q, false);
> +      ctx.resources.push_back(st);
> +   } else
> +      insert(ctx.input, {NULL, NULL});

Same comment as before.

Thank you.

>  }
>  
>  void
> -- 
> 1.8.4.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140115/88733293/attachment.pgp>