gallium-strict-aliasing branch merge

[Roland Scheidegger-4]

Hello,

I'm planning to merge gallium-strict-aliasing branch soon, which will
bring another gallium api change.
pipe_reference function has different arguments, because the old version
was pretty much not really useful for strict-aliasing compliant code
(util_color_pack functions also gets an update for the same reason).
The goal of course it to enable builds which do no longer need
-fno-strict-aliasing. scons builds already didn't do this (which was a
bug since the builds were indeed broken).
I didn't check all drivers for strict-aliasing compliance, but for
gallium everybody should make sure the code they are submitting is
according to strict aliasing rules (*). One downside of compiling with
-fno-strict-aliasing is also that you don't get the warnings wrt strict
aliasing, so you might have missed that in the past.
(There are no build system changes yet, there's still some strict
aliasing violating code in shader/grammar which should get replaced soon
anyway.)

(*) Strictly speaking, it looks like c99 actually has undefined
behaviour writing and reading different members of a union (wtf?), but
this is considered acceptable here, and all compilers should support it.

Roland

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

[Keith Whitwell-3]

Roland,

This looks OK to me, hopefully this will see us getting on top of strict
aliasing issues after all these years...

Keith

On Mon, 2009-12-07 at 18:14 -0800, Roland Scheidegger wrote:

> Hello,
>
> I'm planning to merge gallium-strict-aliasing branch soon, which will
> bring another gallium api change.
> pipe_reference function has different arguments, because the old version
> was pretty much not really useful for strict-aliasing compliant code
> (util_color_pack functions also gets an update for the same reason).
> The goal of course it to enable builds which do no longer need
> -fno-strict-aliasing. scons builds already didn't do this (which was a
> bug since the builds were indeed broken).
> I didn't check all drivers for strict-aliasing compliance, but for
> gallium everybody should make sure the code they are submitting is
> according to strict aliasing rules (*). One downside of compiling with
> -fno-strict-aliasing is also that you don't get the warnings wrt strict
> aliasing, so you might have missed that in the past.
> (There are no build system changes yet, there's still some strict
> aliasing violating code in shader/grammar which should get replaced soon
> anyway.)
>
> (*) Strictly speaking, it looks like c99 actually has undefined
> behaviour writing and reading different members of a union (wtf?), but
> this is considered acceptable here, and all compilers should support it.
>
> Roland
>
> ------------------------------------------------------------------------------
> Return on Information:
> Google Enterprise Search pays you back
> Get the facts.
> http://p.sf.net/sfu/google-dev2dev
> _______________________________________________
> Mesa3d-dev mailing list
> Mesa3d-dev@...
> https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

[Roland Scheidegger-4]

Keith,

I think there might be some slight issue with some of the changes in the
drivers I did. In particular, I was under the impression it would be ok
to do something like
union a_union {
  int i;
  double d;
};
int f() {
   double d = 3.0;
   return ((union a_union *) &d)->i;
};
but in fact gcc manpage tells me it's not (the example is from gcc 4.4
manpage) - this site told me this is ok, "casting through a union (2)"
http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html,
I guess it was considered ok in 2006 but not now (though I'm not sure
why not)... I did that in some places because otherwise there's no way
around assigning the value to the union and pass that around instead.
Curiously though, despite the gcc manpage saying the code "might" not be
ok, gcc doesn't warn about it in the places I used it.
Anyway, I'm not sure it's worth bothering with this now, as drivers
could be fixed up without any interface changes.

Roland


unsigned i =
On 08.12.2009 17:19, Keith Whitwell wrote:

> Roland,
>
> This looks OK to me, hopefully this will see us getting on top of strict
> aliasing issues after all these years...
>
> Keith
>
> On Mon, 2009-12-07 at 18:14 -0800, Roland Scheidegger wrote:
>> Hello,
>>
>> I'm planning to merge gallium-strict-aliasing branch soon, which will
>> bring another gallium api change.
>> pipe_reference function has different arguments, because the old version
>> was pretty much not really useful for strict-aliasing compliant code
>> (util_color_pack functions also gets an update for the same reason).
>> The goal of course it to enable builds which do no longer need
>> -fno-strict-aliasing. scons builds already didn't do this (which was a
>> bug since the builds were indeed broken).
>> I didn't check all drivers for strict-aliasing compliance, but for
>> gallium everybody should make sure the code they are submitting is
>> according to strict aliasing rules (*). One downside of compiling with
>> -fno-strict-aliasing is also that you don't get the warnings wrt strict
>> aliasing, so you might have missed that in the past.
>> (There are no build system changes yet, there's still some strict
>> aliasing violating code in shader/grammar which should get replaced soon
>> anyway.)
>>
>> (*) Strictly speaking, it looks like c99 actually has undefined
>> behaviour writing and reading different members of a union (wtf?), but
>> this is considered acceptable here, and all compilers should support it.
>>
>> Roland
>>
>> ------------------------------------------------------------------------------
>> Return on Information:
>> Google Enterprise Search pays you back
>> Get the facts.
>> http://p.sf.net/sfu/google-dev2dev
>> _______________________________________________
>> Mesa3d-dev mailing list
>> Mesa3d-dev@...
>> https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
>

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

