[Mesa-dev] [PATCH 14/14] egl: Implement EGL_KHR_debug

[Adam Jackson]

On Tue, 2016-09-13 at 17:17 +0100, Emil Velikov wrote:

> > +      } else {
> > +         _eglDebugReportFull(EGL_BAD_ALLOC, __func__, __func__,
> > +               EGL_DEBUG_MSG_CRITICAL_KHR, NULL, NULL);
> > +         return EGL_BAD_ALLOC;
> > +      }
> 
> Nit: Please use the same style as the "objectType ==
> EGL_OBJECT_DISPLAY_KHR" case.

AFAICT the reason this code doesn't use RETURN_EGL_ERROR like
everything else is because it doesn't lock the display. Which is
extremely wrong, since we definitely depend on it not going away from
under us later! Fixed in v2.

> Nit: You can also drop the "else" and flatten (indent one level less)
> all of the following code.

Done in v2.

> Missing EGLAPIENTRY

Fixed in v2.

> > +eglDebugMessageControlKHR(EGLDEBUGPROCKHR callback,
> > +                         const EGLAttrib *attrib_list)
> > +{
> > +   unsigned int newEnabled;
> > +
> > +   _EGL_FUNC_START(NULL, EGL_NONE, NULL, EGL_BAD_ALLOC);
> > +
> > +   mtx_lock(_eglGlobal.Mutex);
> > +
> > +   newEnabled = _eglGlobal.debugTypesEnabled;
> > +   if (attrib_list != NULL) {
> > +      int i;
> > +
> > +      for (i = 0; attrib_list[i] != EGL_NONE; i += 2) {
> 
> Don't think we check it elsewhere (and/or if we should care too much) but still:
> Check if i overflows or use unsigned type ?

There's a bunch of places where we don't check that...

> > +         if (attrib_list[i] >= EGL_DEBUG_MSG_CRITICAL_KHR &&
> > +               attrib_list[i] <= EGL_DEBUG_MSG_INFO_KHR) {
> > +            if (attrib_list[i + 1]) {
> > +               newEnabled |= DebugBitFromType(attrib_list[i]);
> > +            } else {
> > +               newEnabled &= ~DebugBitFromType(attrib_list[i]);
> > +            }
> 
> Nit: break; ?

Nope. You're allowed to set the disposition for multiple error levels
in a single call to DebugMessageControl, so you need to validate them
all.

> > +eglQueryDebugKHR(EGLint attribute, EGLAttrib *value)
> > +{
> > +   _EGL_FUNC_START(NULL, EGL_NONE, NULL, EGL_BAD_ALLOC);
> > +
> > +   mtx_lock(_eglGlobal.Mutex);
> > +   if (attribute >= EGL_DEBUG_MSG_CRITICAL_KHR &&
> > +         attribute <= EGL_DEBUG_MSG_INFO_KHR) {
> > +      if (_eglGlobal.debugTypesEnabled & DebugBitFromType(attribute)) {
> > +         *value = EGL_TRUE;
> > +      } else {
> > +         *value = EGL_FALSE;
> > +      }
> > +   } else if (attribute == EGL_DEBUG_CALLBACK_KHR) {
> > +      *value = (EGLAttrib) _eglGlobal.debugCallback;
> > +   } else {
> > +      mtx_unlock(_eglGlobal.Mutex);
> > +      _eglReportError(EGL_BAD_ATTRIBUTE, NULL,
> > +              "Invalid attribute 0x%04lx", (unsigned long) attribute);
> > +      return EGL_FALSE;
> > +   }
> 
> Nit: Switch statement will be a lot easier to read.

Meh. I factored out the valid-debug-level check to a helper, at which
point you can't really use a switch. Redone as a do-while so I could
use break to bail out of the success conditions.

- ajax