Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-12 Thread Emil Velikov 
On 8 December 2016 at 00:03, Edward O'Callaghan
 wrote:
> On 12/08/2016 02:53 AM, Emil Velikov wrote:
>> On 6 December 2016 at 22:34, Edward O'Callaghan
>>  wrote:
>>> On 12/07/2016 12:36 AM, Emil Velikov wrote:
 On 6 December 2016 at 11:30, Edward O'Callaghan
  wrote:
> As per the C spec, it is illegal to alias pointers to different
> types. This results in undefined behaviour after optimization
> passes, resulting in very subtle bugs that happen only on a
> full moon..
>
> Use a memcpy() as a well defined coercion between the double
> to uint64_t interpretations of the memory.
>
> V.2: Use static_assert() instead of assert().
>
 The lowercase static_assert is a c11 feature. You want to use the all
 caps version.
 See commit 23d1799f7dd5f8d1e8aa9f4efa6b1a4ed45faaa0
>>>
>>> Hi Emil,
>>>
>>> Why? I from that commit it seems only Android would have trouble however
>>> virgl nor svga would want to be enabled on that platform?
>>>
>>> Or is there something else I probably missed?
>>>
>> If there's a valid reason to require X or Y one must clearly point it
>> out, rather than jumping on bandwagons because it's cool/one can/etc.
>> Forcing X and Y [without justification] is disrespectful towards
>> everyone, even fellow developers. It also leads distro maintainers to
>> think that upstream does not give a sh*t about issues they are faced
>> with - old compilers, other.
>>
>> There are [hundreds of] thousands people using mesa many of which with
>> their own quirky requirements. Please consider that for future.
>> Esp. when it's a trivial change as this ;-)
>
> Emil, I understand all that and I had already sent a v3 with the
> required amendment before I replied here. As I am not trying to
> 'force'/'disrespect' anyone or anything, I asked purely to gain a better
> understanding of the reasoning behind requested change.
>
> I don't take things on gospel, no matter how high up the developer is or
> however trivial the change may be, as it is my change it is then my
> responsibility to fully understand any variation to the change I am
> proposing.
>
Pardon for going overboard there, Edward.
We've had enough cases where we (myself including) went ahead for the
new cool stuff and things ended up broken.

On the question:
Barring the point that we build virgl on Android, Vinson (and likely
others) use older versions of GCC and things tend to break every so
often.

It is a bit annoying, yet it's something which can be fixed easily.
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-07 Thread Edward O'Callaghan 


On 12/08/2016 02:53 AM, Emil Velikov wrote:
> On 6 December 2016 at 22:34, Edward O'Callaghan
>  wrote:
>> On 12/07/2016 12:36 AM, Emil Velikov wrote:
>>> On 6 December 2016 at 11:30, Edward O'Callaghan
>>>  wrote:
 As per the C spec, it is illegal to alias pointers to different
 types. This results in undefined behaviour after optimization
 passes, resulting in very subtle bugs that happen only on a
 full moon..

 Use a memcpy() as a well defined coercion between the double
 to uint64_t interpretations of the memory.

 V.2: Use static_assert() instead of assert().

>>> The lowercase static_assert is a c11 feature. You want to use the all
>>> caps version.
>>> See commit 23d1799f7dd5f8d1e8aa9f4efa6b1a4ed45faaa0
>>
>> Hi Emil,
>>
>> Why? I from that commit it seems only Android would have trouble however
>> virgl nor svga would want to be enabled on that platform?
>>
>> Or is there something else I probably missed?
>>
> If there's a valid reason to require X or Y one must clearly point it
> out, rather than jumping on bandwagons because it's cool/one can/etc.
> Forcing X and Y [without justification] is disrespectful towards
> everyone, even fellow developers. It also leads distro maintainers to
> think that upstream does not give a sh*t about issues they are faced
> with - old compilers, other.
> 
> There are [hundreds of] thousands people using mesa many of which with
> their own quirky requirements. Please consider that for future.
> Esp. when it's a trivial change as this ;-)

Emil, I understand all that and I had already sent a v3 with the
required amendment before I replied here. As I am not trying to
'force'/'disrespect' anyone or anything, I asked purely to gain a better
understanding of the reasoning behind requested change.

I don't take things on gospel, no matter how high up the developer is or
however trivial the change may be, as it is my change it is then my
responsibility to fully understand any variation to the change I am
proposing.

Kindest Regards,
Edward.

> 
> Thanks
> Emil
> 



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-07 Thread Emil Velikov 
On 6 December 2016 at 22:34, Edward O'Callaghan
 wrote:
