[Mesa-dev] [PATCH] mesa: Restore functionality to dispatch sanity test

[Ian Romanick]

On 04/29/2015 02:44 PM, Brian Paul wrote:
> On 04/29/2015 02:53 PM, Ian Romanick wrote:
>> On 04/29/2015 12:07 PM, Ian Romanick wrote:
>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>
>>> Along with a couple secondary goals, the dispatch sanity test had two
>>> major, primary goals.
>>>
>>> 1. Ensure that all functions part of an API version are set in the
>>>     dispatch table.
>>>
>>> 2. Ensure that functions that cannot be part of an API version are not
>>>     set in the dispatch table.
>>>
>>> Commit 4bdbb58 removed the tests ability to fulfill either of its
>>
>> This commit also breaks binary compatibility between old libGL and new
>> DRI driver.
>>
>> $ EGL_LOG_LEVEL=debug es2_info
>> libEGL debug: Native platform type: x11 (autodetected)
>> libEGL debug: EGL search path is /opt/xorg-master-x86_64/lib64/egl
>> libEGL debug: added egl_dri2 to module array
>> libEGL debug: failed to open
>> /home/idr/devel/graphics/Mesa/BUILD/10.4-64/lib64/i965_dri.so:
>> /home/idr/devel/graphics/Mesa/BUILD/10.4-64/lib64/i965_dri.so:
>> undefined symbol: _glapi_set_nop_handler
>>
>> That's not ok. :(
>>
>> Brian: Can you propose an alternate solution to your original problem
>> that doesn't break compatibility?  Otherwise, I'm going to have to just
>> revert 4bdbb58.
> 
> Please hold off on that.  I'm going to be off-line until next week and
> won't have time to work on this until then at the earliest.

As it turns out, I spent the rest of the week sick in bed, so I held off
on it. :)

>>> primary goals by removing anything that used _mesa_generic_nop().  It
>>> seems like the problem on Windows could have been resolved by adding the
>>> NULL context pointer check from nop_handler to _mesa_generic_nop().
> 
> Unfortunately, no.  The problem is the the OpenGL API uses __stdcall
> convention on Windows (the callee has to restore the stack).  That means
> plugging a single, generic no-op handler into all dispatch slots will
> not work.  The no-op function will not properly clean up the stack and
> we'll crash.  We found this with a professional CAD app on Windows so
> the fix is critical to those users.

Oh... that's annoying, but makes sense.

> A temporary work-around may be to only call _glapi_set_nop_handler() for
> Windows.  Then, maybe remove the #ifdef _WIN32 at some point down the road.

Either that or condition it on DRI builds.  Other Mesa builds won't have
this particular issue.  Other loader / driver interfaces are dynamic.
If we really want to enable this feature in DRI builds, we can do it.
It will just require someone to do a bunch of typing.

> How often do you test mixing old libGL with newer drivers?  I've always
> suspected this is a bit fragile.  Nobody else seems to have noticed.

Periodically.  I generally have a bunch of Mesa branches built at once
that I test for various things.  I mostly end up using mismatched
libraries when I have to use EGL because, for some reason, using
non-installed libEGL is really painful.

