[Mesa-dev] [PATCH 1/4] egl: simplify _eglDebugReport* API

[Emil Velikov]

On 7 September 2017 at 19:21, Kyle Brenneman <kbrenneman at nvidia.com> wrote:
> On 09/07/2017 11:56 AM, Emil Velikov wrote:
>>
>> On 7 September 2017 at 18:36, Kyle Brenneman <kbrenneman at nvidia.com>
>> wrote:
>>>
>>> On 09/07/2017 10:03 AM, Emil Velikov wrote:
>>>>
>>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>>
>>>> Instead of having three, almost identical but not quite,
>>>> _eglDebugReport* functions, simply fold them into one.
>>>>
>>>> While doing so drop the unnecessary arguments 'command' and
>>>> 'objectLabel'. Former is identical to funcName, while the latter is
>>>> already stored (yet unused) in _EGLThreadInfo::CurrentObjectLabel.
>>>>
>>>> Cc: Kyle Brenneman <kbrenneman at nvidia.com>
>>>> Cc: Adam Jackson <ajax at redhat.com>
>>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>>> ---
>>>>    src/egl/main/eglapi.c     |  3 +--
>>>>    src/egl/main/eglcurrent.c | 53
>>>> ++++++++++++-----------------------------------
>>>>    src/egl/main/eglcurrent.h |  7 -------
>>>>    3 files changed, 14 insertions(+), 49 deletions(-)
>>>>
>>> The command and funcName parameters aren't always identical. funcName is
>>> whatever string got passed to _eglError, which is often the name of some
>>> internal function. For instance, eglCreateWindowSurface can call
>>> _eglError
>>> with the string "_eglCreateWindowSurfaceCommon". A lot of the calls in
>>> the
>>> src/egl/drivers/ directory also use internal function names.
>>>
>>> I had both the command and fullName parameters in order to preserve the
>>> existing error reporting behavior (in _eglInternalError) in addition to
>>> the
>>> debug callback.
>>>
>>> Also, the reason I kept the command parameter at all instead of always
>>> using
>>> thr->CurrentFuncName is so that _eglSetFuncName could correctly report an
>>> EGL_BAD_ALLOC error if it couldn't allocate the _EGLThreadInfo struct in
>>> the
>>> first place. In that case, (thr->CurrentFuncName) would be NULL.
>>>
>>> You could change _eglError to pass NULL for the command name, in which
>>> case
>>> the debug callback would get the correct name in all cases, but then
>>> you'd
>>> lose the internal function name that would otherwise get passed to
>>> _eglLog.
>>
>> I agree on the _eglError front - it is a bit iffy, as described in 3/4
>> [1].
>> At the same time I cannot find an instance where there will be a
>> difference.
>>
>> Can you point me to one?
>>
> Ah, I hadn't read patch 3/4 yet when I typed my reply.
>
> Without patch 3, if the app calls eglCreateWindowSurface with NULL for the
> native window handle, then you'd run into that. eglCreateWindowSurface calls
> _eglCreateWindowSurfaceCommon, which calls the RETURN_EGL_ERROR macro if it
> hits an error, which in turn would pass the string
> "eglCreateWindowSurfaceCommon" to _eglError, and from there to the callback.
>
> Changing _eglError like you did in patch 3 would avoid that problem.
>
Right, my bad. I'll swap the patch order.

> However, I'm not sure how useful an internal function name by itself would
> be for an application developer, which is generally who the debug messages
> are aimed at.
>
Here is where the wonder of open-source comes handy :-P
The message makes it easier to track and resolve the problem... say
you cannot report the issue (for whatever reason).

Overall I agree though, messages kind of suck. Yet again that's seems
orthogonal to what the series does.

> Also, if you do use the msg parameter in eglError as the debug callback
> message, then it should probably use (..., "%s", msg), on the off-chance
> that some message ever has a printf character in it.
>
>
As/if we're to beat sense into _eglError "%s" would be very useful.
ATM nobody misuses the argument so we are safe.

Thanks
Emil