Closed
Bug 675532
Opened 13 years ago
Closed 13 years ago
Add GLX Debug mode
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: mattwoodrow, Unassigned)
Details
(Whiteboard: [inbound])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #549698 -
Flags: review?(bjacob)
Comment 1•13 years ago
|
||
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.
Attachment #549698 -
Flags: review?(bjacob) → review-
Reporter | ||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
(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.
Reporter | ||
Comment 4•13 years ago
|
||
Added printing of the XErrorEvent
Attachment #549698 -
Attachment is obsolete: true
Attachment #554281 -
Flags: review?(bjacob)
Comment 5•13 years ago
|
||
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.
Attachment #554281 -
Flags: review?(bjacob) → review+
Reporter | ||
Comment 6•13 years ago
|
||
Whiteboard: [inbound]
Comment 7•13 years ago
|
||
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•13 years ago
|
||
Backed out on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7797172fc164
http://hg.mozilla.org/integration/mozilla-inbound/rev/955f83dc4372
Whiteboard: [inbound]
Comment 9•13 years ago
|
||
Note that Talos reported massive perf regressions on Linux for the push this was part.
Reporter | ||
Comment 10•13 years ago
|
||
Whiteboard: [inbound]
Comment 11•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in
before you can comment on or make changes to this bug.
Description
•