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.engest...@imgtec.com> > >> Signed-off-by: Edward O'Callaghan <funfunc...@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 > >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev