Uninitialized padding in structs that are used as a key for hash maps
Recently I saw that in intel/blorp/blorp_clear.c
map keys had compiler inserted padding which due to how hashes are calculated (hash of a whole struct memory, not its members) may have had different hashes while having the same values.
E.g. struct:
struct blorp_mcs_partial_resolve_key
{
enum blorp_shader_type shader_type; // 32b
bool indirect_clear_color; // 8b
bool int_format; // 8b
// Compiler inserted padding // 16b
uint32_t num_samples; // 32b
};
And it was initialized:
const struct blorp_mcs_partial_resolve_key blorp_key = {
.shader_type = BLORP_SHADER_TYPE_MCS_PARTIAL_RESOLVE,
.indirect_clear_color = params->dst.clear_color_addr.buffer != NULL,
.int_format = isl_format_has_int_channel(params->dst.view.format),
.num_samples = params->num_samples,
};
Valgrind output (click me)
Conditional jump or move depends on uninitialised value(s)
util_fast_urem32 (fast_urem_by_const.h:71)
hash_table_search (hash_table.c:262)
_mesa_hash_table_search (hash_table.c:296)
anv_pipeline_cache_search_locked (anv_pipeline_cache.c:318)
anv_pipeline_cache_search (anv_pipeline_cache.c:335)
lookup_blorp_shader (anv_blorp.c:38)
blorp_params_get_mcs_partial_resolve_kernel (blorp_clear.c:1112)
blorp_mcs_partial_resolve (blorp_clear.c:1205)
anv_image_mcs_op (anv_blorp.c:1742)
anv_cmd_predicated_mcs_resolve (genX_cmd_buffer.c:774)
transition_color_buffer (genX_cmd_buffer.c:1159)
cmd_buffer_end_subpass (genX_cmd_buffer.c:4840)
Uninitialised value was created by a stack allocation
blorp_params_get_mcs_partial_resolve_kernel (blorp_clear.c:1103)
So the padding had undefined values. In !2552 (merged) we solved it with:
#pragma pack(push, 1)
...
#pragma pack(pop)
Which will eliminate the padding and prevent mistakes if new members will be added to struct.
However it's not the only place with such issue. In other places it is dealt by memset
struct to zero which works but generally does not guarantee to work (correct me if I'm wrong); also there are such structs with bit fields which I think could be viewed as a different case since force packing does not affect them (will memset
be enough for them? probably yes, but again - no guarantee). The biggest example of such struct is brw_wm_prog_key
.
Known for me places that almost certainly have issues with padding:
intel/blorp/blorp.c
:
struct brw_sf_prog_key {
uint64_t attrs;
bool contains_flat_varying;
unsigned char interp_mode[65]; /* BRW_VARYING_SLOT_COUNT */
uint8_t point_sprite_coord_replace;
enum brw_sf_primitive primitive:2;
bool do_twoside_color:1;
bool frontface_ccw:1;
bool do_point_sprite:1;
bool do_point_coord:1;
bool sprite_origin_lower_left:1;
bool userclip_active:1;
};
struct blorp_sf_key {
enum blorp_shader_type shader_type; /* Must be BLORP_SHADER_TYPE_GEN4_SF */
struct brw_sf_prog_key key;
};
...
struct blorp_sf_key key = {
.shader_type = BLORP_SHADER_TYPE_GEN4_SF,
};
gallium/drivers/zink/zink_framebuffer.h
:
struct zink_framebuffer_state {
struct zink_render_pass *rp;
uint32_t width;
uint16_t height, layers;
uint8_t num_attachments;
struct zink_surface *attachments[PIPE_MAX_COLOR_BUFS + 1];
};
...
struct zink_framebuffer_state state = {};
...
struct hash_entry *entry = _mesa_hash_table_search(ctx->framebuffer_cache,
&state);
The question is how should we deal with such cases?
We have:
- force packing of structs
- memset to zero
- manual padding - error prone
What about bit fields?
CC: @llandwerlin @kusma