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

[Pohjolainen, Topi]

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?

>     assert(shader->ref_cnt >= 1);
>     if (__sync_fetch_and_add(&shader->ref_cnt, -1) == 1)
>        anv_shader_bin_destroy(device, shader);
> -- 
> 2.7.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev