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

[Daniel Kurtz]

On Tue, Feb 11, 2014 at 4:27 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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!

Hi Ken,

Thanks for taking a look at this patch.
I don't have rights to push to mesa, though, so hope someone with
rights can push.

I don't think I'll get a chance to write the follow on patch anytime soon.
I did the first one to fix a crash while doing full testing piglit
runs on some test hardware.
It would take a bit of effort to context switch back and expand this.

-Dan

>
> --Ken
>