On 7 September 2017 at 19:21, Kyle Brenneman <kbrenne...@nvidia.com> wrote: > On 09/07/2017 11:56 AM, Emil Velikov wrote: >> >> On 7 September 2017 at 18:36, Kyle Brenneman <kbrenne...@nvidia.com> >> wrote: >>> >>> On 09/07/2017 10:03 AM, Emil Velikov wrote: >>>> >>>> From: Emil Velikov <emil.veli...@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 <kbrenne...@nvidia.com> >>>> Cc: Adam Jackson <a...@redhat.com> >>>> Signed-off-by: Emil Velikov <emil.veli...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev