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

[Emil Velikov]

On 13 September 2016 at 18:55, Adam Jackson <ajax at redhat.com> wrote:
> 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.
>
Hehe, the locking 'issue' mentioned in 09/14 is already upon us. I've
completely missed the lack of unlock here.

>> 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.
>
Right, had a bit of a brain freeze moment.

>> > +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.
>
Whichever works really. Just pointing out that using if/else chains
esp. when the else isn't needed makes things messy/less
appealing/etc..

Thanks
Emil