[Mesa-dev] [PATCH v3] Fixing an x86 FPU bug.
From: Marius Predut marius.pre...@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
This patch is based Neil Roberts review.
This patch is more complete - it include changes on all places that are storing
attribute values to use gl_constant_value.
This patch fix 2 piglit tests.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668
Signed-off-by: Marius Predut marius.pre...@intel.com
---
src/mesa/main/context.c |3 ++-
src/mesa/main/macros.h| 41 +
src/mesa/vbo/vbo_attrib_tmp.h | 20
src/mesa/vbo/vbo_context.h| 16
src/mesa/vbo/vbo_exec.h | 11 ++-
src/mesa/vbo/vbo_exec_api.c | 39 +++
src/mesa/vbo/vbo_exec_draw.c |6 +++---
src/mesa/vbo/vbo_exec_eval.c | 22 +++---
src/mesa/vbo/vbo_save.h | 16
src/mesa/vbo/vbo_save_api.c | 34 +-
src/mesa/vbo/vbo_save_draw.c |6 +++---
11 files changed, 114 insertions(+), 100 deletions(-)
diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
index 63d30a2..f0597e2 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 = 0x1fff7fffULL;
diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
index cd5f2d6..2af15de 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,7 @@ do {\
V[3] = V3; \
} while(0)
+
/*@}*/
@@ -620,24 +621,24 @@ 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_UNION(gl_constant_value dst[4], int sz, const
gl_constant_value src[4],
GLenum type)
{
switch (type) {
case GL_FLOAT:
- ASSIGN_4V(dst, 0, 0, 0, 1);
+ ASSIGN_4V(dst, FLOAT_AS_UNION(0), FLOAT_AS_UNION(0), FLOAT_AS_UNION(0),
FLOAT_AS_UNION(1));
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(dst, INT_AS_UNION(0), INT_AS_UNION(0),
+INT_AS_UNION(0), INT_AS_UNION(1));
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(dst, UINT_AS_UNION(0), UINT_AS_UNION(0),
+UINT_AS_UNION(0), UINT_AS_UNION(1));
break;
default:
- ASSIGN_4V(dst, 0.0f, 0.0f, 0.0f, 1.0f); /* silence warnings */
- ASSERT(!Unexpected type in COPY_CLEAN_4V_TYPE_AS_FLOAT macro);
+ ASSIGN_4V(dst, FLOAT_AS_UNION(0), FLOAT_AS_UNION(0), FLOAT_AS_UNION(0),
FLOAT_AS_UNION(1)); /* silence warnings */
+ ASSERT(!Unexpected type in COPY_CLEAN_4V_TYPE_AS_UNION 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
+
+/*
Re: [Mesa-dev] [PATCH v3] Fixing an x86 FPU bug.
I have a few concerns about this patch.
Firstly, including prog_parameter.h from macros.h feels wrong. Macros
are a lower-level concept than program parameters so the lower thing
shouldn't be including the higher thing.
Instead of using gl_constant_value everywhere, why not use the fi_type
which is basically the same thing?
more below...
On 02/12/2015 11:00 AM, marius.pre...@intel.com wrote:
From: Marius Predut marius.pre...@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
This patch is based Neil Roberts review.
This patch is more complete - it include changes on all places that are storing
attribute values to use gl_constant_value.
This patch fix 2 piglit tests.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668
Signed-off-by: Marius Predut marius.pre...@intel.com
---
src/mesa/main/context.c |3 ++-
src/mesa/main/macros.h| 41 +
src/mesa/vbo/vbo_attrib_tmp.h | 20
src/mesa/vbo/vbo_context.h| 16
src/mesa/vbo/vbo_exec.h | 11 ++-
src/mesa/vbo/vbo_exec_api.c | 39 +++
src/mesa/vbo/vbo_exec_draw.c |6 +++---
src/mesa/vbo/vbo_exec_eval.c | 22 +++---
src/mesa/vbo/vbo_save.h | 16
src/mesa/vbo/vbo_save_api.c | 34 +-
src/mesa/vbo/vbo_save_draw.c |6 +++---
11 files changed, 114 insertions(+), 100 deletions(-)
diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
index 63d30a2..f0597e2 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;
I don't think this has to change if we just keep FLT_AS_UINT().
/* GL_ARB_sync */
consts-MaxServerWaitTimeout = 0x1fff7fffULL;
diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
index cd5f2d6..2af15de 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
See above.
/**
@@ -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,7 @@ do {\
V[3] = V3; \
} while(0)
+
Seems to be a few cases of unrelated whitespace changes.
/*@}*/
@@ -620,24 +621,24 @@ 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_UNION(gl_constant_value dst[4], int sz, const
gl_constant_value src[4],
GLenum type)
{
switch (type) {
case GL_FLOAT:
- ASSIGN_4V(dst, 0, 0, 0, 1);
+ ASSIGN_4V(dst, FLOAT_AS_UNION(0), FLOAT_AS_UNION(0), FLOAT_AS_UNION(0),
FLOAT_AS_UNION(1));
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(dst, INT_AS_UNION(0), INT_AS_UNION(0),
+INT_AS_UNION(0), INT_AS_UNION(1));
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(dst, UINT_AS_UNION(0), UINT_AS_UNION(0),
+UINT_AS_UNION(0), UINT_AS_UNION(1));
break;
default:
- ASSIGN_4V(dst, 0.0f, 0.0f, 0.0f, 1.0f); /* silence warnings */
-
