[Mesa-dev] [PATCH] glsl: Add locking to builtin_builder singleton

[Kenneth Graunke]

On 02/07/2014 03:29 AM, Daniel Kurtz wrote:
> Consider a multithreaded program with two contexts A and B, and the
> following scenario:
> 
> 1. Context A calls initialize(), which allocates mem_ctx and starts
>    building built-ins.
> 2. Context B calls initialize(), which sees mem_ctx != NULL and assumes
>    everything is already set up.  It returns.
> 3. Context B calls find(), which fails to find the built-in since it
>    hasn't been created yet.
> 4. Context A finally finishes initializing the built-ins.
> 
> This will break at step 3.  Adding a lock ensures that subsequent
> callers of initialize() will wait until initialization is actually
> complete.
> 
> Similarly, if any thread calls release while another thread is still
> initializing, or calling find(), the mem_ctx/shader would get free'd while
> from under it, leading to corruption or use-after-free crashes.
> 
> Fixes sporadic failures in Piglit's glx-multithread-shader-compile as
> reported in:
> https://bugs.freedesktop.org/69200
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Signed-off-by: Daniel Kurtz <djkurtz at chromium.org>
> ---
>  src/glsl/builtin_functions.cpp | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/builtin_functions.cpp b/src/glsl/builtin_functions.cpp
> index aeb8e5d..2162baa 100644
> --- a/src/glsl/builtin_functions.cpp
> +++ b/src/glsl/builtin_functions.cpp
> @@ -4100,6 +4100,7 @@ builtin_builder::_mid3(const glsl_type *type)
>  
>  /* The singleton instance of builtin_builder. */
>  static builtin_builder builtins;
> +_glthread_DECLARE_STATIC_MUTEX(builtins_lock);
>  
>  /**
>   * External API (exposing the built-in module to the rest of the compiler):
> @@ -4108,20 +4109,28 @@ static builtin_builder builtins;
>  void
>  _mesa_glsl_initialize_builtin_functions()
>  {
> +   _glthread_LOCK_MUTEX(builtins_lock);
>     builtins.initialize();
> +   _glthread_UNLOCK_MUTEX(builtins_lock);
>  }
>  
>  void
>  _mesa_glsl_release_builtin_functions()
>  {
> +   _glthread_LOCK_MUTEX(builtins_lock);
>     builtins.release();
> +   _glthread_UNLOCK_MUTEX(builtins_lock);
>  }
>  
>  ir_function_signature *
>  _mesa_glsl_find_builtin_function(_mesa_glsl_parse_state *state,
>                                   const char *name, exec_list *actual_parameters)
>  {
> -   return builtins.find(state, name, actual_parameters);
> +   ir_function_signature * s;
> +   _glthread_LOCK_MUTEX(builtins_lock);
> +   s = builtins.find(state, name, actual_parameters);
> +   _glthread_UNLOCK_MUTEX(builtins_lock);
> +   return s;
>  }
>  
>  gl_shader *
> 

This looks good to me.

Locking around initialize() and release() is definitely necessary to
prevent multiple threads from trying to create/tear down the data.

Locking around find() prevents a second thread from calling release()
and wrecking data that's in use.

We definitely do need more locking.  I believe the hash table usage in
glsl_types.cpp for creating new array or record types needs a similar
treatment.  Would you like to cook up a patch for that, Daniel?

In the meantime, I think we can push this.  Even if it's not complete, I
believe everything here is necessary.  Thanks for the patch!

--Ken

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140211/f85ac946/attachment.pgp>