[Mesa-stable] [Mesa-dev] [PATCH v2] Fixing an x86 FPU bug.

Neil Roberts neil at linux.intel.com
Wed Feb 4 06:01:32 PST 2015


Hi Marius,

This patch does indeed make the Piglit test pass but it doesn't seem
like a complete fix. It looks like there are still a number of places
that are copying via floats that the test wouldn't catch. There are also
lots of places that are still using GLfloat to store the attribute
values and then these are cast to gl_constant_value* whenever they are
used in combination with the places that the patch does change. I think
if we wanted to do a complete patch then we should really change all
places that are storing attribute values to use gl_constant_value to
make it clear that they aren't really just floats.

If it is too much of a change just to fix this relatively minor bug then
perhaps it would be better to do a simpler change that just fixes this
one case using a memcpy or something instead of changing so much code. I
don't really like the idea of only changing some of the places to
gl_constant_value and leaving the rest as GLfloat because it seems even
more confusing that way.

I don't think it makes sense to add this patch to the stable branch. The
bug has presumably been there since the introduction of integer
attributes in 2012 (see acf438f537) and nobody has complained so it
doesn't seem particularly urgent. The patch is non-trivial so I think
there is a risk it will introduce more bugs.

I've made some comments inline below.

marius.predut at intel.com writes:

> From: Marius Predut <marius.predut at intel.com>
>
> On 32-bit, for floating point operations is used x86 FPU registers instead SSE,
> reason for  when reinterprets an integer as a float result is unexpected
> (modify floats when they are written to memory).
> The defect was checked with and without -O3 compiler flag.
>
> CC: <mesa-stable at lists.freedesktop.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668
> Signed-off-by: Marius Predut <marius.predut at intel.com>
> ---
>  src/mesa/main/context.c       |    3 ++-
>  src/mesa/main/macros.h        |   47 ++++++++++++++++++++++++-----------------
>  src/mesa/vbo/vbo_attrib_tmp.h |   20 ++++++++++++++----
>  src/mesa/vbo/vbo_exec.h       |    3 ++-
>  src/mesa/vbo/vbo_exec_api.c   |   31 +++++++++++++--------------
>  src/mesa/vbo/vbo_exec_eval.c  |   22 ++++++++++++++-----
>  src/mesa/vbo/vbo_save_api.c   |   16 +++++++-------
>  src/mesa/vbo/vbo_save_draw.c  |    4 ++--
>  8 files changed, 90 insertions(+), 56 deletions(-)
>
> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> index 400c158..11ab8a9 100644
> --- a/src/mesa/main/context.c
> +++ b/src/mesa/main/context.c
> @@ -134,6 +134,7 @@
>  #include "math/m_matrix.h"
>  #include "main/dispatch.h" /* for _gloffset_COUNT */
>  #include "uniforms.h"
> +#include "macros.h"
>  
>  #ifdef USE_SPARC_ASM
>  #include "sparc/sparc.h"
> @@ -656,7 +657,7 @@ _mesa_init_constants(struct gl_constants *consts, gl_api api)
>     consts->MaxSamples = 0;
>  
>     /* GLSL default if NativeIntegers == FALSE */
> -   consts->UniformBooleanTrue = FLT_AS_UINT(1.0f);
> +   consts->UniformBooleanTrue = FLOAT_AS_UNION(1.0f).u;
>  
>     /* GL_ARB_sync */
>     consts->MaxServerWaitTimeout = 0x1fff7fffffffULL;
> diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
> index cd5f2d6..2651ffc 100644
> --- a/src/mesa/main/macros.h
> +++ b/src/mesa/main/macros.h
> @@ -32,6 +32,7 @@
>  #define MACROS_H
>  
>  #include "imports.h"
> +#include "program/prog_parameter.h"
>  
>  
>  /**
> @@ -170,27 +171,26 @@ extern GLfloat _mesa_ubyte_to_float_color_tab[256];
>  	ub = ((GLubyte) F_TO_I((f) * 255.0F))
>  #endif
>  
> -static inline GLfloat INT_AS_FLT(GLint i)
> +static union gl_constant_value UINT_AS_UNION(GLuint u)
>  {
> -   fi_type tmp;
> -   tmp.i = i;
> -   return tmp.f;
> +   union gl_constant_value tmp;
> +   tmp.u = u;
> +   return tmp;
>  }
>  
> -static inline GLfloat UINT_AS_FLT(GLuint u)
> +static inline union gl_constant_value INT_AS_UNION(GLint i)
>  {
> -   fi_type tmp;
> -   tmp.u = u;
> -   return tmp.f;
> +   union gl_constant_value tmp;
> +   tmp.i = i;
> +   return tmp;
>  }
>  
> -static inline unsigned FLT_AS_UINT(float f)
> +static inline union gl_constant_value FLOAT_AS_UNION(GLfloat f)
>  {
> -   fi_type tmp;
> +   union gl_constant_value tmp;
>     tmp.f = f;
> -   return tmp.u;
> +   return tmp;
>  }
> -
>  /**
>   * Convert a floating point value to an unsigned fixed point value.
>   *
> @@ -382,6 +382,15 @@ do {                                    \
>      V[3] = V3;                          \
>  } while(0)
>  
> +/** Assignment union*/
> +#define ASSIGN_4V_UNION( V, V0, V1, V2, V3 )  \
> +do {                                    \
> +    V[0].f = V0;                          \
> +    V[1].f = V1;                          \
> +    V[2].f = V2;                          \
> +    V[3].f = V3;                          \
> +} while(0)
> +

