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 ?
Thanks Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
