[Piglit] [PATCH 2/8] piglit-dispatch: Install waffle procs during dispatch init

[Chad Versace]

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 <djkurtz at chromium.org>
> ---
>  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.