On Tue, Jan 07, 2014 at 12:02:37AM +0800, Daniel Kurtz wrote: > When using the PIGLIT_GL_TEST_CONFIG_* macros to create a test, a > piglit_gl_framework, 'gl_fw' is used to store gl framework state, > including core and extension gl dispatch handlers. > > Just before running the actual test code, piglit_gl_test_run() gets > invoked to setup gl_fw by calling piglit_gl_framework_factory(). > > If using waffle, the get_wfl_*_proc dispatch handlers should be installed > during piglit_dispatch_init(). However, piglit_dispatch_init() is called > *during* piglit_gl_framework_factory(), so, gl_fw is always NULL and the > handlers are not installed. > > The call chain during init looks like this: > > main // from PIGLIT_GL_TEST_CONFIG_[BEGIN,END] > piglit_gl_test_run > piglit_gl_framework_factory > piglit_winsys_framework_factory > piglit_x11_framework_create > piglit_winsys_framework_init > piglit_wfl_framework_init > make_context_current > make_context_current_singlepass > waffle_context_create() // Need to create context first > piglit_dispatch_default_init() <= gl_fw still NULL, gl_fw not set > piglit_dispatch_init > piglit_get_gl_version > glGetString() // piglit-dispatch will use non-WFL glGetString > > Since gl_fw was NULL, piglit_default_init() fails to install the waffle > dispatch handlers, and the wrong glGetString() might be called to get the > GL_VERSION. > > Fix this by installing get_wfl_*_proc() unconditionally. > > Signed-off-by: Daniel Kurtz <[email protected]> > --- > tests/util/piglit-dispatch.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/tests/util/piglit-dispatch.c b/tests/util/piglit-dispatch.c > index 5284b7d..7f9f910 100644 > --- a/tests/util/piglit-dispatch.c > +++ b/tests/util/piglit-dispatch.c > @@ -233,10 +233,8 @@ piglit_dispatch_init(piglit_dispatch_api api, > break; > } > > - if (gl_fw) { > - get_core_proc_address = get_wfl_core_proc; > - get_ext_proc_address = get_wfl_ext_proc; > - } > + get_core_proc_address = get_wfl_core_proc; > + get_ext_proc_address = get_wfl_ext_proc; > #endif > > /* No need to reset the dispatch pointers the first time */ > -- > 1.8.5.1
I suspect that this patch breaks many GLX and EGL tests. It's illegal to call waffle_dl_sym(), and hence also get_wfl_core_proc(), if waffle if not first initialized with waffle_init(). To my knowledge, no GLX or EGL test calls waffle_init() (though there may be an exception). When your patch series is applied, I want to know the behavior of piglit_dispatch_default_init() and glGetIntegerv() in tests/egl/spec/egl_khr_create_context/valid-flag-debug.c. After your series, does piglit_dispatch_default_init() use get_wfl_core/ext_proc? If so, do waffle_dl_sym()/waffle_get_proc_address() actually succeed? I agree that the bug you discovered needs fixing, but I'm doubt that this fix is correct. We need more information to determine if it's safe. By the way, the gl_fw symbol was originally private (static) to the piglit-framework-gl directory, so the piglit-dispatch code would never access it with fragile hacks like this. I searched the history, and 4fa218b70d27c2dd8daedb6c5bc55135c7f32c00 moved gl_fw from private to public, introducing this fragility. The gl_fw genie needs to go back into its bottle, but that task is likely outside the scope of this patch series. _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
