[Mesa-stable] [Mesa-dev] [PATCH] mesa: add missing DebugMessageControl types

[Brian Paul]

On 02/27/2014 10:03 PM, Timothy Arceri wrote:
> Forgot to CC stable. This should apply to both 10 branches.
>
>
> On Sunday, 23 February 2014 9:55 PM, Timothy Arceri
> <t_arceri at yahoo.com.au> wrote:
> To make reviewing easier here is the relevant piece of the spec
>
> "Tokens accepted or provided by the <type> parameters of
> DebugMessageControl and DEBUGPROC, and the <types> parameter of
> GetDebugMessageLog:
>
>          DEBUG_TYPE_PUSH_GROUP                            0x8269
>          DEBUG_TYPE_POP_GROUP                            0x826A"
>
> On Wed, 2014-02-19 at 21:43 +1100, Timothy Arceri wrote:
>  > Signed-off-by: Timothy Arceri <t_arceri at yahoo.com.au
> <mailto:t_arceri at yahoo.com.au>>
>  > ---
>  >  src/mesa/main/errors.c | 5 +++++
>  >  1 file changed, 5 insertions(+)
>  >
>  > diff --git a/src/mesa/main/errors.c b/src/mesa/main/errors.c
>  > index 5f4eac6..c00c796 100644
>  > --- a/src/mesa/main/errors.c
>  > +++ b/src/mesa/main/errors.c
>  > @@ -575,6 +575,11 @@ validate_params(struct gl_context *ctx, unsigned
> caller,
>  >        /* this value is only valid for GL_KHR_debug functions */
>  >        if (caller == CONTROL || caller == INSERT)
>  >          break;
>  > +  case GL_DEBUG_TYPE_PUSH_GROUP:
>  > +  case GL_DEBUG_TYPE_POP_GROUP:
>  > +      /* this value is only valid for GL_KHR_debug */
>  > +      if (caller == CONTROL)
>  > +        break;
>  >    case GL_DONT_CARE:
>  >        if (caller == CONTROL || caller == CONTROL_ARB)
>  >          break;

I'm not sure the fall-through logic in these switch statements is correct.

For example, here:

    switch(type) {
    case GL_DEBUG_TYPE_ERROR_ARB:
    case GL_DEBUG_TYPE_DEPRECATED_BEHAVIOR_ARB:
    case GL_DEBUG_TYPE_UNDEFINED_BEHAVIOR_ARB:
    case GL_DEBUG_TYPE_PERFORMANCE_ARB:
    case GL_DEBUG_TYPE_PORTABILITY_ARB:
    case GL_DEBUG_TYPE_OTHER_ARB:
       break;
    case GL_DEBUG_TYPE_MARKER:
       /* this value is only valid for GL_KHR_debug functions */
       if (caller == CONTROL || caller == INSERT)
          break;
    case GL_DONT_CARE:
       if (caller == CONTROL || caller == CONTROL_ARB)
          break;
    default:
       goto error;
    }

Is type == GL_DEBUG_TYPE_MARKER and caller == CONTROL_ARB a legal 
combination??  From first glance at the code, it appears that it should 
be illegal.

But if type == GL_DEBUG_TYPE_MARKER and caller == CONTROL_ARB we'll 
fall-through to the GL_DONT_CARE case where the condition will be true 
and we break (thus not generating an error).

How about changing the code to add some else clauses (and comments):

    switch(type) {
    case GL_DEBUG_TYPE_ERROR_ARB:
    case GL_DEBUG_TYPE_DEPRECATED_BEHAVIOR_ARB:
    case GL_DEBUG_TYPE_UNDEFINED_BEHAVIOR_ARB:
    case GL_DEBUG_TYPE_PERFORMANCE_ARB:
    case GL_DEBUG_TYPE_PORTABILITY_ARB:
    case GL_DEBUG_TYPE_OTHER_ARB:
       /* legal */
       break;
    case GL_DEBUG_TYPE_MARKER:
       /* this value is only valid for GL_KHR_debug functions */
       if (caller == CONTROL || caller == INSERT)
          /* legal */
          break;
       else
          goto error;
    case GL_DONT_CARE:
       if (caller == CONTROL || caller == CONTROL_ARB)
          /* legal */
          break;
       else
          goto error;
    default:
       goto error;
    }

-Brian