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 <kenn...@whitecape.org> > Signed-off-by: Daniel Kurtz <djku...@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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
