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

[Daniel Kurtz]

Hi Chad,

With this patch applied, we don't even get as far as glGetIntegerv(),
since we can't even resolve "glGetString" to deduce the GL_VERSION in
piglit_dispatch_default_init()->piglit_dispatch_init()->piglit_get_gl_version().

# bin/egl-create-context-valid-flag-debug gles3 -auto
piglit_dispatch_default_init(api:2)
piglit_dispatch_init(api:2)
piglit_get_gl_version: calling glGetString(GL_VERSION)
piglit: error: get_wfl_core_proc failed due to WAFFLE_ERROR_NOT_INITIALIZED
GetProcAddress failed for "glGetString"
PIGLIT: {'result': 'fail' }

Without my patch, however, we were calling the default dispatch resolution path:
piglit_get_gl_version
  piglit_dispatch_glGetString
    stub_glGetString
      resolve_glGetString
         get_core_proc
            get_core_proc_address
              get_ext_proc_address
                glXGetProcAddressARB()

This kind of works, and glXGetProcAddressARB returns the address of
glGetString() in libGLso...  However, this is the wrong glGetString()
function, since we should be finding glGetString() of libGLESv2.so. On
my system, this glGetString() returns NULL, and since
piglit_get_gl_version isn't able to handle a NULL version string, so
the test crashes with a seg fault.

So, on my system at least, my patch changed these crashing EGL tests
into failures.  I considered that progress :-).  Of course, if it
breaks other

My patch does fixes some of the OpenGL ES 2.0 and 3.0 tests, though,
since they use the PIGLIT_GL_TEST_CONFIG_{BEGIN, END} macros, which do
call waffle_init(). So...

Why aren't the EGL/GLX tests initializing waffle?
Is it supposed to work already, or is adding waffle support to EGL
tests still a work in progress?
What do you recommend to get them working?

On Sat, Jan 11, 2014 at 2:57 AM, Chad Versace
<chad.versace at linux.intel.com> wrote:
> 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.