From 9413c6aec3a7aef5af4eb7d7991fbff733b40823 Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Mon, 29 Mar 2021 12:32:57 -0700 Subject: [PATCH] mesa: Add anything dynamically indexed before any non-dynamically indexed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Things that are not dynamically indexed must be added last. This is necessary so that values that are both statically indexed (or used directly) and dynamically indexed will only be added once. With the above change, if the constant 47 is used as a literal in an instruction and in an array that is dynamically indexed, it will be added to `Parameters` twice. On (really old) GPUs that store constants and other parameters in the same storage, this can cause some valid programs to exceed the storage limits. I don't know about R300 or NV30, but R200 was limited to something like 256 vec4s. This applies to constants, state parameters, and local parameters (the assembly shader version of uniforms). The problem this causes here is that the final parameter layout created in `_mesa_layout_parameters` may have more parameters than the input layout. The fundamental assumption of that routine (and documented as an assumption of `copy_indirect_accessed_array`) is that the input size and the output size will be the same. The affected shader had something like below. This is a common pattern for ARB assembly shaders generated by NVIDIA's cgc compiler. As far as I can tell, the majory of applications that use ARB assembly shaders either use cgc or use some sort of DX9 crosscompiler... that generates similar patterns. PARAM c[141] = { program.local[0..133], { 255, 0.1, 3, 1 }, { 0.5, 2, 0.15915491, 0.25 }, { 0, 0.5, 1, -1 }, { 24.980801, -24.980801, -60.145809, 60.145809 }, { 85.453789, -85.453789, -64.939346, 64.939346 }, { 19.73921, -19.73921, -9, 0.75 }, { -999999 } }; The shader contains instructions like MUL R0.x, R0, c[135].y; and DP4 R2.z, c[A0.x + 6], R1; Starting with b9bff76b630, the constants at the end of `c` would get added to `Parameters` twice. The first time they are added due to instructions that directly access the array (e.g., the `c[135].y` above). The second time is because they are part of an array that is dynamically indexed. As a result, the final layout of Parameters (calculated by `_mesa_layout_parameters`) is 7 elements larger than the input layout. Since bcc61a01d49 fixed the allocation size of `ParameterValues`, `copy_indirect_accessed_array` will now write past the end of the array. The eventually results in a crash in `free`. Thankfully Valgrind was able to help find the real source of the problem. Reviewed-by: Marek Olšák Fixes: b9bff76b630 ("mesa: put constants before state vars for ARB programs") Closes: #4505 Part-of: --- src/mesa/program/prog_parameter_layout.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/mesa/program/prog_parameter_layout.c b/src/mesa/program/prog_parameter_layout.c index 181510748dc..fadee607bd5 100644 --- a/src/mesa/program/prog_parameter_layout.c +++ b/src/mesa/program/prog_parameter_layout.c @@ -160,10 +160,6 @@ _mesa_layout_parameters(struct asm_parser_state *state) for (inst = state->inst_head; inst != NULL; inst = inst->next) { for (unsigned i = 0; i < 3; i++) { if (inst->SrcReg[i].Base.RelAddr) { - unsigned begin = inst->SrcReg[i].Symbol->param_binding_begin; - if (state->prog->Parameters->Parameters[begin].Type != file) - continue; - /* Only attempt to add the to the new parameter list once. */ if (!inst->SrcReg[i].Symbol->pass1_done) { @@ -192,7 +188,11 @@ _mesa_layout_parameters(struct asm_parser_state *state) } } - /* PASS 2: Add sorted state variables. */ + /* PASS 2: Add sorted state variables. NOTE: This pass does **not** + * modify the instruction with the updated index. The sorting step + * might invalidate the index that was calculated by + * _mesa_add_state_reference. Instead, it relies on PASS 3 to do this. + */ if (file == PROGRAM_STATE_VAR) { unsigned first_state_var = layout->NumParameters;