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

[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 <eric.engestrom at imgtec.com>
> >> Signed-off-by: Edward O'Callaghan <funfunctor at folklore1984.net>
> >> ---
> >>  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 <stdint.h>
> >> +#include <assert.h>
> >> +#include <string.h>
> >>  
> >>  #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(&qword, &depth, 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 *)&depth);
> >> +   virgl_encoder_write_qword(ctx->cbuf, qword);
> >>     virgl_encoder_write_dword(ctx->cbuf, stencil);
> >>     return 0;
> >>  }
> >> -- 
> >> 2.9.3
> >>
>