Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
On Thu 26 Jul 2018, Eric Engestrom wrote:
> On Thursday, 2018-07-26 02:03:58 -0700, Jason Ekstrand wrote:
> > On Thu, Jul 26, 2018 at 1:50 AM Eric Engestrom
> > wrote:
> >
> > > On Wednesday, 2018-07-25 14:00:29 -0700, Dylan Baker wrote:
> > > > Quoting Eric Engestrom (2018-07-25 11:45:56)
> > > > > CovID: 1438132
> > > > > Signed-off-by: Eric Engestrom
> > > > > ---
> > > > > src/intel/vulkan/anv_device.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/intel/vulkan/anv_device.c
> > > b/src/intel/vulkan/anv_device.c
> > > > > index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
> > > > > --- a/src/intel/vulkan/anv_device.c
> > > > > +++ b/src/intel/vulkan/anv_device.c
> > > > > @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
> > > > > const VkAllocationCallbacks*pAllocator)
> > > > > {
> > > > > ANV_FROM_HANDLE(anv_device, device, _device);
> > > > > - struct anv_physical_device *physical_device =
> > > >instance->physicalDevice;
> > > > > + struct anv_physical_device *physical_device;
> > > >
> > > > Is there a particular reason to create the pointer her but assign it
> > > after the
> > > > null check rather than just move the null check between the
> > > ANV_FROM_HANDLE and
> > > > the anv_pysical_device?
> > >
> > > Just the habit of always putting variable declarations before any logic
> > > ¯\_(ツ)_/¯
> > > I thought that was considered best-practice; has that changed?
> > >
> >
> > Yup; welcome to C99. :-)
>
> Haha, I know it's now allowed, but I thought best practice was to still
> keep things grouped, except when there's a reason to have a declaration
> in the middle of the logic.
>
> Looking at the rest of the file, I see a bunch of examples of logic and
> declarations interleaved, so I guess that rule doesn't apply in anv at
> least; guess I'll change my style to match :)
Even though we live in the future (whoo! 1999!), there are two cases
that I'm aware of that still require declaration-before-assignment.
Neither applies to this patch, though.
* A goto may force the divoce of declaration and assignment. For
example,
void foo(void) {
if (bad_func_fails())
goto fail;
int fd = open(...);
...;
return;
fail:
/* compiler warns of comparison of uninitialized value */
if (fd != -1)
close(fd);
}
* The same issue can happen when using __attribute__((cleanup(...))).
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
On Thu, Jul 26, 2018 at 2:26 AM Eric Engestrom
wrote:
> On Thursday, 2018-07-26 02:03:58 -0700, Jason Ekstrand wrote:
> > On Thu, Jul 26, 2018 at 1:50 AM Eric Engestrom >
> > wrote:
> >
> > > On Wednesday, 2018-07-25 14:00:29 -0700, Dylan Baker wrote:
> > > > Quoting Eric Engestrom (2018-07-25 11:45:56)
> > > > > CovID: 1438132
> > > > > Signed-off-by: Eric Engestrom
> > > > > ---
> > > > > src/intel/vulkan/anv_device.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/intel/vulkan/anv_device.c
> > > b/src/intel/vulkan/anv_device.c
> > > > > index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
> > > > > --- a/src/intel/vulkan/anv_device.c
> > > > > +++ b/src/intel/vulkan/anv_device.c
> > > > > @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
> > > > > const VkAllocationCallbacks*pAllocator)
> > > > > {
> > > > > ANV_FROM_HANDLE(anv_device, device, _device);
> > > > > - struct anv_physical_device *physical_device =
> > > >instance->physicalDevice;
> > > > > + struct anv_physical_device *physical_device;
> > > >
> > > > Is there a particular reason to create the pointer her but assign it
> > > after the
> > > > null check rather than just move the null check between the
> > > ANV_FROM_HANDLE and
> > > > the anv_pysical_device?
> > >
> > > Just the habit of always putting variable declarations before any logic
> > > ¯\_(ツ)_/¯
> > > I thought that was considered best-practice; has that changed?
> > >
> >
> > Yup; welcome to C99. :-)
>
> Haha, I know it's now allowed, but I thought best practice was to still
> keep things grouped, except when there's a reason to have a declaration
> in the middle of the logic.
>
> Looking at the rest of the file, I see a bunch of examples of logic and
> declarations interleaved, so I guess that rule doesn't apply in anv at
> least; guess I'll change my style to match :)
>
Yeah, I (and I think quite a few other people) prefer variables to be as
short-lived as possible. Mixing declarations to achieve that is something
we do at least in all the Intel and NIR code. Separation was required for
a while in core code because of VMWare's use of MSVC but now that they've
moved on to a MSVC version that has a competent C compiler, I think mixing
is allowed there too.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
On Thursday, 2018-07-26 02:03:58 -0700, Jason Ekstrand wrote:
> On Thu, Jul 26, 2018 at 1:50 AM Eric Engestrom
> wrote:
>
> > On Wednesday, 2018-07-25 14:00:29 -0700, Dylan Baker wrote:
> > > Quoting Eric Engestrom (2018-07-25 11:45:56)
> > > > CovID: 1438132
> > > > Signed-off-by: Eric Engestrom
> > > > ---
> > > > src/intel/vulkan/anv_device.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/intel/vulkan/anv_device.c
> > b/src/intel/vulkan/anv_device.c
> > > > index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
> > > > --- a/src/intel/vulkan/anv_device.c
> > > > +++ b/src/intel/vulkan/anv_device.c
> > > > @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
> > > > const VkAllocationCallbacks*pAllocator)
> > > > {
> > > > ANV_FROM_HANDLE(anv_device, device, _device);
> > > > - struct anv_physical_device *physical_device =
> > >instance->physicalDevice;
> > > > + struct anv_physical_device *physical_device;
> > >
> > > Is there a particular reason to create the pointer her but assign it
> > after the
> > > null check rather than just move the null check between the
> > ANV_FROM_HANDLE and
> > > the anv_pysical_device?
> >
> > Just the habit of always putting variable declarations before any logic
> > ¯\_(ツ)_/¯
> > I thought that was considered best-practice; has that changed?
> >
>
> Yup; welcome to C99. :-)
Haha, I know it's now allowed, but I thought best practice was to still
keep things grouped, except when there's a reason to have a declaration
in the middle of the logic.
Looking at the rest of the file, I see a bunch of examples of logic and
declarations interleaved, so I guess that rule doesn't apply in anv at
least; guess I'll change my style to match :)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
On 25/07/18 19:45, Eric Engestrom wrote:
CovID: 1438132
Signed-off-by: Eric Engestrom
Reviewed-by: Lionel Landwerlin
---
src/intel/vulkan/anv_device.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
const VkAllocationCallbacks*pAllocator)
{
ANV_FROM_HANDLE(anv_device, device, _device);
- struct anv_physical_device *physical_device =
>instance->physicalDevice;
+ struct anv_physical_device *physical_device;
if (!device)
return;
+ physical_device = >instance->physicalDevice;
+
anv_device_finish_blorp(device);
anv_pipeline_cache_finish(>default_pipeline_cache);
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
On Thu, Jul 26, 2018 at 1:50 AM Eric Engestrom
wrote:
> On Wednesday, 2018-07-25 14:00:29 -0700, Dylan Baker wrote:
> > Quoting Eric Engestrom (2018-07-25 11:45:56)
> > > CovID: 1438132
> > > Signed-off-by: Eric Engestrom
> > > ---
> > > src/intel/vulkan/anv_device.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_device.c
> b/src/intel/vulkan/anv_device.c
> > > index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
> > > --- a/src/intel/vulkan/anv_device.c
> > > +++ b/src/intel/vulkan/anv_device.c
> > > @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
> > > const VkAllocationCallbacks*pAllocator)
> > > {
> > > ANV_FROM_HANDLE(anv_device, device, _device);
> > > - struct anv_physical_device *physical_device =
> >instance->physicalDevice;
> > > + struct anv_physical_device *physical_device;
> >
> > Is there a particular reason to create the pointer her but assign it
> after the
> > null check rather than just move the null check between the
> ANV_FROM_HANDLE and
> > the anv_pysical_device?
>
> Just the habit of always putting variable declarations before any logic
> ¯\_(ツ)_/¯
> I thought that was considered best-practice; has that changed?
>
Yup; welcome to C99. :-)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
On Wednesday, 2018-07-25 14:00:29 -0700, Dylan Baker wrote:
> Quoting Eric Engestrom (2018-07-25 11:45:56)
> > CovID: 1438132
> > Signed-off-by: Eric Engestrom
> > ---
> > src/intel/vulkan/anv_device.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> > index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
> > --- a/src/intel/vulkan/anv_device.c
> > +++ b/src/intel/vulkan/anv_device.c
> > @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
> > const VkAllocationCallbacks*pAllocator)
> > {
> > ANV_FROM_HANDLE(anv_device, device, _device);
> > - struct anv_physical_device *physical_device =
> > >instance->physicalDevice;
> > + struct anv_physical_device *physical_device;
>
> Is there a particular reason to create the pointer her but assign it after the
> null check rather than just move the null check between the ANV_FROM_HANDLE
> and
> the anv_pysical_device?
Just the habit of always putting variable declarations before any logic
¯\_(ツ)_/¯
I thought that was considered best-practice; has that changed?
>
> >
> > if (!device)
> >return;
> >
> > + physical_device = >instance->physicalDevice;
> > +
> > anv_device_finish_blorp(device);
> >
> > anv_pipeline_cache_finish(>default_pipeline_cache);
> > --
> > Cheers,
> > Eric
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
Quoting Eric Engestrom (2018-07-25 11:45:56)
> CovID: 1438132
> Signed-off-by: Eric Engestrom
> ---
> src/intel/vulkan/anv_device.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
> const VkAllocationCallbacks*pAllocator)
> {
> ANV_FROM_HANDLE(anv_device, device, _device);
> - struct anv_physical_device *physical_device =
> >instance->physicalDevice;
> + struct anv_physical_device *physical_device;
Is there a particular reason to create the pointer her but assign it after the
null check rather than just move the null check between the ANV_FROM_HANDLE and
the anv_pysical_device?
>
> if (!device)
>return;
>
> + physical_device = >instance->physicalDevice;
> +
> anv_device_finish_blorp(device);
>
> anv_pipeline_cache_finish(>default_pipeline_cache);
> --
> Cheers,
> Eric
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
Reviewed-by: Jose Maria Casanova Crespo
El 25/07/18 a las 21:25, Eric Engestrom escribió:
> On Wednesday, 2018-07-25 19:45:56 +0100, Eric Engestrom wrote:
>> CovID: 1438132
>> Signed-off-by: Eric Engestrom
>
> Forgot to check before sending:
>
> Fixes: a99c9e63a07477634ab73 "anv: finish the binding_table_pool on
> destroyDevice when use_softpin"
> Cc: Jose Maria Casanova Crespo
>
>> ---
>> src/intel/vulkan/anv_device.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
>> index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
>> --- a/src/intel/vulkan/anv_device.c
>> +++ b/src/intel/vulkan/anv_device.c
>> @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
>> const VkAllocationCallbacks*pAllocator)
>> {
>> ANV_FROM_HANDLE(anv_device, device, _device);
>> - struct anv_physical_device *physical_device =
>> >instance->physicalDevice;
>> + struct anv_physical_device *physical_device;
>>
>> if (!device)
>>return;
>>
>> + physical_device = >instance->physicalDevice;>>
>> anv_device_finish_blorp(device);
>>
>> anv_pipeline_cache_finish(>default_pipeline_cache);
>> --
>> Cheers,
>> Eric
>>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
On Wednesday, 2018-07-25 19:45:56 +0100, Eric Engestrom wrote:
> CovID: 1438132
> Signed-off-by: Eric Engestrom
Forgot to check before sending:
Fixes: a99c9e63a07477634ab73 "anv: finish the binding_table_pool on
destroyDevice when use_softpin"
Cc: Jose Maria Casanova Crespo
> ---
> src/intel/vulkan/anv_device.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
> const VkAllocationCallbacks*pAllocator)
> {
> ANV_FROM_HANDLE(anv_device, device, _device);
> - struct anv_physical_device *physical_device =
> >instance->physicalDevice;
> + struct anv_physical_device *physical_device;
>
> if (!device)
>return;
>
> + physical_device = >instance->physicalDevice;
> +
> anv_device_finish_blorp(device);
>
> anv_pipeline_cache_finish(>default_pipeline_cache);
> --
> Cheers,
> Eric
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)
CovID: 1438132
Signed-off-by: Eric Engestrom
---
src/intel/vulkan/anv_device.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
const VkAllocationCallbacks*pAllocator)
{
ANV_FROM_HANDLE(anv_device, device, _device);
- struct anv_physical_device *physical_device =
>instance->physicalDevice;
+ struct anv_physical_device *physical_device;
if (!device)
return;
+ physical_device = >instance->physicalDevice;
+
anv_device_finish_blorp(device);
anv_pipeline_cache_finish(>default_pipeline_cache);
--
Cheers,
Eric
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
