On Friday, 27 May 2016, Tapani Pälli <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
On 05/26/2016 05:16 PM, Emil Velikov wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi all,<br>
<br>
On 29 February 2016 at 07:14, Tapani Pälli <<a>tapani.palli@intel.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 02/22/2016 10:16 PM, Ian Romanick wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
There are 17 total occurrences of<br>
<br>
     grep -r '[(]!gc[)]' src/glx/<br>
<br>
and<br>
<br>
     grep -r 'gc[[:space:]]*==[[:space:]]*NULL' src/glx/<br>
<br>
None of these check for dummyContext.  This is all very suspicious.<br>
Looking at the implementation(s) of __glXGetCurrentContext, I don't<br>
think it can ever return NULL.  Look in src/glx/glxcurrent.c.  It's<br>
possible that __glXGetCurrentContext used to be able to return NULL, but<br>
I find it unlikely.<br>
<br>
My guess is that all (or nearly all) of the !gc or gc == NULL checks are<br>
wrong.  A bunch of them probably "just work" because they end up sending<br>
protocol requests to the server, and the server sends back an error.<br>
</blockquote>
<br>
<br>
I spent some time with this and it looks like some of these are correct as<br>
create_context (or indirect_create_context) can return NULL and also pointer<br>
given by client may be NULL (and can't be dummyContext). The places with<br>
explicit __glXGetCurrentContext call (9 of these) and a NULL check are<br>
incorrect. I can add these to the patch.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
At the very least, I think these gc == NULL checks should be replaced by<br>
asserts.  If the unit tests call these functions with<br>
__glXGetCurrentContext returning NULL, the unit tests should be fixed to<br>
return &dummyContext instead.<br>
</blockquote>
<br>
<br>
Should it be then 'own dummyContext' implemented by fake_glx_screen.cpp<br>
something along lines in this patch and not trying to link with<br>
glxcurrent.c?<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'd really like to see analysis of the other NULL checks and either have<br>
justifications for no change or have changes.  I'd also really like to<br>
see piglit tests that could hit some of these.<br>
</blockquote>
<br>
<br>
It looks like glx-test is testing return value of __glXGetCurrentContext<br>
currently (which is why it breaks), wouldn't fixing glx-test be sufficient?<br>
<br>
<br>
</blockquote>
Any news on the status of this patch ? The Suse guys did bring some<br>
fixes recently (check __glXGetCurrentContext() vs dummyContext as<br>
opposed to NULL), although I think we still want something like the<br>
proposed here. Correct ?<br>
<br>
</blockquote>
<br>
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.<br>
<br></blockquote><div>Indeed. I believe I mentioned /suggested that ;-)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br></blockquote><div>I think there were some concerns</div><div> - not everything going is updated (Ian), looks fine now but will need to double check</div><div> - piglits (Ian)</div><div> - some mesa glx tests are crashing/failing (yourself mentioned that iirc). I take it that things are fine now ?</div><div><br></div><div>Thanks</div><div>Emil</div>