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

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> 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.
>
Yeah, good point -- Let's go with the other approach in that case.

>>  - 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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160513/59aac94c/attachment.sig>