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

[Kyle Brenneman]

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.

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.

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.