On 02/22/2016 10:16 PM, Ian Romanick wrote:
There are 17 total occurrences ofgrep -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?
On 02/11/2016 04:04 AM, Tapani Pälli wrote:From: Bernard Kilarski <bernard.r.kilar...@intel.com> Signed-off-by: Bernard Kilarski <bernard.r.kilar...@intel.com> Signed-off-by: Matt Roper <matthew.d.ro...@intel.com> Cc: "11.0 11.1" <mesa-sta...@lists.freedesktop.org --- src/glx/glxcmds.c | 2 +- src/glx/query_renderer.c | 4 ++-- src/glx/tests/query_renderer_unittest.cpp | 4 ++++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c index 93e8db5..4db67ec 100644 --- a/src/glx/glxcmds.c +++ b/src/glx/glxcmds.c @@ -1727,7 +1727,7 @@ __glXSwapIntervalSGI(int interval) CARD32 *interval_ptr; CARD8 opcode; - if (gc == NULL) { + if (gc == NULL || gc == &dummyContext) { return GLX_BAD_CONTEXT; } diff --git a/src/glx/query_renderer.c b/src/glx/query_renderer.c index 9108ec2..d49b8fe 100644 --- a/src/glx/query_renderer.c +++ b/src/glx/query_renderer.c @@ -106,7 +106,7 @@ glXQueryCurrentRendererIntegerMESA(int attribute, unsigned int *value) { struct glx_context *gc = __glXGetCurrentContext(); - if (gc == NULL) + if (gc == NULL || gc == &dummyContext) return False; return __glXQueryRendererInteger(gc->psc, attribute, value); @@ -166,7 +166,7 @@ glXQueryCurrentRendererStringMESA(int attribute) { struct glx_context *gc = __glXGetCurrentContext(); - if (gc == NULL) + if (gc == NULL || gc == &dummyContext) return False; return __glXQueryRendererString(gc->psc, attribute); diff --git a/src/glx/tests/query_renderer_unittest.cpp b/src/glx/tests/query_renderer_unittest.cpp index 2f3c4ef..4c96260 100644 --- a/src/glx/tests/query_renderer_unittest.cpp +++ b/src/glx/tests/query_renderer_unittest.cpp @@ -40,6 +40,10 @@ struct attribute_test_vector { #define E(x) { # x, x } +/* This is necessary so that we don't have to link with glxcurrent.c + * which would require us to link with X libraries and what not. + */ +struct glx_context dummyContext; static bool got_sigsegv; static jmp_buf jmp;
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
