[Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted

[Ilia Mirkin]

On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
>> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut <marius.predut at intel.com> wrote:
>>> Comparison with a signed expression and unsigned value
>>> is converted to unsigned value, reason for minus value is interpreted
>>> as a big unsigned value. For this case the "for" loop
>>> is going into unexpected behavior.
>>>
>>> v1: Brian Paul: code style fix.
>>> v2: Ian Romanick: glDrawArrays(GL_QUADS, 0, (n * 4) + k) fail , k < 4.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
>>> Signed-off-by: Marius Predut <marius.predut at intel.com>
>>> ---
>>>  src/mesa/tnl_dd/t_dd_dmatmp.h | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
>>> index 7be3954..f99d977 100644
>>> --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
>>> +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
>>> @@ -627,6 +627,13 @@ static void TAG(render_quads_verts)( struct gl_context *ctx,
>>>        LOCAL_VARS;
>>>        GLuint j;
>>>
>>> +      /* Page 18 (page 32 of the PDF) of the OpenGL 2.1 spec says:
>>> +       * The total number of vertices between Begin and End is 4n + k,
>>> +       * where 0 ≤ k ≤ 3; if k is not zero, the final k vertices are  ignored.
>>> +       */
>>> +      count = (count / 4) * 4;
>>
>> Might be just me, but I'd find
>>
>> count &= ~0x3
>>
>> to be a lot clearer. Don't know if the compiler can make such an
>> optimization.
>
> I think it can if count is unsigned.  Of course, GLsizei is not
> unsigned.  It is already invalid for count < 0, so your optimization is
> safe.

Actually count is a GLuint, so you're probably right that the compiler
can work it out. I definitely have to think about what it's doing
though, whereas with something like & ~3 it's pretty obvious. Perhaps
I've been in bit-land too long.

>
>> However this seems wrong... you're supposed to draw
>> start..count, so that's the value that has to be div-by-4. Further up,
>> when there's native quad support, the logic does:
>
> I don't think that's right.  Count is the number of vertices, not the
> index of the last vertex.  Calling
>
>     glDrawArrays(GL_QUADS, 47000, 4);
>
> still draws one quad.
>
> Look at the pseudocode on page 28 (page 42 of the PDF) of the OpenGL 2.1
> spec:
>
>     "The command
>
>         void DrawArrays(enum mode, int first, sizei count);
>
>     constructs a sequence of geometric primitives using elements first
>     through first + count − 1 of each enabled array. mode specifies
>     what kind of primitives are constructed; it accepts the same token
>     values as the mode parameter of the Begin command. The effect of
>
>         DrawArrays(mode, first, count);
>
>     is the same as the effect of the command sequence
>
>         if (mode or count is invalid)
>             generate appropriate error
>         else {
>             Begin(mode);
>             for (int i = 0; i < count; i++)
>                 ArrayElement(first + i);
>             End();
>         }"
>
> Combining that with the language previously quoted, I think this change
> is right.

Well, the code in question is

      for (j = start; j < count-3; j += 4) {
         void *tmp = ALLOC_VERTS( 6 );
         /* Send v0, v1, v3
          */
         tmp = EMIT_VERTS(ctx, j,     2, tmp);
         tmp = EMIT_VERTS(ctx, j + 3, 1, tmp);
         /* Send v1, v2, v3
          */
         tmp = EMIT_VERTS(ctx, j + 1, 3, tmp);
         (void) tmp;
      }

If count worked the way you're suggesting, then this would never work
for start != 0. I think "count" is really "end" in this case. Here is
one of the callers of this function:

tab[prim & PRIM_MODE_MASK]( ctx, start, start + length, prim );

The fact that the variable is called 'count' is actively misleading of
course, but that doesn't make the code any more right. The HAVE_QUADS
and HAVE_ELTS cases both have:

      count -= (count-start)%4;

which I believe further confirms my analysis.

  -ilia