[Mesa-dev] [PATCH v2 02/24] anv: do not try to ref/unref NULL shaders

[Iago Toral]

On Tue, 2017-03-14 at 10:22 +0200, Pohjolainen, Topi wrote:
> On Fri, Mar 10, 2017 at 01:38:15PM +0100, Iago Toral Quiroga wrote:
> > 
> > This situation can happen if we failed to allocate memory for the
> > shader.
> > 
> > v2:
> >  - We shouldn't see NULL shaders in anv_shader_bin_ref so we should
> > not check
> >    for that (Jason). Make sure that callers don't attempt to call
> > this
> >    function with a NULL shader and assert that this never happens
> > (Iago).
> > ---
> >  src/intel/vulkan/anv_pipeline_cache.c | 5 ++++-
> >  src/intel/vulkan/anv_private.h        | 5 ++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/intel/vulkan/anv_pipeline_cache.c
> > b/src/intel/vulkan/anv_pipeline_cache.c
> > index a8ea80f..d6c8413 100644
> > --- a/src/intel/vulkan/anv_pipeline_cache.c
> > +++ b/src/intel/vulkan/anv_pipeline_cache.c
> > @@ -308,7 +308,8 @@ anv_pipeline_cache_upload_kernel(struct
> > anv_pipeline_cache *cache,
> >        pthread_mutex_unlock(&cache->mutex);
> >  
> >        /* We increment refcount before handing it to the caller */
> > -      anv_shader_bin_ref(bin);
> > +      if (bin)
> > +         anv_shader_bin_ref(bin);
> >  
> >        return bin;
> >     } else {
> > @@ -546,6 +547,8 @@ VkResult anv_MergePipelineCaches(
> >        struct hash_entry *entry;
> >        hash_table_foreach(src->cache, entry) {
> >           struct anv_shader_bin *bin = entry->data;
> > +         assert(bin);
> > +
> >           if (_mesa_hash_table_search(dst->cache, bin->key))
> >              continue;
> >  
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index 27c1923..aa2b6d7 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1543,13 +1543,16 @@ anv_shader_bin_destroy(struct anv_device
> > *device, struct anv_shader_bin *shader)
> >  static inline void
> >  anv_shader_bin_ref(struct anv_shader_bin *shader)
> >  {
> > -   assert(shader->ref_cnt >= 1);
> > +   assert(shader && shader->ref_cnt >= 1);
> >     __sync_fetch_and_add(&shader->ref_cnt, 1);
> >  }
> >  
> >  static inline void
> >  anv_shader_bin_unref(struct anv_device *device, struct
> > anv_shader_bin *shader)
> >  {
> > +   if (!shader)
> > +      return;
> > +
> There are two callers in anv_pipeline.c: anv_DestroyPipeline and
> anv_pipeline_init. Both check against NULL before calling.
> 
> In anv_pipeline_cache.c::anv_pipeline_cache_finish() this is only
> called if
> item is found in cache.
> 
> Finally lookup_blorp_shader() and upload_blorp_shader() call this if
> and only
> if earlier reference is found in cache. Assert instead?

Sounds reasonable to me, I'll assert instead. Thanks!

Iago

> > 
> >     assert(shader->ref_cnt >= 1);
> >     if (__sync_fetch_and_add(&shader->ref_cnt, -1) == 1)
> >        anv_shader_bin_destroy(device, shader);