[Mesa-dev] [PATCH 12/59] i965: add brw_imm_df

[Francisco Jerez]

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

> On 11/05/16 05:56, Francisco Jerez wrote:
>> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
>> 
>>> From: Connor Abbott <connor.w.abbott at intel.com>
>>>
>>> v2 (Iago)
>>>   - Fixup accessibility in backend_reg
>>>
>>> Signed-off-by: Iago Toral Quiroga <itoral at igalia.com>
>> 
>> I've just noticed (while running valgrind) that this patch causes
>> serious breakage in the back-end.  The reason is that the extra bits
>> required to make room for the df field of the union don't get
>> initialized in all codepaths, so backend_reg comparisons done using
>> memcmp() can basically return random results now.  Can you please look
>> into this?  Some ways to fix it would be to make sure we zero-initialize
>> the whole brw_reg in all cases (or at least the union padding), or stop
>> using memcmp() to compare registers -- I guess the latter might be
>> somewhat less intrusive and increase the likelihood that we can get this
>> sorted out timely.
>> 
>
> Attached is a patch for it, I initialized all union bits to zero before
> setting them in brw_reg(). Can you test it? If it is not fixed, Would
> you mind sending me an example to run it with valgrind here?
>
I'm afraid it's not fixed, I still see plenty of "Conditional jump or
move depends on uninitialised value(s)" errors while running pretty much
any piglit test on valgrind with the patch below applied.

> I am thinking that maybe we want to change backend_reg::equals() if this
> doesn't work.
>
> Sam
>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_reg.h    | 9 +++++++++
>>>  src/mesa/drivers/dri/i965/brw_shader.h | 1 +
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
>>> index b84c709..6d51623 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_reg.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
>>> @@ -254,6 +254,7 @@ struct brw_reg {
>>>           unsigned pad1:1;
>>>        };
>>>  
>>> +      double df;
>>>        float f;
>>>        int   d;
>>>        unsigned ud;
>>> @@ -544,6 +545,14 @@ brw_imm_reg(enum brw_reg_type type)
>>>  
>>>  /** Construct float immediate register */
>>>  static inline struct brw_reg
>>> +brw_imm_df(double df)
>>> +{
>>> +   struct brw_reg imm = brw_imm_reg(BRW_REGISTER_TYPE_DF);
>>> +   imm.df = df;
>>> +   return imm;
>>> +}
>>> +
>>> +static inline struct brw_reg
>>>  brw_imm_f(float f)
>>>  {
>>>     struct brw_reg imm = brw_imm_reg(BRW_REGISTER_TYPE_F);
>>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
>>> index fc228f6..f6f6167 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_shader.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
>>> @@ -90,6 +90,7 @@ struct backend_reg : private brw_reg
>>>     using brw_reg::width;
>>>     using brw_reg::hstride;
>>>  
>>> +   using brw_reg::df;
>>>     using brw_reg::f;
>>>     using brw_reg::d;
>>>     using brw_reg::ud;
>>> -- 
>>> 2.5.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> From 35254624d63b77aa2024bc2b08612e28cae4bb98 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Samuel=20Iglesias=20Gons=C3=A1lvez?= <siglesias at igalia.com>
> Date: Wed, 11 May 2016 07:44:10 +0200
> Subject: [PATCH] i965: initialize struct brw_reg's union bits to zero.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Extra bits required to make room for the df field of the union don't get
> initialized in all codepaths, so backend_reg comparisons done using
> memcmp() can basically return random results.
>
> Initialize them to zero before setting the rest of union's fields.
>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> Reported-by: Francisco Jerez <currojerez at riseup.net>
> ---
>  src/mesa/drivers/dri/i965/brw_reg.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
> index 6d51623..3b76d7d 100644
> --- a/src/mesa/drivers/dri/i965/brw_reg.h
> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
> @@ -338,6 +338,9 @@ brw_reg(enum brw_reg_file file,
>     reg.subnr = subnr * type_sz(type);
>     reg.nr = nr;
>  
> +   /* Initialize all union's bits to zero before setting them. */
> +   reg.df = 0;
> +
>     /* Could do better: If the reg is r5.3<0;1,0>, we probably want to
>      * set swizzle and writemask to W, as the lower bits of subnr will
>      * be lost when converted to align16.  This is probably too much to
> -- 
> 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/20160511/d2a43dbc/attachment-0001.sig>