Re: Made ENTER_GL() - LEAVE_GL() free X11 lock on exception inside opengl, code

[Chris Robinson]

On Saturday 28 June 2008 07:51:53 am Massimo Del Fedele wrote:
> Roderick Colenbrander ha scritto:
> > Hi,
> >
> > I don't think we want to go this way. First of all we want to emulate
> > win32 opengl. If windows does this we MIGHT have to do something like
> > this.
>
> Well, I've never seen windows hang for an opengl call exception... But I
>    can be wrong. IMHO the exception should be catched and/or propagated,
> not allowed to block the whole app.....
> Here the problem is that an exception thrown by gl code skips the
> wine_tsx11_unlock() call, blocking all following X11 code.

I think the issue is that it shouldn't be crashing. Especially if we don't 
know why it's crashing, this patch is just hiding the error more than 
anything. For all we know it can be a buffer overrun somewhere, and you don't 
want to just work around things like that.
> > Second all opengl calls hit ENTER_GL / LEAVE_GL and these few extra calls
> > affect performance.
>
> which extra calls ?

The __TRY stuff. It's rather expensive to set up an exception block, even in 
C++ where it's part of the language (let alone how Wine does it in C). But we 
do know D3D doesn't catch exceptions in most cases (it can crash in plenty of 
ways), so I don't think this is correct anyway.

Made ENTER_GL() - LEAVE_GL() free X11 lock on exception inside opengl, code

[Massimo Del Fedele]

Chris Robinson ha scritto:
> On Saturday 28 June 2008 07:51:53 am Massimo Del Fedele wrote:
> > Roderick Colenbrander ha scritto:
> > > Hi,
> > >
> > > I don't think we want to go this way. First of all we want to emulate
> > > win32 opengl. If windows does this we MIGHT have to do something like
> > > this.
> >
> > Well, I've never seen windows hang for an opengl call exception... But I
> >    can be wrong. IMHO the exception should be catched and/or propagated,
> > not allowed to block the whole app.....
> > Here the problem is that an exception thrown by gl code skips the
> > wine_tsx11_unlock() call, blocking all following X11 code.
>
> I think the issue is that it shouldn't be crashing. Especially if we don't 
> know why it's crashing,...

well, there are not so many places... it crashes on a glGetString(), 
which accepts a scalar value and returns a static string pointer.
No dynamic data, no bad pointers outside it.
So it must be a crash INSIDE opengl code.
It's called (wrongly) without an opengl context, but that's how autocad 
installer does, so it should NOT crash because of that.
In windows it just returns NULL pointer, in wine, with a simple test app 
it also returns NULL, so we can't reproduce with a simple test case.
Maybe some previous GL call damages some internal opengl data... but I'm 
still convinced that as it doesn't crash in windows it should not do it 
in wine. Or, at least it should free the lock when crashing.....
> this patch is just hiding the error more than 
> anything.

Not exactly. Latest patch (the try...finally) doesn't hide anything, it 
just frees the lock upon crash in gl code, which imho is correct 
behaviour. Autocad installer ignores the crash and follows, with the 
patch. Without it, it hangs on next x11 call.

For all we know it can be a buffer overrun somewhere, and you don't
> want to just work around things like that.
> > > Second all opengl calls hit ENTER_GL / LEAVE_GL and these few extra calls
> > > affect performance.
> >
> > which extra calls ?
>
> The __TRY stuff. It's rather expensive to set up an exception block, even in 
> C++ where it's part of the language (let alone how Wine does it in C). But we 
> do know D3D doesn't catch exceptions in most cases (it can crash in plenty of 
> ways), so I don't think this is correct anyway.

The first patch proposed (and another, more complete), IS a workaround.
We checks if a context exists and, if none, we just return NULL instead 
locking-calling-unlocking. Windows behaviour is that one, so it should 
do no harm.

The problem here is not catching/not catching the exception; I agree 
that it should be uncaught and let to user code. The problem is the 
stale lock with has nothing to do with user code.
If some app has a code like that :

try {
   do_some_gl_call();
} catch (...)
   error_handling()
}
some_x11_call();  <== it will hang here

On ANY opengl exception it'll not have any chance to correct the error 
and continue.

Max

Made ENTER_GL() - LEAVE_GL() free X11 lock on exception inside opengl, code

[Chris Robinson]

On Saturday 28 June 2008 01:50:09 pm Massimo Del Fedele wrote:
> Chris Robinson ha scritto:
> > I think the issue is that it shouldn't be crashing. Especially if we
> > don't know why it's crashing,...
>
> well, there are not so many places... it crashes on a glGetString(),
> which accepts a scalar value and returns a static string pointer.
> No dynamic data, no bad pointers outside it.
> So it must be a crash INSIDE opengl code.
> It's called (wrongly) without an opengl context, but that's how autocad
> installer does, so it should NOT crash because of that.

And as was shown, Wine correctly returns NULL when its called without a 
context, so there is something else making it crash. It may be a buggy 
driver. It may be memory corruption Wine is causing.. who knows.
> but I'm
> still convinced that as it doesn't crash in windows it should not do it
> in wine.

Of course, which is why we should try to find the reason it crashes and fix 
it, not hide the crash behind a costly try block.