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 <[email protected]> >>> >>> 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 <[email protected]> > > >>> 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 <[email protected]> >>> Cc: Brian Paul <[email protected]> >>> --- >>> 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[] = { >>> _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
