Last Comment Bug 675532 - Add GLX Debug mode
: Add GLX Debug mode
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla9
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-31 16:02 PDT by Matt Woodrow (:mattwoodrow) (PTO until 27 June)
Modified: 2011-08-24 01:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
GLX Debug Mode (29.79 KB, patch)
2011-07-31 16:02 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
jacob.benoit.1: review-
Details | Diff | Review
GLX Debug Mode v2 (30.19 KB, patch)
2011-08-18 18:20 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
jacob.benoit.1: review+
Details | Diff | Review

Description Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-07-31 16:02:04 PDT
Created attachment 549698 [details] [diff] [review]
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) 2011-07-31 22:30:27 PDT
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) (PTO until 27 June) 2011-08-01 00:24:19 PDT
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) 2011-08-17 12:03:21 PDT
(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) (PTO until 27 June) 2011-08-18 18:20:27 PDT
Created attachment 554281 [details] [diff] [review]
GLX Debug Mode v2

Added printing of the XErrorEvent
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-08-19 08:53:25 PDT
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) (PTO until 27 June) 2011-08-19 19:06:17 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/96e052b3e845
Comment 7 Ed Morley [:emorley] 2011-08-20 05:55:02 PDT
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 9 Dão Gottwald [:dao] 2011-08-22 05:42:15 PDT
Note that Talos reported massive perf regressions on Linux for the push this was part.
Comment 10 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-08-23 19:22:20 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/0bc16cab7c82
Comment 11 Marco Bonardo [::mak] 2011-08-24 01:54:21 PDT
http://hg.mozilla.org/mozilla-central/rev/0bc16cab7c82

Note You need to log in before you can comment on or make changes to this bug.