[Mesa-dev] [PATCH 2/3] glsl: clone builtin function constants

[Nicolai Hähnle]

On 04.08.2017 09:25, 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>
> ---
>   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);
>         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.

*destroying

More importantly, this is not really a race condition, is it? More like 
use-after-free is possible if value is reparented.

Cheers,
Nicolai


> +             */
> +            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;
>   
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.