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

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

[Hans de Goede]

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, };
 
    // The handles are created during exec_context::bind(), so we need make
    // sure to call exec_context::bind() before retrieving them.
-- 
2.7.2

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Samuel Pitoiset]

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.

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?

Thanks.

>
>      // The handles are created during exec_context::bind(), so we need make
>      // sure to call exec_context::bind() before retrieving them.
>

-- 
-Samuel
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Hans de Goede]

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  ", ...".

> 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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Samuel Pitoiset]

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.

>
>> 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

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

[Samuel Pitoiset]

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.

>
>>
>>> 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

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

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 2952 bytes --]

Samuel Pitoiset <samuel.pitoiset-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>   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:

Reviewed-by: Francisco Jerez <currojerez-sGOZH3hwPm2sTnJN9+BGXg@public.gmane.org>

>>
>>>
>>>> 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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Samuel Pitoiset]

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