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

[Timothy Arceri]

Hi Brian,

Yes you are right the case statement is not behaving as expected. Please
ignore my patch I think its about time I wrote a piglit test to see how
Nvidia and AMD binaries handle the difference in debug_output and
khr_debug as Ian suggested when I first implemented the extension now
that piglit can create debug contexts. We may be able to remove all this
extra code that currently separates the extensions in mesa.

Ian back in September you said:
> There's a Khronos face-to-face next week, and I'll raise this as a
bug.
>  KHR_debug really should have listed interactions with ARB_debug.

Did anything ever come of this? Or do I need to just rely on checking
what the binary drivers do?

Tim
 
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