[Mesa-dev] [PATCH] r600: compare structure elements instead of doing a memcmp

[Roland Scheidegger]

Looks good to me. Although I wonder if it wouldn't be a good idea to
make sure it's actually all initialized (via memset). But I think that's
not quite consistently handled the same everywhere (that is, some things
are memset, some are not). (There might be similar bugs lurking
elswhere, but OTOH there also might be multiple places where it needs to
be initialized.)

Reviewed-by: Roland Scheidegger <sroland at vmware.com>



Am 01.07.2018 um 10:37 schrieb Gert Wollny:
> From: Gert Wollny <gert.wollny at collabora.com>
> 
> Structures might be padded by the compiler and these padding bytes remain
> un-initialized which in turn makes memcmp return a difference where from
> the logical point of view there is none.
> 
>  Fixes valgrind:
>      Conditional jump or move depends on uninitialised value(s)
>        at 0x4C32CBA: __memcmp_sse4_1 (vg_replace_strmem.c:1099)
>        by 0xB8D2537: r600_set_vertex_buffers (r600_state_common.c:573)
>        by 0xB71D44A: u_vbuf_set_driver_vertex_buffers (u_vbuf.c:1129)
>        by 0xB71F7BB: u_vbuf_draw_vbo (u_vbuf.c:1153)
>        by 0xB3B92CB: st_draw_vbo (st_draw.c:235)
>        by 0xB36B1AE: vbo_draw_arrays (vbo_exec_array.c:391)
>        by 0xB36BB0D: vbo_exec_DrawArrays (vbo_exec_array.c:550)
>        by 0x10A989: piglit_display (textureSize.c:157)
>        by 0x4F8F174: run_test (piglit_fbo_framework.c:52)
>        by 0x4F7BA12: piglit_gl_test_run (piglit-framework-gl.c:229)
>        by 0x10A60A: main (textureSize.c:71)
>      Uninitialised value was created by a stack allocation
>        at 0xB3948FD: st_update_array (st_atom_array.c:388)
> 
> Signed-off-by: Gert Wollny <gert.wollny at collabora.com>
> ---
>  src/gallium/drivers/r600/r600_state_common.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gallium/drivers/r600/r600_state_common.c b/src/gallium/drivers/r600/r600_state_common.c
> index 1e775e565b..939a14654e 100644
> --- a/src/gallium/drivers/r600/r600_state_common.c
> +++ b/src/gallium/drivers/r600/r600_state_common.c
> @@ -570,7 +570,10 @@ static void r600_set_vertex_buffers(struct pipe_context *ctx,
>  	/* Set vertex buffers. */
>  	if (input) {
>  		for (i = 0; i < count; i++) {
> -			if (memcmp(&input[i], &vb[i], sizeof(struct pipe_vertex_buffer))) {
> +			if ((input[i].buffer.resource != vb[i].buffer.resource) ||
> +			    (vb[i].stride != input[i].stride) ||
> +			    (vb[i].buffer_offset != input[i].buffer_offset) ||
> +			    (vb[i].is_user_buffer != input[i].is_user_buffer)) {
>  				if (input[i].buffer.resource) {
>  					vb[i].stride = input[i].stride;
>  					vb[i].buffer_offset = input[i].buffer_offset;
>