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

[Brian Paul]

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.


>
>> 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.

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.

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.

-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[] = {
>>
>