[Mesa-dev] [PATCH] egl: Create queryable strings in eglInitialize().

[Chad Versace]

On 03/10/2015 09:15 PM, Matt Turner wrote:
> On Tue, Mar 10, 2015 at 8:19 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 03/10/2015 05:20 PM, Matt Turner wrote:
>>> Creating/recreating the strings in eglQueryString() is extra work and
>>> isn't thread-safe, as exhibited by shader-db's run.c using libepoxy.
>>>
>>> Multiple threads in run.c call eglReleaseThread() around the same time.
>>> libepoxy calls eglQueryString() to determine whether eglReleaseThread()
>>> exists, and our EGL implementation passes a pointer to the version
>>> string to libepoxy while simultaneously overwriting the string, leading
>>> to a failure in libepoxy.
>>>
>>> Moreover, the EGL spec says (emphasis mine):
>>>
>>> "eglQueryString returns a pointer to a *static*, zero-terminated string"
>>>
>>> This patch moves some auxiliary functions from eglmisc.c to eglapi.c so
>>> that they may be used to create the extension, API, and version strings
>>> once during eglInitialize(). The auxiliary functions are renamed from
>>> _eglUpdate* to _eglCreate*, and some checks made unnecessary by calling
>>> the functions from eglInitialize() are removed.
>>>
>>> It was suggested to me to simply make the generation of the version
>>> string behave like that of the extension and API strings -- to do a
>>> check like
>>>
>>>    if (exts[0])
>>>       return;
>>>
>>> before creating the string, but that is not thread-safe either.
>>
>> It is safe because eglQueryString calls _eglQueryString (via
>> drv->API.QueryString) inside a _eglLockDisplay region.
> 
> Oh, you're right. I'll drop that bit from the commit message, pending
> removal of API.QueryString.

I like Matt's approach better (creating the strings during eglInitialize)
because its correctness does not rely on the display lock. The big display
lock is embarrassing, and we should avoid writing code that depends on it
whenever possible.

(Aside: The big display lock will probably cause trouble when implementing
EGL_KHR_fence_sync, because that extension expects the display
to be re-entrant with regard to syncs).

>> The method you use here presumes that every implementation will use the
>> _eglQueryString fallback for drv->API.QueryString.  Right now it does,
>> and I suspect that they always will.  A little git archaeology leads me
>> to believe that this has always been the case.
>>
>> Perhaps we should just remove API.QueryString altogether?  Perhaps Brian
>> knows of a reason not to...
> 
> Getting rid of that could be nice. We could inline _eglQueryString,
> avoid locking the display, and throw out eglmisc.c/eglmisc.h.

Yes, let's get rid of API.QueryString. Mesa's egl/main directory contains
a superfluous amount of virtual dispatch, and API.QueryString is one
of the guilty parties.

I like this patch is. It's
Reviewed-by: Chad Versace <chad.versace at intel.com>