[Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)

[Chad Versace]

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 <eric.engestrom at intel.com>
> > 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 <eric.engestrom at intel.com>
> > > > > ---
> > > > >  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 =
> > > &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(...))).