[Piglit] [PATCH 13/14] dispatch: Use dlsym to lookup core symbols for EGL

[Marek Olšák]

Correction: Only 2 pipeline statistics tests were broken, so all seems
to be good (except those 2 tests).

Marek

On Tue, Apr 28, 2015 at 3:18 PM, Marek Olšák <maraeo at gmail.com> wrote:
> Hi Chad,
>
> The patch breaks GetProcAddress for libGL, because libGL doesn't
> export a lot of GL functions and glXGetProcAddress is the only way to
> get them. Any idea for a fix?
>
> Marek
>
> On Fri, Apr 24, 2015 at 9:36 PM, Chad Versace <chad.versace at intel.com> wrote:
>> On Mon 13 Apr 2015, Marek Olšák wrote:
>>> From: Daniel Kurtz <djkurtz at chromium.org>
>>>
>>> eglGetProcAddress() only supports extension functions.
>>> Therefore, we must use dlsym() directly on the GL client library to look
>>> up core functions.
>>>
>>> The implementation here is a much simplified version of the one in
>>> libepoxy.  Most of the simplification is because piglit dispatch already
>>> knows exactly for which GL API it is looking up a symbol.
>>>
>>> Signed-off-by: Daniel Kurtz <djkurtz at chromium.org>
>>> ---
>>>  tests/util/piglit-dispatch-init.c | 57 +++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 55 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/util/piglit-dispatch-init.c b/tests/util/piglit-dispatch-init.c
>>> index cc8b684..b0e1649 100644
>>> --- a/tests/util/piglit-dispatch-init.c
>>> +++ b/tests/util/piglit-dispatch-init.c
>>> @@ -33,6 +33,8 @@
>>>
>>>  #else /* Linux */
>>>
>>> +#include <dlfcn.h>
>>> +
>>>  #if defined(PIGLIT_HAS_GLX)
>>>  #    include "glxew.h"
>>>  #elif defined(PIGLIT_HAS_EGL)
>>> @@ -154,6 +156,42 @@ get_core_proc_address(const char *function_name, int gl_10x_version)
>>>
>>>  #else /* Linux */
>>>
>>> +#if defined(PIGLIT_HAS_EGL)
>>> +#define GLX_LIB "libGL.so.1"
>>> +#define GLES1_LIB "libGLESv1_CM.so.1"
>>> +#define GLES2_LIB "libGLESv2.so.2"
>>> +
>>> +/** dlopen() return value for libGL.so.1 */
>>> +static void *glx_handle;
>>> +
>>> +/** dlopen() return value for libGLESv1_CM.so.1 */
>>> +static void *gles1_handle;
>>> +
>>> +/** dlopen() return value for libGLESv2.so.2 */
>>> +static void *gles2_handle;
>>> +
>>> +static void *
>>> +do_dlsym(void **handle, const char *lib_name, const char *function_name)
>>> +{
>>> +    void *result;
>>> +
>>> +    if (!*handle)
>>> +        *handle = dlopen(lib_name, RTLD_LAZY | RTLD_LOCAL);
>>> +
>>> +    if (!*handle) {
>>> +        fprintf(stderr, "Could not open %s: %s\n", lib_name, dlerror());
>>> +        return NULL;
>>> +    }
>>> +
>>> +    result = dlsym(*handle, function_name);
>>> +    if (!result)
>>> +        fprintf(stderr, "%s() not found in %s: %s\n", function_name, lib_name,
>>> +                dlerror());
>>> +
>>> +    return result;
>>> +}
>>> +#endif
>>> +
>>>  /**
>>>   * This function is used to retrieve the address of all GL functions
>>>   * on Linux.
>>
>> Everyting up to here looks good.
>>
>>> @@ -174,16 +212,31 @@ get_ext_proc_address(const char *function_name)
>>>  /**
>>>   * This function is used to retrieve the address of core GL functions
>>>   * on Linux.
>>> + *
>>> + * Since eglGetProcAddress() only supports extension functions, we must use
>>> + * dlsym() directly on the GL client library to lookup core functions.
>>
>> This comment isn't true. Sometimes eglGetProcAddress does supports core
>> functions. It should instead say:
>>
>>   """
>>   eglGetProcAddress supports querying core functions only if EGL >= 1.5
>>   or if EGL_KHR_get_all_proc_addresses or
>>   EGL_KHR_client_get_all_proc_addresses is supported. Rather than worry
>>   about such details, when using EGL we consistently use dlsym() on the
>>   client library to lookup core functions.
>>   """
>>
>> This works for libGLESv1_CM.so and libGLESv2.so. But libGL.so wasn't designed
>> with EGL in mind.  The antiquated OpenGL ABI for Linux [1] states:
>>
>>   3.4. [The OpenGL library] must export all OpenGL 1.2, [...] GLX 1.3, and
>>        ARB_multitexture entry points statically.  [...] Applications should not
>>        expect to link statically against any entry points not specified here.
>>
>> The specification of eglGetProcAddress and the official ABI specification for
>> libGL are mutually incompatible. I think it's best to comply with
>> eglGetProcAddress's restrictions rather than complying with a 15 year old
>> standard that no one follows anymore. In other words, I think this patch takes
>> the right approach.
>>
>>>   */
>>>  static piglit_dispatch_function_ptr
>>>  get_core_proc_address(const char *function_name, int gl_10x_version)
>>>  {
>>> -     /* We don't need to worry about the GL version, since on Apple
>>> +#if defined(PIGLIT_HAS_EGL)
>>> +     switch (gl_10x_version) {
>>> +     case 11:
>>> +             return do_dlsym(&gles1_handle, GLES1_LIB, function_name);
>>> +     case 20:
>>> +             return do_dlsym(&gles2_handle, GLES2_LIB, function_name);
>>> +     case 10:
>>> +     default:
>>> +             /* GL does not have its own library, so use GLX */
>>> +             return do_dlsym(&glx_handle, GLX_LIB, function_name);
>>> +     }
>>> +#else
>>> +     /* We don't need to worry about the GL version, since when using GLX
>>>        * we retrieve all proc addresses in the same way.
>>>        */
>>>       (void) gl_10x_version;
>>> -
>>>       return get_ext_proc_address(function_name);
>>> +#endif
>>>  }
>>
>> I was initially certain this patch would break piglit-dispatch for certain core
>> GL functions. But I was wrong. This patch's approach looks good to me. (Its
>> correctness relies on piglit-dispatch's subtle interpretation of the
>> gl_10x_version parameter, a subtlety that's easy to forget).
>>
>> The default case may cause problems on some GL implementations, because of the
>> specification conflicts I explained above. According to Linux's official OpenGL
>> ABI, one cannot dlsym(libGL.so.1) for core functions that appeared in GL
>> versions after 1.2. But, I think the risk is minor. If we later discover that
>> the default case does break piglit-dispatch on some GL implementation, then we
>> can easily fix it by changing it to use get_ext_proc_address() instead of dlsym().
>>
>> Reviewed-by: Chad Versace <chad.versace at intel.com>
>>
>> Thanks for upstreaming all of Daniel's fixes.