[Jose Fonseca]

Roland,

Looks good to me too.

Jose

On Tue, 2009-12-08 at 08:19 -0800, Keith Whitwell wrote:

> Roland,
>
> This looks OK to me, hopefully this will see us getting on top of strict
> aliasing issues after all these years...
>
> Keith
>
> On Mon, 2009-12-07 at 18:14 -0800, Roland Scheidegger wrote:
> > Hello,
> >
> > I'm planning to merge gallium-strict-aliasing branch soon, which will
> > bring another gallium api change.
> > pipe_reference function has different arguments, because the old version
> > was pretty much not really useful for strict-aliasing compliant code
> > (util_color_pack functions also gets an update for the same reason).
> > The goal of course it to enable builds which do no longer need
> > -fno-strict-aliasing. scons builds already didn't do this (which was a
> > bug since the builds were indeed broken).
> > I didn't check all drivers for strict-aliasing compliance, but for
> > gallium everybody should make sure the code they are submitting is
> > according to strict aliasing rules (*). One downside of compiling with
> > -fno-strict-aliasing is also that you don't get the warnings wrt strict
> > aliasing, so you might have missed that in the past.
> > (There are no build system changes yet, there's still some strict
> > aliasing violating code in shader/grammar which should get replaced soon
> > anyway.)
> >
> > (*) Strictly speaking, it looks like c99 actually has undefined
> > behaviour writing and reading different members of a union (wtf?), but
> > this is considered acceptable here, and all compilers should support it.
> >
> > Roland
> >
> > ------------------------------------------------------------------------------
> > Return on Information:
> > Google Enterprise Search pays you back
> > Get the facts.
> > http://p.sf.net/sfu/google-dev2dev
> > _______________________________________________
> > Mesa3d-dev mailing list
> > Mesa3d-dev@...
> > https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
>
>
> ------------------------------------------------------------------------------
> Return on Information:
> Google Enterprise Search pays you back
> Get the facts.
> http://p.sf.net/sfu/google-dev2dev
> _______________________________________________
> Mesa3d-dev mailing list
> Mesa3d-dev@...
> https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

[Keith Whitwell-3]

On Tue, 2009-12-08 at 08:31 -0800, Roland Scheidegger wrote:

> Keith,
>
> I think there might be some slight issue with some of the changes in the
> drivers I did. In particular, I was under the impression it would be ok
> to do something like
> union a_union {
>   int i;
>   double d;
> };
> int f() {
>    double d = 3.0;
>    return ((union a_union *) &d)->i;
> };
> but in fact gcc manpage tells me it's not (the example is from gcc 4.4
> manpage) - this site told me this is ok, "casting through a union (2)"
> http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html,
> I guess it was considered ok in 2006 but not now (though I'm not sure
> why not)... I did that in some places because otherwise there's no way
> around assigning the value to the union and pass that around instead.
> Curiously though, despite the gcc manpage saying the code "might" not be
> ok, gcc doesn't warn about it in the places I used it.
> Anyway, I'm not sure it's worth bothering with this now, as drivers
> could be fixed up without any interface changes.

Is it a lot of extra work to fix?  I wouldn't mind getting on top of
this once and for all.

Keith


------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

[Roland Scheidegger-4]

On 08.12.2009 17:37, Keith Whitwell wrote:

> On Tue, 2009-12-08 at 08:31 -0800, Roland Scheidegger wrote:
>> Keith,
>>
>> I think there might be some slight issue with some of the changes in the
>> drivers I did. In particular, I was under the impression it would be ok
>> to do something like
>> union a_union {
>>   int i;
>>   double d;
>> };
>> int f() {
>>    double d = 3.0;
>>    return ((union a_union *) &d)->i;
>> };
>> but in fact gcc manpage tells me it's not (the example is from gcc 4.4
>> manpage) - this site told me this is ok, "casting through a union (2)"
>> http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html,
>> I guess it was considered ok in 2006 but not now (though I'm not sure
>> why not)... I did that in some places because otherwise there's no way
>> around assigning the value to the union and pass that around instead.
>> Curiously though, despite the gcc manpage saying the code "might" not be
>> ok, gcc doesn't warn about it in the places I used it.
>> Anyway, I'm not sure it's worth bothering with this now, as drivers
>> could be fixed up without any interface changes.
>
> Is it a lot of extra work to fix?  I wouldn't mind getting on top of
> this once and for all.

