[Mesa-dev] [PATCH] clover: fix build failure since bfd695e

[Francisco Jerez]

Ilia Mirkin <imirkin at alum.mit.edu> writes:

> On Sat, Feb 13, 2016 at 12:01 PM, Serge Martin <edb+mesa at sigluy.net> wrote:
>> ---
>>  src/gallium/state_trackers/clover/core/kernel.cpp | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp b/src/gallium/state_trackers/clover/core/kernel.cpp
>> index 41b3852..a4ef2b1 100644
>> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
>> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
>> @@ -76,9 +76,10 @@ kernel::launch(command_queue &q,
>>                                exec.g_buffers.data(), g_handles.data());
>>
>>     // Fill information for the launch_grid() call.
>> -   info.block = pad_vector(q, block_size, 1).data(),
>> -   info.grid = pad_vector(q, reduced_grid_size, 1).data(),
>> -   info.pc = find(name_equals(_name), m.sysm).offset;
>> +   memcpy(info.block, pad_vector(q, block_size, 1).data(), sizeof(info.block));
>
> This is equivalent to
> int *x;
> {
>   std::vector foo = pad_vector(...);
>   x = foo.data();
> }
> use(x)
>
> Of course by the time you use x, the underlying vector backing it will
> have been freed. This was also a problem in the original code before
> Samuel ever touched it.

Nope, it wasn't, both Serge's and my original code were correct.  In C++
the lifetime of a temporary extends until the end of the topmost
lexically containing expression (i.e. the end of the memcpy() call in
Serge's patch and the end of the launch_grid() call in my original
code), so it's effectively equivalent to:

| {
|   int *x;
|   std::vector foo = pad_vector(...);
|   x = foo.data();
|   use(x)
| }

> You could do something like...
>
> {
>   std::vector<uint> padded = pad_vector(...);
>   memcpy(info.block, padded.data(), sizeof(info.block));
> }
>
> and then again for the second one. I couldn't come up with a cleaner
> alternative... std::copy() needs the begin/end iterators, which would
> also require a temp var.
>

That's precisely the reason clover::copy() exists (basically the same as
std::copy but more easily composable).  Serge, can we take v1 of your
patch and replace memcpy(v, u.data(), size(v)) with copy(u, v) for the
sake of type safety?  With that fixed:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

>> +   memcpy(info.grid, pad_vector(q, reduced_grid_size, 1).data(),
>> +                                                            sizeof(info.grid));
>> +   info.pc = find(name_equals(_name), m.syms).offset;
>>     info.input = exec.input.data();
>>
>>     q.pipe->launch_grid(q.pipe, &info);
>> --
>> 2.5.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160213/b51d7e90/attachment.sig>