Re: [Nouveau] [PATCH mesa] clover: Fix pipe_grid_info.indirect not being initialized

[Samuel Pitoiset <samuel.pitoiset@gmail.com>]

On 03/14/2016 09:49 PM, Francisco Jerez wrote:
> Samuel Pitoiset <samuel.pitoiset@gmail.com> writes:
>
>> On 03/14/2016 02:29 PM, Samuel Pitoiset wrote:
>>>
>>>
>>> On 03/14/2016 02:26 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 14-03-16 14:01, Samuel Pitoiset wrote:
>>>>>
>>>>>
>>>>> On 03/14/2016 01:50 PM, Hans de Goede wrote:
>>>>>> After pipe_grid_info.indirect was introduced, clover was not modified
>>>>>> to set it causing it to pass uninitialized memory for it to
>>>>>> launch_grid.
>>>>>>
>>>>>> This commit fixes this by zero-ing the entire pipe_grid_info struct
>>>>>> when
>>>>>> declaring it, to avoid similar problems popping-up in the future.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>>    src/gallium/state_trackers/clover/core/kernel.cpp | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp
>>>>>> b/src/gallium/state_trackers/clover/core/kernel.cpp
>>>>>> index 8396be9..dad66aa 100644
>>>>>> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
>>>>>> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
>>>>>> @@ -55,7 +55,7 @@ kernel::launch(command_queue &q,
>>>>>>       const auto reduced_grid_size =
>>>>>>          map(divides(), grid_size, block_size);
>>>>>>       void *st = exec.bind(&q, grid_offset);
>>>>>> -   struct pipe_grid_info info;
>>>>>> +   struct pipe_grid_info info = { 0, };
>>>>>
>>>>> Right, good catch, it's my fault.
>>>>>
>>>>> = { 0 }; is enough btw.
>>>>
>>>> I prefer to add the "," to make clear that we are initializing the
>>>> entire struct,
>>>> I read it as  ", ...".
>>>
>>> Well, usually we use { 0 } in mesa, try to grep and you will see. :-)
>>> There is only 3 occurrences of { 0, }, but I think they are quite old.
>>
>> Of course, I'm not really against this ",", but I just want consistency
>> with the other parts.
>>
>
> In C++ '{}' is standard, more concise, and works for a wider range of
> types regardless of their layout ('{ 0 }' is valid or not depending on
> what the first member of the struct is, while '{}' works regardless, in
> C++11 it can even be used to initialize non-POD types with custom
> constructors), so it should be generally preferred instead.
>
> Don't bother to resend just because of my nitpicking, I'll fix it up
> before I push the last revision of your change, which is:

I didn't know that, thanks for this very good explanation. :-)

>
> Reviewed-by: Francisco Jerez <currojerez@riseup.net>
>
>>>
>>>>
>>>>> This should be backported to mesa 11.2 I guess, could you please send
>>>>> a v2 with this minor fix and add the cc thing?
>>>>
>>>> Sure, as soon as we're done bikeshedding on the "," :)
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>
>>
>> --
>> -Samuel
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev