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 <kenn...@whitecape.org>
> Cc: Lionel Landwerlin <lionel.g.landwer...@intel.com>
>
> Tested-by: Dieter Nützel <die...@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;
>
>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