> -Brian
> 
> PS: the patch looks OK to me.
> 
> Reviewed-by: Brian Paul <brianp at vmware.com>
> 
> 
>>> There is, however, some debugging benefit to actually getting the
>>> (supposed) function name logged in the "unsupported function called"
>>> message.
>>>
>>> The preceding commit added a function, _glapi_new_nop_table, that
>>> allocates a table of per-entry point no-op functions.  Restore the
>>> ability to actually validate the sanity of the dispatch table by using
>>> _glapi_new_nop_table.
>>>
>>> Previous to this commit removing a function from one of the
>>> *_functions_possible lists would not cause the test to fail.  With this
>>> commit removing such a function will result in failure, as is expected.
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>> Cc: Brian Paul <brianp at vmware.com>
>>> ---
>>>   src/mesa/main/tests/dispatch_sanity.cpp | 49
>>> +++++++++++++++++++++++++++++----
>>>   1 file changed, 43 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/mesa/main/tests/dispatch_sanity.cpp
>>> b/src/mesa/main/tests/dispatch_sanity.cpp
>>> index 946eabb..2a5afcd 100644
>>> --- a/src/mesa/main/tests/dispatch_sanity.cpp
>>> +++ b/src/mesa/main/tests/dispatch_sanity.cpp
>>> @@ -82,6 +82,7 @@ public:
>>>      struct dd_function_table driver_functions;
>>>      struct gl_context share_list;
>>>      struct gl_context ctx;
>>> +   _glapi_proc *nop_table;
>>>   };
>>>
>>>   void
>>> @@ -93,6 +94,9 @@ DispatchSanity_test::SetUp()
>>>      memset(&ctx, 0, sizeof(ctx));
>>>
>>>      _mesa_init_driver_functions(&driver_functions);
>>> +
>>> +   const unsigned size = _glapi_get_dispatch_table_size();
>>> +   nop_table = (_glapi_proc *) _glapi_new_nop_table(size);
>>>   }
>>>
>>>   void
>>> @@ -122,11 +126,18 @@ offset_to_proc_name_safe(unsigned offset)
>>>    * _glapi_proc *table exist.
>>>    */
>>>   static void
>>> -validate_functions(struct gl_context *ctx, const struct function
>>> *function_table)
>>> +validate_functions(struct gl_context *ctx, const struct function
>>> *function_table,
>>> +                   const _glapi_proc *nop_table)
>>>   {
>>>      _glapi_proc *table = (_glapi_proc *) ctx->Exec;
>>>
>>>      for (unsigned i = 0; function_table[i].name != NULL; i++) {
>>> +      /* The context version is >= the GL version where the function
>>> was
>>> +       * introduced. Therefore, the function cannot be set to the nop
>>> +       * function.
>>> +       */
>>> +      const bool cant_be_nop = ctx->Version >=
>>> function_table[i].Version;
>>> +
>>>         const int offset = (function_table[i].offset != -1)
>>>            ? function_table[i].offset
>>>            : _glapi_get_proc_offset(function_table[i].name);
>>> @@ -136,32 +147,58 @@ validate_functions(struct gl_context *ctx,
>>> const struct function *function_table
>>>         ASSERT_EQ(offset,
>>>                   _glapi_get_proc_offset(function_table[i].name))
>>>            << "Function: " << function_table[i].name;
>>> +      if (cant_be_nop) {
>>> +         EXPECT_NE(nop_table[offset], table[offset])
>>> +            << "Function: " << function_table[i].name
>>> +            << " at offset " << offset;
>>> +      }
>>> +
>>> +      table[offset] = nop_table[offset];
>>> +   }
>>> +}
>>> +
>>> +/* Scan through the table and ensure that there is nothing except
>>> + * nop functions (as set by validate_functions().
>>> + */
>>> +static void
>>> +validate_nops(struct gl_context *ctx, const _glapi_proc *nop_table)
>>> +{
>>> +   _glapi_proc *table = (_glapi_proc *) ctx->Exec;
>>> +
>>> +   const unsigned size = _glapi_get_dispatch_table_size();
>>> +   for (unsigned i = 0; i < size; i++) {
>>> +      EXPECT_EQ(nop_table[i], table[i])
>>> +         << "i = " << i << " (" << offset_to_proc_name_safe(i) << ")";
>>>      }
>>>   }
>>>
>>>   TEST_F(DispatchSanity_test, GL31_CORE)
>>>   {
>>>      SetUpCtx(API_OPENGL_CORE, 31);
>>> -   validate_functions(&ctx, gl_core_functions_possible);
>>> +   validate_functions(&ctx, gl_core_functions_possible, nop_table);
>>> +   validate_nops(&ctx, nop_table);
>>>   }
>>>
>>>   TEST_F(DispatchSanity_test, GLES11)
>>>   {
>>>      SetUpCtx(API_OPENGLES, 11);
>>> -   validate_functions(&ctx, gles11_functions_possible);
>>> +   validate_functions(&ctx, gles11_functions_possible, nop_table);
>>> +   validate_nops(&ctx, nop_table);
>>>   }
>>>
>>>   TEST_F(DispatchSanity_test, GLES2)
>>>   {
>>>      SetUpCtx(API_OPENGLES2, 20);
>>> -   validate_functions(&ctx, gles2_functions_possible);
>>> +   validate_functions(&ctx, gles2_functions_possible, nop_table);
>>> +   validate_nops(&ctx, nop_table);
>>>   }
>>>
>>>   TEST_F(DispatchSanity_test, GLES3)
>>>   {
>>>      SetUpCtx(API_OPENGLES2, 30);
>>> -   validate_functions(&ctx, gles2_functions_possible);
>>> -   validate_functions(&ctx, gles3_functions_possible);
>>> +   validate_functions(&ctx, gles2_functions_possible, nop_table);
>>> +   validate_functions(&ctx, gles3_functions_possible, nop_table);
>>> +   validate_nops(&ctx, nop_table);
>>>   }
>>>
>>>   const struct function gl_core_functions_possible[] = {
>>>