<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Apr 16, 2018, 5:24 PM Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Aaron Watry <<a href="mailto:awatry@gmail.com" target="_blank">awatry@gmail.com</a>> writes:<br>
<br>
>   From CL 1.2 Section 5.2.1:<br>
>     CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY and<br>
>     flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with<br>
>     CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or if<br>
>     buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify<br>
>     CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .<br>
><br>
> Fixes CL 1.2 CTS test/api get_buffer_info<br>
><br>
<br>
What combination of flags is the test-case providing for both the<br>
parent and sub buffer?<br></blockquote></div><div><br></div><div>The original motivation for this was a CTS test that was creating a sub buffer with flags of:</div><div>CL_MEM_HOST_NO_ACCESS | CL_MEM_READ_WRITE</div><div><br></div><div>With a parent buffer created as:</div><div>CL_MEM_HOST_READ_ONLY | CL_MEM_READ_WRITE</div><div><br></div><div>Which according to my reading of the spec should be allowed.</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Signed-off-by: Aaron Watry <<a href="mailto:awatry@gmail.com" target="_blank">awatry@gmail.com</a>><br>
> Cc: Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>><br>
> ---<br>
>  src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--<br>
>  1 file changed, 6 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp<br>
> index 9b3cd8b1f5..451e8a8c56 100644<br>
> --- a/src/gallium/state_trackers/clover/api/memory.cpp<br>
> +++ b/src/gallium/state_trackers/clover/api/memory.cpp<br>
> @@ -57,10 +57,14 @@ namespace {<br>
>                                        parent.flags() & host_access_flags) |<br>
>                                       (parent.flags() & host_ptr_flags));<br>
>  <br>
> -         if (~flags & parent.flags() &<br>
> -             ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))<br>
> +         if (~flags & parent.flags() & (dev_access_flags & ~CL_MEM_READ_WRITE))<br>
>              throw error(CL_INVALID_VALUE);<br>
>  <br>
> +         //Check if new host access flags cause a mismatch between host-read/write-only.<br>
> +         const cl_mem_flags new_flags = flags & ~(parent.flags()) & ~CL_MEM_HOST_NO_ACCESS;<br>
> +         if (new_flags & host_access_flags & parent.flags())<br>
> +            throw error (CL_INVALID_VALUE);<br>
> +<br>
<br>
This doesn't look correct to me, the condition will always evaluate to<br>
zero, you're calculating the conjunction of ~parent.flags() and<br>
parent.flags() which is zero, so the error will never be emitted.<br></blockquote></div><div><br></div><div>I'll see what I can do. I agree with a fresh reading that it looks fishy at best.</div><div><br></div><div>--Aaron</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>           return flags;<br>
>  <br>
>        } else {<br>
> -- <br>
> 2.14.1<br>
</blockquote></div>