I think it would be better not to have this macro and just use the
ASSIGN_4V macro to copy the gl_constant_value structs directly. This
macro is copying the union values via a floating-point operation which
is what we're trying to avoid. Technically it doesn't really matter in
this case because it's only used for fixed constant values but it seems
like a bad idea to introduce a problematic macro in case someone uses it
for other purposes later.

>  /*@}*/
>  
>  
> @@ -620,23 +629,23 @@ do {				\
>   * The default values are chosen based on \p type.
>   */
>  static inline void
> -COPY_CLEAN_4V_TYPE_AS_FLOAT(GLfloat dst[4], int sz, const GLfloat src[4],
> +COPY_CLEAN_4V_TYPE_AS_FLOAT(gl_constant_value dst[4], int sz, const gl_constant_value src[4],
>                              GLenum type)

It might make sense to change the name of this macro now that it doesn't
actually copy as float anymore.

>  {
>     switch (type) {
>     case GL_FLOAT:
> -      ASSIGN_4V(dst, 0, 0, 0, 1);
> +      ASSIGN_4V_UNION(dst, 0, 0, 0, 1);

This could use the ASSIGN_4V macro instead if all of the arguments were
wrapped in FLOAT_AS_UNION.

>        break;
>     case GL_INT:
> -      ASSIGN_4V(dst, INT_AS_FLT(0), INT_AS_FLT(0),
> -                     INT_AS_FLT(0), INT_AS_FLT(1));
> +      ASSIGN_4V_UNION(dst, INT_AS_UNION(0).f, INT_AS_UNION(0).f,
> +                INT_AS_UNION(0).f, INT_AS_UNION(1).f);

If the ASSIGN_4V macro was used instead then this could be the same but
without the .f members on the arguments.

>        break;
>     case GL_UNSIGNED_INT:
> -      ASSIGN_4V(dst, UINT_AS_FLT(0), UINT_AS_FLT(0),
> -                     UINT_AS_FLT(0), UINT_AS_FLT(1));
> +      ASSIGN_4V_UNION(dst, UINT_AS_UNION(0).f, UINT_AS_UNION(0).f,
> +                UINT_AS_UNION(0).f, UINT_AS_UNION(1).f);
>        break;
>     default:
> -      ASSIGN_4V(dst, 0.0f, 0.0f, 0.0f, 1.0f); /* silence warnings */
> +      ASSIGN_4V_UNION(dst, 0.0f, 0.0f, 0.0f, 1.0f); /* silence warnings */
>        ASSERT(!"Unexpected type in COPY_CLEAN_4V_TYPE_AS_FLOAT macro");
>     }
>     COPY_SZ_4V(dst, sz, src);
> diff --git a/src/mesa/vbo/vbo_attrib_tmp.h b/src/mesa/vbo/vbo_attrib_tmp.h
> index ec66934..17a0a10 100644
> --- a/src/mesa/vbo/vbo_attrib_tmp.h
> +++ b/src/mesa/vbo/vbo_attrib_tmp.h
> @@ -28,6 +28,18 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
>  #include "util/u_format_r11g11b10f.h"
>  #include "main/varray.h"
>  
> +
> +/* ATTR */
> +#define ATTR( A, N, T, V0, V1, V2, V3 )	  ATTR_##T((A), (N), (T), (V0), (V1), (V2), (V3))
> +
> +#define ATTR_GL_UNSIGNED_INT( A, N, T, V0, V1, V2, V3 ) \
> +    ATTR_UNION(A, N, T, UINT_AS_UNION(V0), UINT_AS_UNION(V1), UINT_AS_UNION(V2), UINT_AS_UNION(V3))
> +#define ATTR_GL_INT( A, N, T, V0, V1, V2, V3 ) \
> +    ATTR_UNION(A, N, T, INT_AS_UNION(V0), INT_AS_UNION(V1), INT_AS_UNION(V2), INT_AS_UNION(V3))
> +#define ATTR_GL_FLOAT( A, N, T, V0, V1, V2, V3 )	\
> +    ATTR_UNION(A, N, T, FLOAT_AS_UNION(V0), FLOAT_AS_UNION(V1), FLOAT_AS_UNION(V2), FLOAT_AS_UNION(V3))
> +
> +
>  /* float */
>  #define ATTR1FV( A, V ) ATTR( A, 1, GL_FLOAT, (V)[0], 0, 0, 1 )
>  #define ATTR2FV( A, V ) ATTR( A, 2, GL_FLOAT, (V)[0], (V)[1], 0, 1 )
> @@ -41,8 +53,8 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
>  
>  /* int */
>  #define ATTRI( A, N, X, Y, Z, W) ATTR( A, N, GL_INT, \
> -                                       INT_AS_FLT(X), INT_AS_FLT(Y), \
> -                                       INT_AS_FLT(Z), INT_AS_FLT(W) )
> +                                       X, Y, \
> +                                       Z, W )
>  
>  #define ATTR2IV( A, V ) ATTRI( A, 2, (V)[0], (V)[1], 0, 1 )
>  #define ATTR3IV( A, V ) ATTRI( A, 3, (V)[0], (V)[1], (V)[2], 1 )
> @@ -56,8 +68,8 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
>  
>  /* uint */
>  #define ATTRUI( A, N, X, Y, Z, W) ATTR( A, N, GL_UNSIGNED_INT, \
> -                                        UINT_AS_FLT(X), UINT_AS_FLT(Y), \
> -                                        UINT_AS_FLT(Z), UINT_AS_FLT(W) )
> +                                        X, Y, \
> +                                        Z, W )
>  
>  #define ATTR2UIV( A, V ) ATTRUI( A, 2, (V)[0], (V)[1], 0, 1 )
>  #define ATTR3UIV( A, V ) ATTRUI( A, 3, (V)[0], (V)[1], (V)[2], 1 )
> diff --git a/src/mesa/vbo/vbo_exec.h b/src/mesa/vbo/vbo_exec.h
> index bb265de..7db6b6b 100644
> --- a/src/mesa/vbo/vbo_exec.h
> +++ b/src/mesa/vbo/vbo_exec.h
> @@ -38,6 +38,7 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
>  #include "vbo.h"
>  #include "vbo_attrib.h"
>  
> +#include "program/prog_parameter.h"
>  
>  /**
>   * Max number of primitives (number of glBegin/End pairs) per VBO.
> @@ -104,7 +105,7 @@ struct vbo_exec_context
>        GLenum attrtype[VBO_ATTRIB_MAX];
>        GLubyte active_sz[VBO_ATTRIB_MAX];
>  
> -      GLfloat *attrptr[VBO_ATTRIB_MAX]; 
> +      gl_constant_value *attrptr[VBO_ATTRIB_MAX];
>        struct gl_client_array arrays[VERT_ATTRIB_MAX];
>  
>        /* According to program mode, the values above plus current
> diff --git a/src/mesa/vbo/vbo_exec_api.c b/src/mesa/vbo/vbo_exec_api.c
> index 5f8250e..a61efde 100644
> --- a/src/mesa/vbo/vbo_exec_api.c
> +++ b/src/mesa/vbo/vbo_exec_api.c
> @@ -46,7 +46,6 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
>  #include "vbo_context.h"
>  #include "vbo_noop.h"
>  
> -
>  #ifdef ERROR
>  #undef ERROR
>  #endif
> @@ -159,7 +158,7 @@ static void vbo_exec_copy_to_current( struct vbo_exec_context *exec )
>            * ctx->Current.Attrib and ctx->Light.Material.Attrib arrays.
>            */
>  	 GLfloat *current = (GLfloat *)vbo->currval[i].Ptr;
> -         GLfloat tmp[4];
> +         gl_constant_value tmp[4];
>  
>           COPY_CLEAN_4V_TYPE_AS_FLOAT(tmp,
>                                       exec->vtx.attrsz[i],
> @@ -216,10 +215,10 @@ vbo_exec_copy_from_current(struct vbo_exec_context *exec)
>     for (i = VBO_ATTRIB_POS + 1; i < VBO_ATTRIB_MAX; i++) {
>        const GLfloat *current = (GLfloat *) vbo->currval[i].Ptr;
>        switch (exec->vtx.attrsz[i]) {
> -      case 4: exec->vtx.attrptr[i][3] = current[3];
> -      case 3: exec->vtx.attrptr[i][2] = current[2];
> -      case 2: exec->vtx.attrptr[i][1] = current[1];
> -      case 1: exec->vtx.attrptr[i][0] = current[0];
> +      case 4: exec->vtx.attrptr[i][3].f = current[3];
> +      case 3: exec->vtx.attrptr[i][2].f = current[2];
> +      case 2: exec->vtx.attrptr[i][1].f = current[1];
> +      case 1: exec->vtx.attrptr[i][0].f = current[0];

This looks problematic because it is copying via a float. I think it
would be better to make current be a pointer to a gl_constant_value and
then cast vbo->currval[i].Ptr to that instead. That way you wouldn't
need to change the case statements and it wouldn't copy via a float.

>  	 break;
>        }
>     }
> @@ -291,7 +290,7 @@ vbo_exec_wrap_upgrade_vertex(struct vbo_exec_context *exec,
>  
>        for (i = 0 ; i < VBO_ATTRIB_MAX ; i++) {
>  	 if (exec->vtx.attrsz[i]) {
> -	    exec->vtx.attrptr[i] = tmp;
> +	    exec->vtx.attrptr[i] = (gl_constant_value*) tmp;

I think a more complete patch would be to also change
vbo_exec_context::vtx.vertex to be an array of gl_constant_value instead
of floats as that would make it more clear that integers may be stored
there. That way this tmp variable could also be a gl_constant_value and
we wouldn't need this cast.

>  	    tmp += exec->vtx.attrsz[i];
>  	 }
>  	 else
> @@ -305,8 +304,8 @@ vbo_exec_wrap_upgrade_vertex(struct vbo_exec_context *exec,
>     }
>     else {
>        /* Just have to append the new attribute at the end */
> -      exec->vtx.attrptr[attr] = exec->vtx.vertex +
> -	 exec->vtx.vertex_size - newSize;
> +      exec->vtx.attrptr[attr] = (gl_constant_value*)(exec->vtx.vertex +
> +        exec->vtx.vertex_size - newSize);

Same goes here, if exec->vtx.vertex were changed to be a
gl_constant_value then this cast wouldn't be necessary.

>     }
>  
>     /* Replay stored vertices to translate them
> @@ -327,15 +326,15 @@ vbo_exec_wrap_upgrade_vertex(struct vbo_exec_context *exec,
>  
>  	    if (sz) {
>  	       GLint old_offset = old_attrptr[j] - exec->vtx.vertex;
> -	       GLint new_offset = exec->vtx.attrptr[j] - exec->vtx.vertex;
> +	       GLint new_offset = (GLfloat*)exec->vtx.attrptr[j] - exec->vtx.vertex;

Same here.

>  
>  	       if (j == attr) {
>  		  if (oldSize) {
> -		     GLfloat tmp[4];
> +             gl_constant_value tmp[4];
>                       COPY_CLEAN_4V_TYPE_AS_FLOAT(tmp, oldSize,
> -                                                 data + old_offset,
> +                                                 (gl_constant_value*)(data + old_offset),

I think it would be good to change exec->vtx.copied.buffer and
exec->btx.buffer_ptr to be a gl_constant_value as well so that we could
avoid this cast.

>                                                   exec->vtx.attrtype[j]);
> -		     COPY_SZ_4V(dest + new_offset, newSize, tmp);
> +             COPY_SZ_4V(dest + new_offset, newSize, (GLfloat*)tmp);
>  		  } else {
>  		     GLfloat *current = (GLfloat *)vbo->currval[j].Ptr;
>  		     COPY_SZ_4V(dest + new_offset, sz, current);
> @@ -383,7 +382,7 @@ vbo_exec_fixup_vertex(struct gl_context *ctx, GLuint attr, GLuint newSize)
>         * zeros.  Don't need to flush or wrap.
>         */
>        for (i = newSize; i <= exec->vtx.attrsz[attr]; i++)
> -	 exec->vtx.attrptr[attr][i-1] = id[i-1];
> +        exec->vtx.attrptr[attr][i-1].f = id[i-1];

This is copying as a float so it looks like it will have the same bug.
We should probably change vbo_get_default_vals_as_float to be
vbo_get_default_vals_as_union or something and make it return a pointer
to const gl_constant_value *

>     }
>  
>     exec->vtx.active_sz[attr] = newSize;
> @@ -401,7 +400,7 @@ vbo_exec_fixup_vertex(struct gl_context *ctx, GLuint attr, GLuint newSize)
>   * This macro is used to implement all the glVertex, glColor, glTexCoord,
>   * glVertexAttrib, etc functions.
>   */
> -#define ATTR( A, N, T, V0, V1, V2, V3 )					\
> +#define ATTR_UNION( A, N, T, V0, V1, V2, V3 )					\
>  do {									\
>     struct vbo_exec_context *exec = &vbo_context(ctx)->exec;		\
>  									\
> @@ -412,7 +411,7 @@ do {									\
>        vbo_exec_fixup_vertex(ctx, A, N);					\
>     									\
>     {									\
> -      GLfloat *dest = exec->vtx.attrptr[A];				\
> +      gl_constant_value *dest = exec->vtx.attrptr[A];				\
>        if (N>0) dest[0] = V0;						\
>        if (N>1) dest[1] = V1;						\
>        if (N>2) dest[2] = V2;						\
> diff --git a/src/mesa/vbo/vbo_exec_eval.c b/src/mesa/vbo/vbo_exec_eval.c
> index 82f89b9..670e763 100644
> --- a/src/mesa/vbo/vbo_exec_eval.c
> +++ b/src/mesa/vbo/vbo_exec_eval.c
> @@ -33,6 +33,18 @@
>  #include "vbo_exec.h"
>  
>  
> +/** Copy \p SZ elements into a 4-element vector */
> +#define COPY_UNION_SZ_4V(DST, SZ, SRC)  \
> +do {                              \
> +   switch (SZ) {                  \
> +   case 4: (DST)[3].f = (SRC)[3];   \
> +   case 3: (DST)[2].f = (SRC)[2];   \
> +   case 2: (DST)[1].f = (SRC)[1];   \
> +   case 1: (DST)[0].f = (SRC)[0];   \
> +   }                              \
> +} while(0)
> +
> +

I think this macro could do with a better name because as it stands it
looks like it is just bringing back the problematic macro to copy via
floats. However I think a better idea would be to avoid the macro
altogether and do as described below.

>  static void clear_active_eval1( struct vbo_exec_context *exec, GLuint attr ) 
>  {
>     assert(attr < Elements(exec->eval.map1));
> @@ -138,9 +150,9 @@ void vbo_exec_do_EvalCoord1f(struct vbo_exec_context *exec, GLfloat u)
>  				   exec->eval.map1[attr].sz, 
>  				   map->Order);
>  
> -	 COPY_SZ_4V( exec->vtx.attrptr[attr],
> -		     exec->vtx.attrsz[attr],
> -		     data );
> +	 COPY_UNION_SZ_4V( exec->vtx.attrptr[attr],
> +             exec->vtx.attrsz[attr],
> +             data );

If we make data be an array of gl_constant_values then we can pass
&data[0].f to _math_horner_bezier_curve and then continue to use
COPY_SZ_4V.

>        }
>     }
>  
> @@ -186,7 +198,7 @@ void vbo_exec_do_EvalCoord2f( struct vbo_exec_context *exec,
>  				  exec->eval.map2[attr].sz, 
>  				  map->Uorder, map->Vorder);
>  
> -	 COPY_SZ_4V( exec->vtx.attrptr[attr],
> +	 COPY_UNION_SZ_4V( exec->vtx.attrptr[attr],
>  		     exec->vtx.attrsz[attr],
>  		     data );

Simiarly here _math_horner_bezier_surf could be passed &data[0].f.

>        }
> @@ -225,7 +237,7 @@ void vbo_exec_do_EvalCoord2f( struct vbo_exec_context *exec,
>           NORMALIZE_3FV(normal);
>  	 normal[3] = 1.0;
>  
> -	 COPY_SZ_4V( exec->vtx.attrptr[VBO_ATTRIB_NORMAL],
> + 	 COPY_UNION_SZ_4V( exec->vtx.attrptr[VBO_ATTRIB_NORMAL],
>  		     exec->vtx.attrsz[VBO_ATTRIB_NORMAL],
>  		     normal );

And here normal could be gl_constant_value and you could pass
&normal[0].f to CROSS3 and NORMALIZE_3FV.

>  
> diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
> index 5055c22..c24adde 100644
> --- a/src/mesa/vbo/vbo_save_api.c
> +++ b/src/mesa/vbo/vbo_save_api.c
> @@ -575,8 +575,8 @@ _save_copy_to_current(struct gl_context *ctx)
>     for (i = VBO_ATTRIB_POS + 1; i < VBO_ATTRIB_MAX; i++) {
>        if (save->attrsz[i]) {
>           save->currentsz[i][0] = save->attrsz[i];
> -         COPY_CLEAN_4V_TYPE_AS_FLOAT(save->current[i], save->attrsz[i],
> -                                     save->attrptr[i], save->attrtype[i]);
> +         COPY_CLEAN_4V_TYPE_AS_FLOAT((gl_constant_value*)save->current[i], save->attrsz[i],
> +                                     (gl_constant_value*)save->attrptr[i], save->attrtype[i]);

Again it would be more complete to change vbo_save_context::current and
attrptr to be pointers to gl_constant_value rather than casting it
everywhere.

>        }
>     }
>  }
> @@ -682,7 +682,7 @@ _save_upgrade_vertex(struct gl_context *ctx, GLuint attr, GLuint newsz)
>              if (save->attrsz[j]) {
>                 if (j == attr) {
>                    if (oldsz) {
> -                     COPY_CLEAN_4V_TYPE_AS_FLOAT(dest, oldsz, data,
> +                     COPY_CLEAN_4V_TYPE_AS_FLOAT((gl_constant_value*)dest, oldsz, (gl_constant_value*)data,

save->copied.buffer and save->buffer could be changed to a
gl_constant_value as well.

>                                                   save->attrtype[j]);
>                       data += oldsz;
>                       dest += newsz;
> @@ -769,7 +769,7 @@ _save_reset_vertex(struct gl_context *ctx)
>   * 3f version won't otherwise set color[3] to 1.0 -- this is the job
>   * of the chooser function when switching between Color4f and Color3f.
>   */
> -#define ATTR(A, N, T, V0, V1, V2, V3)				\
> +#define ATTR_UNION(A, N, T, V0, V1, V2, V3)				\
>  do {								\
>     struct vbo_save_context *save = &vbo_context(ctx)->save;	\
>  								\
> @@ -778,10 +778,10 @@ do {								\
>  								\
>     {								\
>        GLfloat *dest = save->attrptr[A];				\
> -      if (N>0) dest[0] = V0;					\
> -      if (N>1) dest[1] = V1;					\
> -      if (N>2) dest[2] = V2;					\
> -      if (N>3) dest[3] = V3;					\
> +      if (N>0) dest[0] = V0.f;					\
> +      if (N>1) dest[1] = V1.f;					\
> +      if (N>2) dest[2] = V2.f;					\
> +      if (N>3) dest[3] = V3.f;					\

This is copying via a float again. save->attrptr should be changed to a
gl_constant_value like you have already done for exec->attrptr.

>        save->attrtype[A] = T;                                    \
>     }								\
>  								\
> diff --git a/src/mesa/vbo/vbo_save_draw.c b/src/mesa/vbo/vbo_save_draw.c
> index d0521d7..d923403 100644
> --- a/src/mesa/vbo/vbo_save_draw.c
> +++ b/src/mesa/vbo/vbo_save_draw.c
> @@ -76,11 +76,11 @@ _playback_copy_to_current(struct gl_context *ctx,
>     for (i = VBO_ATTRIB_POS+1 ; i < VBO_ATTRIB_MAX ; i++) {
>        if (node->attrsz[i]) {
>  	 GLfloat *current = (GLfloat *)vbo->currval[i].Ptr;
> -         GLfloat tmp[4];
> +         gl_constant_value tmp[4];
>  
>           COPY_CLEAN_4V_TYPE_AS_FLOAT(tmp,
>                                       node->attrsz[i],
> -                                     data,
> +                                     (gl_constant_value*)data,
>                                       node->attrtype[i]);
>           

I guess it would also make sense to change
vbo_save_vertex_list->current_data to be a gl_constant_value so we can
avoid this cast too.

>           if (node->attrtype[i] != vbo->currval[i].Type ||
> -- 
> 1.7.9.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

- Neil


More information about the mesa-stable mailing list