Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mattwoodrow, Unassigned)

Tracking

unspecified
mozilla9
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
Attachment #549698 - Flags: review?(bjacob)
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

6 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 :)
(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

6 years ago
Created attachment 554281 [details] [diff] [review]
GLX Debug Mode v2

Added printing of the XErrorEvent
Attachment #549698 - Attachment is obsolete: true
Attachment #554281 - Flags: review?(bjacob)
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

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/96e052b3e845
Whiteboard: [inbound]

Comment 7

6 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
Backed out on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7797172fc164
http://hg.mozilla.org/integration/mozilla-inbound/rev/955f83dc4372
Whiteboard: [inbound]
Note that Talos reported massive perf regressions on Linux for the push this was part.
(Reporter)

Comment 10

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/0bc16cab7c82
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/0bc16cab7c82
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.