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

[Jose Fonseca]

On 24/04/18 17:27, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
> 
> 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 = &inEdges[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 <jfonseca at vmware.com>