[Mesa-dev] [PATCH 2/2] i965: initialize the alignment related bits in struct brw_reg

[Francisco Jerez]

Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:

> With the inclusion of the "df" field in the union, this union is going
> to be at the offset 8 because of the alignment rules. The alignment
> bits in the middle are uninitialized and valgrind complains with errors
> similar to this:
>
> ==10298== Conditional jump or move depends on uninitialised value(s)
> ==10298==    at 0x4C31D52: __memcmp_sse4_1 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==10298==    by 0xAB16663: backend_reg::equals(backend_reg const&) const (brw_shader.cpp:690)
> ==10298==    by 0xAAB629D: fs_reg::equals(fs_reg&) const (brw_fs.cpp:456)
> ==10298==    by 0xAAD4452: operands_match(fs_inst*, fs_inst*, bool*) (brw_fs_cse.cpp:161)
> ==10298==    by 0xAAD46C3: instructions_match(fs_inst*, fs_inst*, bool*) (brw_fs_cse.cpp:187)
> ==10298==    by 0xAAD4BAA: fs_visitor::opt_cse_local(bblock_t*) (brw_fs_cse.cpp:251)
> ==10298==    by 0xAAD5216: fs_visitor::opt_cse() (brw_fs_cse.cpp:361)
> ==10298==    by 0xAAC8AAD: fs_visitor::optimize() (brw_fs.cpp:5401)
> ==10298==    by 0xAACB9DC: fs_visitor::run_fs(bool) (brw_fs.cpp:5803)
> ==10298==    by 0xAACC38B: brw_compile_fs (brw_fs.cpp:6029)
> ==10298==    by 0xAA39796: brw_codegen_wm_prog (brw_wm.c:137)
> ==10298==    by 0xAA3B068: brw_fs_precompile (brw_wm.c:637)
>
> This patch adds an explicit padding and initializes it to zero.
>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> ---
>
> This patch replaces the following one:
>
> [PATCH 2/2] i965: check each field separately in backend_end::equals()
>
>  src/mesa/drivers/dri/i965/brw_reg.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
> index 3b76d7d..ebb7f29 100644
> --- a/src/mesa/drivers/dri/i965/brw_reg.h
> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
> @@ -243,6 +243,9 @@ struct brw_reg {
>     unsigned subnr:5;              /* :1 in align16 */
>     unsigned nr:16;
>  
> +   /* IMPORTANT: adjust padding bits if you add new fields */
> +   unsigned padding:32;
> +

Ugh!  It seems terribly fragile to me to make assumptions about the
amount of (implementation-defined) padding that you're going to end up
with.  It would be awful if someone builds the driver on a different
compiler or architecture that happens to align things differently, what
would cause the whole compiler back-end to behave non-deterministically
(possibly without any obvious sign of anything being wrong other than
decreased shader performance).  I think the two least insane
possibilities we have to fix the problem are:

 - memset() the whole struct at the top of brw_reg() and anywhere else a
   brw_reg struct is initialized.

 - Accept the reality that the struct contains some amount of undefined
   padding and define a helper function (e.g. brw_regs_equal() in
   brw_reg.h) to do the comparison manually, then use it everywhere we
   currently use memcmp() to compare brw_regs.

Any suggestions Matt?

>     union {
>        struct {
>           unsigned swizzle:8;      /* src only, align16 only */
> @@ -337,7 +340,7 @@ brw_reg(enum brw_reg_file file,
>     reg.pad0 = 0;
>     reg.subnr = subnr * type_sz(type);
>     reg.nr = nr;
> -
> +   reg.padding = 0;
>     /* Initialize all union's bits to zero before setting them. */
>     reg.df = 0;
>  
> -- 
> 2.5.0
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160513/fab3632e/attachment.sig>