> On 12/07/2016 12:36 AM, Emil Velikov wrote:
>> On 6 December 2016 at 11:30, Edward O'Callaghan
>>  wrote:
>>> As per the C spec, it is illegal to alias pointers to different
>>> types. This results in undefined behaviour after optimization
>>> passes, resulting in very subtle bugs that happen only on a
>>> full moon..
>>>
>>> Use a memcpy() as a well defined coercion between the double
>>> to uint64_t interpretations of the memory.
>>>
>>> V.2: Use static_assert() instead of assert().
>>>
>> The lowercase static_assert is a c11 feature. You want to use the all
>> caps version.
>> See commit 23d1799f7dd5f8d1e8aa9f4efa6b1a4ed45faaa0
>
> Hi Emil,
>
> Why? I from that commit it seems only Android would have trouble however
> virgl nor svga would want to be enabled on that platform?
>
> Or is there something else I probably missed?
>
If there's a valid reason to require X or Y one must clearly point it
out, rather than jumping on bandwagons because it's cool/one can/etc.
Forcing X and Y [without justification] is disrespectful towards
everyone, even fellow developers. It also leads distro maintainers to
think that upstream does not give a sh*t about issues they are faced
with - old compilers, other.

There are [hundreds of] thousands people using mesa many of which with
their own quirky requirements. Please consider that for future.
Esp. when it's a trivial change as this ;-)

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-06 Thread Dave Airlie 
On 7 December 2016 at 08:34, Edward O'Callaghan
 wrote:
>
>
> On 12/07/2016 12:36 AM, Emil Velikov wrote:
>> On 6 December 2016 at 11:30, Edward O'Callaghan
>>  wrote:
>>> As per the C spec, it is illegal to alias pointers to different
>>> types. This results in undefined behaviour after optimization
>>> passes, resulting in very subtle bugs that happen only on a
>>> full moon..
>>>
>>> Use a memcpy() as a well defined coercion between the double
>>> to uint64_t interpretations of the memory.
>>>
>>> V.2: Use static_assert() instead of assert().
>>>
>> The lowercase static_assert is a c11 feature. You want to use the all
>> caps version.
>> See commit 23d1799f7dd5f8d1e8aa9f4efa6b1a4ed45faaa0
>
> Hi Emil,
>
> Why? I from that commit it seems only Android would have trouble however
> virgl nor svga would want to be enabled on that platform?
>
> Or is there something else I probably missed?

That virgl is enabled on android?

Dave.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-06 Thread Edward O'Callaghan 


On 12/07/2016 12:36 AM, Emil Velikov wrote:
> On 6 December 2016 at 11:30, Edward O'Callaghan
>  wrote:
>> As per the C spec, it is illegal to alias pointers to different
>> types. This results in undefined behaviour after optimization
>> passes, resulting in very subtle bugs that happen only on a
>> full moon..
>>
>> Use a memcpy() as a well defined coercion between the double
>> to uint64_t interpretations of the memory.
>>
>> V.2: Use static_assert() instead of assert().
>>
> The lowercase static_assert is a c11 feature. You want to use the all
> caps version.
> See commit 23d1799f7dd5f8d1e8aa9f4efa6b1a4ed45faaa0

Hi Emil,

Why? I from that commit it seems only Android would have trouble however
virgl nor svga would want to be enabled on that platform?

Or is there something else I probably missed?

Kind Regards,
Edward.

> 
> We might want to use update the final vulkan [codebase] references, as well.
> 
> Thanks
> Emil
> 



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-06 Thread Emil Velikov 
On 6 December 2016 at 11:30, Edward O'Callaghan
 wrote:
> As per the C spec, it is illegal to alias pointers to different
> types. This results in undefined behaviour after optimization
> passes, resulting in very subtle bugs that happen only on a
> full moon..
>
> Use a memcpy() as a well defined coercion between the double
> to uint64_t interpretations of the memory.
>
> V.2: Use static_assert() instead of assert().
>
The lowercase static_assert is a c11 feature. You want to use the all
caps version.
See commit 23d1799f7dd5f8d1e8aa9f4efa6b1a4ed45faaa0

