675532 - Add GLX Debug mode

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: GLX Debug Mode

When we get an X error from a GLX call, the stack is useless because of not having frame pointers in the GL binaries.

This adds a GLX debug mode that delays the assertion until after the call so we get readable callstacks.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 549698 [details] [diff] [review]
GLX Debug Mode

Review of attachment 549698 [details] [diff] [review]:
-----------------------------------------------------------------

Really great idea!! Having had this 6 months ago would have saved lots of time. It would be great to have that for all X calls but I don't know how one could override them all.

r- for implementation details only.

::: gfx/src/X11Util.h
@@ +125,5 @@
>   * not used on Mac, it should be easy to make it thread-safe by using thread-local storage with __thread.
>   */
>  class NS_GFX ScopedXErrorHandler
>  {
> +public:

As the comment says, this ErrorEvent struct only differs from XErrorEvent in that its constructor zeroes it out. Maybe not enough to justify using it. Anyway: see below, you don't even need to use your own X error handler as you can use a ScopedXErrorHandler directly from the BEFORE_GLX_CALL macro (as opposed to the BeforeGLXCall function).

::: gfx/thebes/GLContextProviderGLX.cpp
@@ +119,3 @@
>      LibrarySymbolLoader::SymLoadStruct symbols[] = {
>          /* functions that were in GLX 1.0 */
> +        { (PRFuncPtr*) &xDestroyContextInternal, { "glXDestroyContext", NULL } },

Have you considered applying the same idea that we did in GLContext, i.e. move the function pointers to a separate struct, so that instead of FooInternal you have mSymbols.Foo?

Not requiring this at all, just asking if there's a specific reason not to do so. also 'symbols' has the merit of being more specific than 'internal', and for GLContext we've found it convenient to have all the function pointers in a separate struct so we could easily zero them out with a single memset call in case of initialization errors, to avoid hard-to-figure crashes.

@@ +348,5 @@
> +#ifdef DEBUG
> +
> +static int (*sOldErrorHandler)(Display *, XErrorEvent *);
> +ScopedXErrorHandler::ErrorEvent sErrorEvent;
> +static int GLXErrorHandler(Display *display, XErrorEvent *ev)

Actually, you can do without using your own X error handler here. You can declare a ScopedXErrorHandler directly in the body of the BEFORE_GLX_CALL macro, and then in AFTER_GLX_CALL you can call the SyncAndGetError method on it.

@@ +369,5 @@
> +GLXLibrary::AfterGLXCall()
> +{
> +    if (mDebug) {
> +        XSync(DefaultXDisplay(), False);
> +        NS_ABORT_IF_FALSE(!sErrorEvent.mError.error_code, "GLX caused an X error!");

Really here we should print all the fields of the XErrorEvent, not just error_code. Again, following my suggestion to do that using ScopedXErrorHandler, that would go to the AFTER_GLX_CALL macro body.

Comment 2

[Matt Woodrow (:mattwoodrow)]

XScopedErrorEvent changes the X error handler in it's constructor. I wanted to avoid doing this, unless the env var was set. I'm not sure if this is a real concern, I can happily change it if not.

I don't have any real problems with the mSymbols approach, other than not wanting to type it all out :)

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> XScopedErrorEvent changes the X error handler in it's constructor. I wanted
> to avoid doing this, unless the env var was set. I'm not sure if this is a
> real concern, I can happily change it if not.

Oh OK I didn't realize that issue. Now I don't have a preference either way anymore.

> I don't have any real problems with the mSymbols approach, other than not
> wanting to type it all out :)

OK, don't want to make you spend more time on that.

Please address the remaining couple review comments.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: GLX Debug Mode v2

Added printing of the XErrorEvent

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 554281 [details] [diff] [review]
GLX Debug Mode v2

OK. I also notice that in this new version the env var becomes MOZ_GLX_DEBUG instead of MOZ_GL_DEBUG. OK by me.

Comment 6

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/96e052b3e845

Comment 7

[Ed Morley [:emorley]]

I believe one of the changesets in that push (bug 594876, bug 675474 or bug 675532) has caused an OSX 10.6 reftest perma-orange, so needs backing out:
http://tbpl.allizom.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=0a920411e64c

Comment 8

[Justin Wood (:Callek)]

Backed out on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7797172fc164
http://hg.mozilla.org/integration/mozilla-inbound/rev/955f83dc4372

Comment 9

[Dão Gottwald [:dao]]

Note that Talos reported massive perf regressions on Linux for the push this was part.

Comment 10

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0bc16cab7c82

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/0bc16cab7c82