On 05/27/2016 08:16 PM, Emil Velikov wrote:
On Friday, 27 May 2016, Tapani Pälli <tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>> wrote:
On 05/26/2016 05:16 PM, Emil Velikov wrote:
Hi all,
On 29 February 2016 at 07:14, Tapani Pälli
<tapani.pa...@intel.com> wrote:
On 02/22/2016 10:16 PM, Ian Romanick wrote:
There are 17 total occurrences of
grep -r '[(]!gc[)]' src/glx/
and
grep -r 'gc[[:space:]]*==[[:space:]]*NULL' src/glx/
None of these check for dummyContext. This is all
very suspicious.
Looking at the implementation(s) of
__glXGetCurrentContext, I don't
think it can ever return NULL. Look in
src/glx/glxcurrent.c. It's
possible that __glXGetCurrentContext used to be able
to return NULL, but
I find it unlikely.
My guess is that all (or nearly all) of the !gc or gc
== NULL checks are
wrong. A bunch of them probably "just work" because
they end up sending
protocol requests to the server, and the server sends
back an error.
I spent some time with this and it looks like some of
these are correct as
create_context (or indirect_create_context) can return
NULL and also pointer
given by client may be NULL (and can't be dummyContext).
The places with
explicit __glXGetCurrentContext call (9 of these) and a
NULL check are
incorrect. I can add these to the patch.
At the very least, I think these gc == NULL checks
should be replaced by
asserts. If the unit tests call these functions with
__glXGetCurrentContext returning NULL, the unit tests
should be fixed to
return &dummyContext instead.
Should it be then 'own dummyContext' implemented by
fake_glx_screen.cpp
something along lines in this patch and not trying to link
with
glxcurrent.c?
I'd really like to see analysis of the other NULL
checks and either have
justifications for no change or have changes. I'd
also really like to
see piglit tests that could hit some of these.
It looks like glx-test is testing return value of
__glXGetCurrentContext
currently (which is why it breaks), wouldn't fixing
glx-test be sufficient?
Any news on the status of this patch ? The Suse guys did bring
some
fixes recently (check __glXGetCurrentContext() vs dummyContext as
opposed to NULL), although I think we still want something
like the
proposed here. Correct ?
No progress, this patch has been living as is in internal project.
The fix itself is quite simple, all places with
__glXGetCurrentContext should check against dummyContext.
Indeed. I believe I mentioned /suggested that ;-)
This patch introduced its own 'dummyContext' in the unit test
since it seemed very challenging to compile the test together with
files in glx folder (results in linking with a *lot* of stuff). I
can take a peek again what was the issue it replacing all of the
checks and reply back to this.
I think there were some concerns
- not everything going is updated (Ian), looks fine now but will need
to double check
This patch should cover all instances where __glXGetCurrentContext was
called and had a check.
- piglits (Ian)
- some mesa glx tests are crashing/failing (yourself mentioned that
iirc). I take it that things are fine now ?
Yes things are fine now as I moved the dummyContext to fake_glx_screen
where it should have been, 'glx-test' unit tests get their context from
there.
Thanks
Emil
// Tapani
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