We might want to use update the final vulkan [codebase] references, as well.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-06 Thread Eric Engestrom 
On Tuesday, 2016-12-06 22:56:54 +1100, Edward O'Callaghan wrote:
> 
> 
> On 12/06/2016 10:48 PM, Eric Engestrom wrote:
> > On Tuesday, 2016-12-06 22:30:58 +1100, Edward O'Callaghan wrote:
> >> As per the C spec, it is illegal to alias pointers to different
> >> types. This results in undefined behaviour after optimization
> >> passes, resulting in very subtle bugs that happen only on a
> >> full moon..
> >>
> >> Use a memcpy() as a well defined coercion between the double
> >> to uint64_t interpretations of the memory.
> >>
> >> V.2: Use static_assert() instead of assert().
> >>
> >> Reviewed-by: Eric Engestrom 
> >> Signed-off-by: Edward O'Callaghan 
> >> ---
> >>  src/gallium/drivers/virgl/virgl_encode.c | 8 +++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/gallium/drivers/virgl/virgl_encode.c 
> >> b/src/gallium/drivers/virgl/virgl_encode.c
> >> index be72f70..58890df 100644
> >> --- a/src/gallium/drivers/virgl/virgl_encode.c
> >> +++ b/src/gallium/drivers/virgl/virgl_encode.c
> >> @@ -21,6 +21,8 @@
> >>   * USE OR OTHER DEALINGS IN THE SOFTWARE.
> >>   */
> >>  #include 
> >> +#include 
> >> +#include 
> >>  
> >>  #include "util/u_format.h"
> >>  #include "util/u_memory.h"
> >> @@ -315,12 +317,16 @@ int virgl_encode_clear(struct virgl_context *ctx,
> >>double depth, unsigned stencil)
> >>  {
> >> int i;
> >> +   uint64_t qword;
> >> +
> >> +   static_assert(sizeof(qword) == sizeof(depth), "");
> > 
> > Please fill in the string :)
> > eg.: "`depth` needs to be a 64-bit floating point type"
> 
> ACK, done locally.
> 
> You can check my branch rather than sending another version:
> 
> https://cgit.freedesktop.org/~funfunctor/mesa/log/?h=strict-aliasing-vioations
> 
> Good to go? I'll push once the svga one gets Rb'ed also.

All good on the virgl patch :)

svga is essentially the same, but you're using structs and bit-fields
I don't know, so I'll let someone else r-b it.
You wouldn't be introducing any *new* bug anyway, as the behaviour
wouldn't change (bar the static_assert).

Cheers,
  Eric

> 
> Thanks for the review.
> Kind Regards,
> Edward.
> 
> > 
> >> +   memcpy(, , sizeof(qword));
> >>  
> >> virgl_encoder_write_cmd_dword(ctx, VIRGL_CMD0(VIRGL_CCMD_CLEAR, 0, 
> >> VIRGL_OBJ_CLEAR_SIZE));
> >> virgl_encoder_write_dword(ctx->cbuf, buffers);
> >> for (i = 0; i < 4; i++)
> >>virgl_encoder_write_dword(ctx->cbuf, color->ui[i]);
> >> -   virgl_encoder_write_qword(ctx->cbuf, *(uint64_t *));
> >> +   virgl_encoder_write_qword(ctx->cbuf, qword);
> >> virgl_encoder_write_dword(ctx->cbuf, stencil);
> >> return 0;
> >>  }
> >> -- 
> >> 2.9.3
> >>
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-06 Thread Edward O'Callaghan 


On 12/06/2016 10:48 PM, Eric Engestrom wrote:
> On Tuesday, 2016-12-06 22:30:58 +1100, Edward O'Callaghan wrote:
>> As per the C spec, it is illegal to alias pointers to different
>> types. This results in undefined behaviour after optimization
>> passes, resulting in very subtle bugs that happen only on a
>> full moon..
>>
>> Use a memcpy() as a well defined coercion between the double
>> to uint64_t interpretations of the memory.
>>
>> V.2: Use static_assert() instead of assert().
>>
>> Reviewed-by: Eric Engestrom 
>> Signed-off-by: Edward O'Callaghan 
>> ---
>>  src/gallium/drivers/virgl/virgl_encode.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/drivers/virgl/virgl_encode.c 
>> b/src/gallium/drivers/virgl/virgl_encode.c
>> index be72f70..58890df 100644
>> --- a/src/gallium/drivers/virgl/virgl_encode.c
>> +++ b/src/gallium/drivers/virgl/virgl_encode.c
>> @@ -21,6 +21,8 @@
>>   * USE OR OTHER DEALINGS IN THE SOFTWARE.
>>   */
>>  #include 
>> +#include 
>> +#include 
>>  
>>  #include "util/u_format.h"
>>  #include "util/u_memory.h"
>> @@ -315,12 +317,16 @@ int virgl_encode_clear(struct virgl_context *ctx,
>>double depth, unsigned stencil)
>>  {
>> int i;
>> +   uint64_t qword;
>> +
>> +   static_assert(sizeof(qword) == sizeof(depth), "");
> 
> Please fill in the string :)
> eg.: "`depth` needs to be a 64-bit floating point type"

ACK, done locally.

You can check my branch rather than sending another version:

https://cgit.freedesktop.org/~funfunctor/mesa/log/?h=strict-aliasing-vioations

Good to go? I'll push once the svga one gets Rb'ed also.

Thanks for the review.
Kind Regards,
Edward.

