On 11/05/16 22:30, Francisco Jerez wrote:
> 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.
>
:-(
OK, I will fix it.
Thanks,
Sam
>> 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