Not in the places I touched. It'll just make the code uglier, though at
least the compiler might still optimize extra assignments away.
For example in st_atom_pixeltransfer.c it now looks like this:
util_pack_color_ub(r, g, b, a, pt->format, (union util_color *)(dest + k));
and I'd need to change it to:
union util_color uc;
util_pack_color_ub(r, g, b, a, pt->format, &uc);
*(dest + k) = uc.ui;
Ok, not really a lot more ugly.
Will do this then, though there are other places where things like that
might already be used, and since the compiler does not issue any
warnings it might be a bit time consuming to find all of them.

Roland


------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

[Roland Scheidegger-4]

On 08.12.2009 18:12, Roland Scheidegger wrote:

> On 08.12.2009 17:37, Keith Whitwell wrote:
>> On Tue, 2009-12-08 at 08:31 -0800, Roland Scheidegger wrote:
>>> Keith,
>>>
>>> I think there might be some slight issue with some of the changes in the
>>> drivers I did. In particular, I was under the impression it would be ok
>>> to do something like
>>> union a_union {
>>>   int i;
>>>   double d;
>>> };
>>> int f() {
>>>    double d = 3.0;
>>>    return ((union a_union *) &d)->i;
>>> };
>>> but in fact gcc manpage tells me it's not (the example is from gcc 4.4
>>> manpage) - this site told me this is ok, "casting through a union (2)"
>>> http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html,
>>> I guess it was considered ok in 2006 but not now (though I'm not sure
>>> why not)... I did that in some places because otherwise there's no way
>>> around assigning the value to the union and pass that around instead.
>>> Curiously though, despite the gcc manpage saying the code "might" not be
>>> ok, gcc doesn't warn about it in the places I used it.
>>> Anyway, I'm not sure it's worth bothering with this now, as drivers
>>> could be fixed up without any interface changes.
>> Is it a lot of extra work to fix?  I wouldn't mind getting on top of
>> this once and for all.
>
> Not in the places I touched. It'll just make the code uglier, though at
> least the compiler might still optimize extra assignments away.
> For example in st_atom_pixeltransfer.c it now looks like this:
> util_pack_color_ub(r, g, b, a, pt->format, (union util_color *)(dest + k));
> and I'd need to change it to:
> union util_color uc;
> util_pack_color_ub(r, g, b, a, pt->format, &uc);
> *(dest + k) = uc.ui;
> Ok, not really a lot more ugly.
> Will do this then, though there are other places where things like that
> might already be used, and since the compiler does not issue any
> warnings it might be a bit time consuming to find all of them.

Ok, unfortunately code in vg_translate.c got a lot more verbose :-(.
Also, I think there's quite some usage of casting void * to other types.
That could also lead to strict-aliasing violations, as you're only
allowed to do casts back to the original type it had (hence the
strict-aliasing warnings if you do *((float *) (void *)
&some-uint-value), because the compiler is able to determine original
type). Might be safe though as long as gcc doesn't do too much
interprocedural optimizations, and if it does it should probably be able
to at least output a warning, since in this case it should also be able
to determine the original type I guess...

Roland

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

[Martin Olsson-10]

Roland Scheidegger wrote:

> Keith,
>
> I think there might be some slight issue with some of the changes in the
> drivers I did. In particular, I was under the impression it would be ok
> to do something like
> union a_union {
>   int i;
>   double d;
> };
> int f() {
>    double d = 3.0;
>    return ((union a_union *) &d)->i;
> };
> but in fact gcc manpage tells me it's not (the example is from gcc 4.4
> manpage)

I think the issue you are describing is explained here:
http://patrakov.blogspot.com/2009/03/dont-use-old-dtoac.html
Also note the link he posts to the GCC manual:
http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Optimize-Options.html#index-fstrict_002daliasing-721


                Martin

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

[Roland Scheidegger-4]

On 08.12.2009 20:57, Martin Olsson wrote:

> Roland Scheidegger wrote:
>> Keith,
>>
>> I think there might be some slight issue with some of the changes in the
>> drivers I did. In particular, I was under the impression it would be ok
>> to do something like
>> union a_union {
>>   int i;
>>   double d;
>> };
>> int f() {
>>    double d = 3.0;
>>    return ((union a_union *) &d)->i;
>> };
>> but in fact gcc manpage tells me it's not (the example is from gcc 4.4
>> manpage)
>
> I think the issue you are describing is explained here:
> http://patrakov.blogspot.com/2009/03/dont-use-old-dtoac.html

Yes, probably. Note though it says gcc generates warnings for it, which
didn't happen, so I think gcc would actually not miscompile it.
(I suspect gcc doesn't complain and does not miscompile as long as it
can't determine the original type of the value). Still, the explanation
is imho not really satisfactory. I think a lot of people used to think
this would be perfectly fine (see for instance
http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html
"casting through a union (2)").


> Also note the link he posts to the GCC manual:
> http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Optimize-Options.html#index-fstrict_002daliasing-721
Yep, that's the same stuff I used for the example.

Roland

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@...
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev