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

[Kenneth Graunke]

On Friday, May 13, 2016 1:21:03 PM PDT Francisco Jerez wrote:
> 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.

This would still break in the case of:

    struct brw_reg foo = brw_imm_df(-1.0); // imm.df = 0xBFF0000000000000
    struct brw_reg bar = brw_imm_df(-2.0); // imm.df = 0xC000000000000000

    foo.type = BRW_REGISTER_TYPE_D;
    bar.type = BRW_REGISTER_TYPE_D;
    foo.f = 123;
    bar.f = 123;

Here, the values are the same, but the top 32 bits are different garbage.
Initialized, but irrelevant.

>  - 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.

I think this is the best approach.

> Any suggestions Matt?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160513/3555e67c/attachment-0001.sig>