This seems reasonable, thanks for fixing. Maybe just add a comment in
the code also.
/* 12 * 4 == (max # of FS outputs) * max components */
Or something similar.
Reviewed-by: Timothy Arceri <tarc...@itsqueeze.com>
On 18/09/17 19:30, Nicolai Hähnle wrote:
From: Nicolai Hähnle <nicolai.haeh...@amd.com>
Prevent an overflow caused by too many output variables. To limit the
scope of the issue, write to the assigned array only for the non-ES
fragment shader path, which is the only place where it's needed.
Since the function will bail with an error when output variables with
overlapping components are found, (max # of FS outputs) * 4 is an upper
limit to the space we need.
Found by address sanitizer.
Fixes dEQP-GLES3.functional.attribute_location.bind_aliasing.*
Cc: mesa-sta...@lists.freedesktop.org
---
src/compiler/glsl/linker.cpp | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index 5c3f1d12bbc..ddd8301a739 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -2647,26 +2647,28 @@ assign_attribute_or_color_locations(void *mem_ctx,
{
const temp_attr *const l = (const temp_attr *) a;
const temp_attr *const r = (const temp_attr *) b;
/* Reversed because we want a descending order sort below. */
return r->slots - l->slots;
}
} to_assign[32];
assert(max_index <= 32);
- /* Temporary array for the set of attributes that have locations assigned.
+ /* Temporary array for the set of attributes that have locations assigned,
+ * for the purpose of checking overlapping slots/components of (non-ES)
+ * fragment shader outputs.
*/
- ir_variable *assigned[16];
+ ir_variable *assigned[12 * 4];
+ unsigned assigned_attr = 0;
unsigned num_attr = 0;
- unsigned assigned_attr = 0;
foreach_in_list(ir_instruction, node, sh->ir) {
ir_variable *const var = node->as_variable();
if ((var == NULL) || (var->data.mode != (unsigned) direction))
continue;
if (var->data.explicit_location) {
var->data.is_unmatched_generic_inout = 0;
if ((var->data.location >= (int)(max_index + generic_base))
@@ -2878,20 +2880,26 @@ assign_attribute_or_color_locations(void *mem_ctx,
if (assigned_component_mask & component_mask) {
linker_error(prog, "overlapping component is "
"assigned to %ss %s and %s "
"(component=%d)\n",
string, assigned[i]->name, var->name,
var->data.location_frac);
return false;
}
}
}
+
+ /* At most one variable per fragment output component should
+ * reach this. */
+ assert(assigned_attr < ARRAY_SIZE(assigned));
+ assigned[assigned_attr] = var;
+ assigned_attr++;
} else if (target_index == MESA_SHADER_FRAGMENT ||
(prog->IsES && prog->data->Version >= 300)) {
linker_error(prog, "overlapping location is assigned "
"to %s `%s' %d %d %d\n", string, var->name,
used_locations, use_mask, attr);
return false;
} else {
linker_warning(prog, "overlapping location is assigned "
"to %s `%s' %d %d %d\n", string, var->name,
used_locations, use_mask, attr);
@@ -2917,23 +2925,20 @@ assign_attribute_or_color_locations(void *mem_ctx,
*
* Mark this attribute slot as taking up twice as much space
* so we can count it properly against limits. According to
* issue (3) of the GL_ARB_vertex_attrib_64bit behavior, this
* is optional behavior, but it seems preferable.
*/
if (var->type->without_array()->is_dual_slot())
double_storage_locations |= (use_mask << attr);
}
- assigned[assigned_attr] = var;
- assigned_attr++;
-
continue;
}
if (num_attr >= max_index) {
linker_error(prog, "too many %s (max %u)",
target_index == MESA_SHADER_VERTEX ?
"vertex shader inputs" : "fragment shader outputs",
max_index);
return false;
}
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
