Re: [Mesa-dev] [PATCH 2/2] draw: fix different sign logic when clipping

[Jose Fonseca]

On 24/04/18 17:27, srol...@vmware.com wrote:

From: Roland Scheidegger 

The logic was flawed, since mul(x,y) will be <= 0 (exactly 0) when
the sign is the same but both numbers are sufficiently small
(if the product is smaller than 2^-128).
This could apparently lead to emitting a sufficient amount of
additional bogus vertices to overflow the allocated array for them,
hitting an assertion (still safe with release builds since we just
aborted clipping after the assertion in this case - I'm however unsure
if this is now really no longer possible, so that code stays).
Not sure if the additional vertices could cause other grief, I didn't
see anything wrong even when hitting the assertion.

Essentially, both +-0 are treated as positive (the vertex is considered
to be inside the clip volume for this plane), so integrate the logic
determining different sign into the branch there.
---
  src/gallium/auxiliary/draw/draw_pipe_clip.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c 
b/src/gallium/auxiliary/draw/draw_pipe_clip.c
index b7a1b5c..6af5c09 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
@@ -47,11 +47,6 @@
  /** Set to 1 to enable printing of coords before/after clipping */
  #define DEBUG_CLIP 0
  
-

-#ifndef DIFFERENT_SIGNS
-#define DIFFERENT_SIGNS(x, y) ((x) * (y) <= 0.0F && (x) - (y) != 0.0F)
-#endif
-
  #define MAX_CLIPPED_VERTICES ((2 * (6 + PIPE_MAX_CLIP_PLANES))+1)
  
  
@@ -479,6 +474,7 @@ do_clip_tri(struct draw_stage *stage,

for (i = 1; i <= n; i++) {
   struct vertex_header *vert = inlist[i];
   boolean *edge = [i];
+ boolean different_sign;
  
   float dp = getclipdist(clipper, vert, plane_idx);
  
@@ -491,9 +487,12 @@ do_clip_tri(struct draw_stage *stage,

 return;
  outEdges[outcount] = *edge_prev;
  outlist[outcount++] = vert_prev;
+different_sign = dp < 0.0f;
+ } else {
+different_sign = !(dp < 0.0f);
   }
  
- if (DIFFERENT_SIGNS(dp, dp_prev)) {

+ if (different_sign) {
  struct vertex_header *new_vert;
  boolean *new_edge;
  
@@ -511,7 +510,7 @@ do_clip_tri(struct draw_stage *stage,
  
  if (dp < 0.0f) {

 /* Going out of bounds.  Avoid division by zero as we
-* know dp != dp_prev from DIFFERENT_SIGNS, above.
+* know dp != dp_prev from different_sign, above.
  */
 float t = dp / (dp - dp_prev);
 interp( clipper, new_vert, t, vert, vert_prev, viewport_index 
);



Series looks good to me.

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

[Mesa-dev] [PATCH 2/2] draw: fix different sign logic when clipping

[sroland]

From: Roland Scheidegger 

The logic was flawed, since mul(x,y) will be <= 0 (exactly 0) when
the sign is the same but both numbers are sufficiently small
(if the product is smaller than 2^-128).
This could apparently lead to emitting a sufficient amount of
additional bogus vertices to overflow the allocated array for them,
hitting an assertion (still safe with release builds since we just
aborted clipping after the assertion in this case - I'm however unsure
if this is now really no longer possible, so that code stays).
Not sure if the additional vertices could cause other grief, I didn't
see anything wrong even when hitting the assertion.

Essentially, both +-0 are treated as positive (the vertex is considered
to be inside the clip volume for this plane), so integrate the logic
determining different sign into the branch there.
---
 src/gallium/auxiliary/draw/draw_pipe_clip.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c 
b/src/gallium/auxiliary/draw/draw_pipe_clip.c
index b7a1b5c..6af5c09 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
@@ -47,11 +47,6 @@
 /** Set to 1 to enable printing of coords before/after clipping */
 #define DEBUG_CLIP 0
 
-
-#ifndef DIFFERENT_SIGNS
-#define DIFFERENT_SIGNS(x, y) ((x) * (y) <= 0.0F && (x) - (y) != 0.0F)
-#endif
-
 #define MAX_CLIPPED_VERTICES ((2 * (6 + PIPE_MAX_CLIP_PLANES))+1)
 
 
@@ -479,6 +474,7 @@ do_clip_tri(struct draw_stage *stage,
   for (i = 1; i <= n; i++) {
  struct vertex_header *vert = inlist[i];
  boolean *edge = [i];
+ boolean different_sign;
 
  float dp = getclipdist(clipper, vert, plane_idx);
 
@@ -491,9 +487,12 @@ do_clip_tri(struct draw_stage *stage,
return;
 outEdges[outcount] = *edge_prev;
 outlist[outcount++] = vert_prev;
+different_sign = dp < 0.0f;
+ } else {
+different_sign = !(dp < 0.0f);
  }
 
- if (DIFFERENT_SIGNS(dp, dp_prev)) {
+ if (different_sign) {
 struct vertex_header *new_vert;
 boolean *new_edge;
 
@@ -511,7 +510,7 @@ do_clip_tri(struct draw_stage *stage,
 
 if (dp < 0.0f) {
/* Going out of bounds.  Avoid division by zero as we
-* know dp != dp_prev from DIFFERENT_SIGNS, above.
+* know dp != dp_prev from different_sign, above.
 */
float t = dp / (dp - dp_prev);
interp( clipper, new_vert, t, vert, vert_prev, viewport_index );
-- 
2.7.4

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