Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes: > On 11/05/16 05:56, Francisco Jerez wrote: >> Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes: >> >>> From: Connor Abbott <connor.w.abb...@intel.com> >>> >>> v2 (Iago) >>> - Fixup accessibility in backend_reg >>> >>> Signed-off-by: Iago Toral Quiroga <ito...@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@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?= <sigles...@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 <sigles...@igalia.com>
> Reported-by: Francisco Jerez <curroje...@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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