> 
>> +   memcpy(, , sizeof(qword));
>>  
>> virgl_encoder_write_cmd_dword(ctx, VIRGL_CMD0(VIRGL_CCMD_CLEAR, 0, 
>> VIRGL_OBJ_CLEAR_SIZE));
>> virgl_encoder_write_dword(ctx->cbuf, buffers);
>> for (i = 0; i < 4; i++)
>>virgl_encoder_write_dword(ctx->cbuf, color->ui[i]);
>> -   virgl_encoder_write_qword(ctx->cbuf, *(uint64_t *));
>> +   virgl_encoder_write_qword(ctx->cbuf, qword);
>> virgl_encoder_write_dword(ctx->cbuf, stencil);
>> return 0;
>>  }
>> -- 
>> 2.9.3
>>



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-06 Thread Eric Engestrom 
On Tuesday, 2016-12-06 22:30:58 +1100, Edward O'Callaghan wrote:
> As per the C spec, it is illegal to alias pointers to different
> types. This results in undefined behaviour after optimization
> passes, resulting in very subtle bugs that happen only on a
> full moon..
> 
> Use a memcpy() as a well defined coercion between the double
> to uint64_t interpretations of the memory.
> 
> V.2: Use static_assert() instead of assert().
> 
> Reviewed-by: Eric Engestrom 
> Signed-off-by: Edward O'Callaghan 
> ---
>  src/gallium/drivers/virgl/virgl_encode.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gallium/drivers/virgl/virgl_encode.c 
> b/src/gallium/drivers/virgl/virgl_encode.c
> index be72f70..58890df 100644
> --- a/src/gallium/drivers/virgl/virgl_encode.c
> +++ b/src/gallium/drivers/virgl/virgl_encode.c
> @@ -21,6 +21,8 @@
>   * USE OR OTHER DEALINGS IN THE SOFTWARE.
>   */
>  #include 
> +#include 
> +#include 
>  
>  #include "util/u_format.h"
>  #include "util/u_memory.h"
> @@ -315,12 +317,16 @@ int virgl_encode_clear(struct virgl_context *ctx,
>double depth, unsigned stencil)
>  {
> int i;
> +   uint64_t qword;
> +
> +   static_assert(sizeof(qword) == sizeof(depth), "");

Please fill in the string :)
eg.: "`depth` needs to be a 64-bit floating point type"

> +   memcpy(, , sizeof(qword));
>  
> virgl_encoder_write_cmd_dword(ctx, VIRGL_CMD0(VIRGL_CCMD_CLEAR, 0, 
> VIRGL_OBJ_CLEAR_SIZE));
> virgl_encoder_write_dword(ctx->cbuf, buffers);
> for (i = 0; i < 4; i++)
>virgl_encoder_write_dword(ctx->cbuf, color->ui[i]);
> -   virgl_encoder_write_qword(ctx->cbuf, *(uint64_t *));
> +   virgl_encoder_write_qword(ctx->cbuf, qword);
> virgl_encoder_write_dword(ctx->cbuf, stencil);
> return 0;
>  }
> -- 
> 2.9.3
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v2 1/2] virgl: Fix a strict-aliasing violation in the encoder

2016-12-06 Thread Edward O'Callaghan 
As per the C spec, it is illegal to alias pointers to different
types. This results in undefined behaviour after optimization
passes, resulting in very subtle bugs that happen only on a
full moon..

Use a memcpy() as a well defined coercion between the double
to uint64_t interpretations of the memory.

V.2: Use static_assert() instead of assert().

Reviewed-by: Eric Engestrom 
Signed-off-by: Edward O'Callaghan 
---
 src/gallium/drivers/virgl/virgl_encode.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/gallium/drivers/virgl/virgl_encode.c 
b/src/gallium/drivers/virgl/virgl_encode.c
index be72f70..58890df 100644
--- a/src/gallium/drivers/virgl/virgl_encode.c
+++ b/src/gallium/drivers/virgl/virgl_encode.c
@@ -21,6 +21,8 @@
  * USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
 #include 
+#include 
+#include 
 
 #include "util/u_format.h"
 #include "util/u_memory.h"
@@ -315,12 +317,16 @@ int virgl_encode_clear(struct virgl_context *ctx,
   double depth, unsigned stencil)
 {
int i;
+   uint64_t qword;
+
+   static_assert(sizeof(qword) == sizeof(depth), "");
+   memcpy(, , sizeof(qword));
 
virgl_encoder_write_cmd_dword(ctx, VIRGL_CMD0(VIRGL_CCMD_CLEAR, 0, 
VIRGL_OBJ_CLEAR_SIZE));
virgl_encoder_write_dword(ctx->cbuf, buffers);
for (i = 0; i < 4; i++)
   virgl_encoder_write_dword(ctx->cbuf, color->ui[i]);
-   virgl_encoder_write_qword(ctx->cbuf, *(uint64_t *));
+   virgl_encoder_write_qword(ctx->cbuf, qword);
virgl_encoder_write_dword(ctx->cbuf, stencil);
return 0;
 }
-- 
2.9.3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev