<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 05/27/2016 08:16 PM, Emil Velikov
wrote:<br>
</div>
<blockquote
cite="mid:CACvgo52o0cf7GLsTtfdQ227GnHE_j_DfBi_SdZwB1AmSo74=Fg@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
On Friday, 27 May 2016, Tapani Pälli <<a moz-do-not-send="true"
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
moz-do-not-send="true"><a class="moz-txt-link-abbreviated" href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a></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>
</blockquote>
<br>
This patch should cover all instances where __glXGetCurrentContext
was called and had a check.<br>
<br>
<blockquote
cite="mid:CACvgo52o0cf7GLsTtfdQ227GnHE_j_DfBi_SdZwB1AmSo74=Fg@mail.gmail.com"
type="cite">
<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>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:CACvgo52o0cf7GLsTtfdQ227GnHE_j_DfBi_SdZwB1AmSo74=Fg@mail.gmail.com"
type="cite">
<div>Thanks</div>
<div>Emil</div>
</blockquote>
<p><br>
</p>
<p>// Tapani</p>
<p><br>
</p>
</body>
</html>