From 623bc176fbb4af874c39d9b329fdebf412db716c Mon Sep 17 00:00:00 2001 From: Caio Oliveira Date: Thu, 11 May 2023 15:36:55 -0700 Subject: [PATCH] mesa/spirv: Provide more specific error message for glSpecializeShader() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Distinguish between the "entry point not found" and "parsing error" cases in the error text. For consistency, identify the unhandled specialization index case as part of the verification function. The verification function was renamed to make clearer its scope and what module it belongs. Reviewed-by: Tapani Pälli Part-of: --- src/compiler/spirv/gl_spirv.c | 20 +++++++++++------ src/compiler/spirv/nir_spirv.h | 14 +++++++++--- src/mesa/main/glspirv.c | 41 ++++++++++++++++++++-------------- 3 files changed, 48 insertions(+), 27 deletions(-) diff --git a/src/compiler/spirv/gl_spirv.c b/src/compiler/spirv/gl_spirv.c index 9b5e351c6c0..065a4f2d1b1 100644 --- a/src/compiler/spirv/gl_spirv.c +++ b/src/compiler/spirv/gl_spirv.c @@ -226,10 +226,11 @@ vtn_validate_handle_constant_instruction(struct vtn_builder *b, SpvOp opcode, * would need to trigger the specific errors. * */ -bool -gl_spirv_validation(const uint32_t *words, size_t word_count, - struct nir_spirv_specialization *spec, unsigned num_spec, - gl_shader_stage stage, const char *entry_point_name) +enum spirv_verify_result +spirv_verify_gl_specialization_constants( + const uint32_t *words, size_t word_count, + struct nir_spirv_specialization *spec, unsigned num_spec, + gl_shader_stage stage, const char *entry_point_name) { /* vtn_warn/vtn_log uses debug.func. Setting a null to prevent crash. Not * need to print the warnings now, would be done later, on the real @@ -248,7 +249,7 @@ gl_spirv_validation(const uint32_t *words, size_t word_count, /* See also _vtn_fail() */ if (vtn_setjmp(b->fail_jump)) { ralloc_free(b); - return false; + return SPIRV_VERIFY_PARSER_ERROR; } /* Skip the SPIR-V header, handled at vtn_create_builder */ @@ -260,7 +261,7 @@ gl_spirv_validation(const uint32_t *words, size_t word_count, if (b->entry_point == NULL) { ralloc_free(b); - return false; + return SPIRV_VERIFY_ENTRY_POINT_NOT_FOUND; } b->specializations = spec; @@ -274,6 +275,11 @@ gl_spirv_validation(const uint32_t *words, size_t word_count, ralloc_free(b); - return true; + for (unsigned i = 0; i < num_spec; i++) { + if (!spec[i].defined_on_module) + return SPIRV_VERIFY_UNKNOWN_SPEC_INDEX; + } + + return SPIRV_VERIFY_OK; } diff --git a/src/compiler/spirv/nir_spirv.h b/src/compiler/spirv/nir_spirv.h index eb7550dc0cf..debcf9acea0 100644 --- a/src/compiler/spirv/nir_spirv.h +++ b/src/compiler/spirv/nir_spirv.h @@ -124,9 +124,17 @@ struct spirv_to_nir_options { bool skip_os_break_in_debug_build; }; -bool gl_spirv_validation(const uint32_t *words, size_t word_count, - struct nir_spirv_specialization *spec, unsigned num_spec, - gl_shader_stage stage, const char *entry_point_name); +enum spirv_verify_result { + SPIRV_VERIFY_OK = 0, + SPIRV_VERIFY_PARSER_ERROR = 1, + SPIRV_VERIFY_ENTRY_POINT_NOT_FOUND = 2, + SPIRV_VERIFY_UNKNOWN_SPEC_INDEX = 3, +}; + +enum spirv_verify_result spirv_verify_gl_specialization_constants( + const uint32_t *words, size_t word_count, + struct nir_spirv_specialization *spec, unsigned num_spec, + gl_shader_stage stage, const char *entry_point_name); nir_shader *spirv_to_nir(const uint32_t *words, size_t word_count, struct nir_spirv_specialization *specializations, diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c index 48649a4a187..949962af877 100644 --- a/src/mesa/main/glspirv.c +++ b/src/mesa/main/glspirv.c @@ -329,7 +329,6 @@ _mesa_SpecializeShaderARB(GLuint shader, { GET_CURRENT_CONTEXT(ctx); struct gl_shader *sh; - bool has_entry_point; struct nir_spirv_specialization *spec_entries = NULL; if (!ctx->Extensions.ARB_gl_spirv) { @@ -384,27 +383,35 @@ _mesa_SpecializeShaderARB(GLuint shader, spec_entries[i].defined_on_module = false; } - has_entry_point = - gl_spirv_validation((uint32_t *)&spirv_data->SpirVModule->Binary[0], - spirv_data->SpirVModule->Length / 4, - spec_entries, numSpecializationConstants, - sh->Stage, pEntryPoint); + enum spirv_verify_result r = spirv_verify_gl_specialization_constants( + (uint32_t *)&spirv_data->SpirVModule->Binary[0], + spirv_data->SpirVModule->Length / 4, + spec_entries, numSpecializationConstants, + sh->Stage, pEntryPoint); - /* See previous spec comment */ - if (!has_entry_point) { + switch (r) { + case SPIRV_VERIFY_OK: + break; + case SPIRV_VERIFY_PARSER_ERROR: _mesa_error(ctx, GL_INVALID_VALUE, - "glSpecializeShaderARB(\"%s\" is not a valid entry point" + "glSpecializeShaderARB(failed to parse entry point \"%s\"" " for shader)", pEntryPoint); goto end; - } - - for (unsigned i = 0; i < numSpecializationConstants; ++i) { - if (spec_entries[i].defined_on_module == false) { - _mesa_error(ctx, GL_INVALID_VALUE, - "glSpecializeShaderARB(constant \"%i\" does not exist " - "in shader)", spec_entries[i].id); - goto end; + case SPIRV_VERIFY_ENTRY_POINT_NOT_FOUND: + _mesa_error(ctx, GL_INVALID_VALUE, + "glSpecializeShaderARB(could not find entry point \"%s\"" + " for shader)", pEntryPoint); + goto end; + case SPIRV_VERIFY_UNKNOWN_SPEC_INDEX: + for (unsigned i = 0; i < numSpecializationConstants; ++i) { + if (spec_entries[i].defined_on_module == false) { + _mesa_error(ctx, GL_INVALID_VALUE, + "glSpecializeShaderARB(constant \"%i\" does not exist " + "in shader)", spec_entries[i].id); + break; + } } + goto end; } spirv_data->SpirVEntryPoint = ralloc_strdup(spirv_data, pEntryPoint);