[Mesa-dev] [PATCH 5/8] glsl: clone builtin function constants

[Kenneth Graunke]

On Tuesday, August 8, 2017 8:34:06 PM PDT Timothy Arceri wrote:
> f81ede469910d fixed a problem with shaders including IR that was
> owned by builtins. However the approach of cloning the whole
> function each time we referenced it lead to a significant
> reduction in the GLSL IR compiler performance.
> 
> Everything was already cloned when inlining the function, as
> far as I can tell this is the only place where we are grabbing
> IR owned by the builtins without cloning it.
> 
> The double free can be easily reproduced by compiling the
> Deus Ex: Mankind Divided shaders with shader db, and then
> compiling them again after deleting mesa's shader cache
> index file. This will cause optimisations to never be performed
> on the IR and which presumably caused this issue to be hidden
> under normal circumstances.
> 
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> 
> Tested-by: Dieter Nützel <Dieter at nuetzel-hh.de>
> ---
>  src/compiler/glsl/ast_function.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
> index f7e90fba5b..73c4c0df7b 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -514,21 +514,29 @@ generate_call(exec_list *instructions, ir_function_signature *sig,
>      *           return 0 when evaluated inside an initializer with an argument
>      *           that is a constant expression."
>      *
>      * If the function call is a constant expression, don't generate any
>      * instructions; just generate an ir_constant.
>      */
>     if (state->is_version(120, 100)) {
>        ir_constant *value = sig->constant_expression_value(actual_parameters,
>                                                            NULL);

NAK on both 5 and 6.  Without cloning, "sig" is part of a global
"built-ins" memory context that's shared across multiple shaders.

Calling sig->constant_expression_value() will allocate new ir_constant
objects out of ralloc_parent(something in the same context as sig) - the
global context.  Ralloc is not thread-safe.  If you compile multiple
shaders concurrently, simply calling this will corrupt the linked list
in the ralloc context for built-ins, causing segmentation faults.

It's a really subtle bug, one that Lionel, Matt, and I spent a lot of
time tracking down.  Matt and I never figured it out - only Lionel
managed.  We were able to reproduce this by running shader-db on a
system with 36 cores / 72 threads.

I do dislike the cloning, though.  I think the best approach here would be
to make ::constant_expression_value() take a memory context, and allocate
all memory out of that, rather than ralloc_parent().  That way, we can make
generate_inline supply "ctx" here, so the new constant comes out of the
compile-local memory context.

With that changed, I think it would be safe to drop the cloning.

>        if (value != NULL) {
> -         return value;
> +         if (sig->is_builtin()) {
> +            /* This value belongs to a builtin so we must clone it to avoid
> +             * race conditions when freeing shaders and destorying the
> +             * context.
> +             */
> +            return value->clone(ctx, NULL);
> +         } else {
> +            return value;
> +         }
>        }
>     }
>  
>     ir_dereference_variable *deref = NULL;
>     if (!sig->return_type->is_void()) {
>        /* Create a new temporary to hold the return value. */
>        char *const name = ir_variable::temporaries_allocate_names
>           ? ralloc_asprintf(ctx, "%s_retval", sig->function_name())
>           : NULL;
>  
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170809/cdde0c57/attachment.sig>