[Mesa-dev] [PATCH] mesa: do not use _glapi_new_nop_table() for DRI builds

[Emil Velikov]

On 15/05/15 17:58, Brian Paul wrote:
> On 05/15/2015 11:14 AM, Emil Velikov wrote:
>> On 15/05/15 15:13, Brian Paul wrote:
>>> Commit 4bdbb588a9d38 introduced new _glapi_new_nop_table() and
>>> _glapi_set_nop_handler() functions in the glapi dispatcher (which
>>> live in libGL.so).  The calls to those functions from context.c
>>> would be undefined (i.e. an ABI break) if the libGL used at runtime
>>> was older.
>>>
>>> For the time being, use the old single generic_nop() function for
>>> DRI builds to avoid this problem.  At some point in the future it
>>> should be safe to remove this work-around.  See comments for more
>>> details.
>>> ---
>>>   src/mesa/main/context.c | 60
>>> ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 59 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
>>> index 544cc14..0649708 100644
>>> --- a/src/mesa/main/context.c
>>> +++ b/src/mesa/main/context.c
>>> @@ -883,6 +883,19 @@ update_default_objects(struct gl_context *ctx)
>>>   }
>>>
>>>
>>> +/* XXX this is temporary and should be removed at some point in the
>>> + * future when there's a reasonable expectation that the libGL library
>>> + * contains the _glapi_new_nop_table() and _glapi_set_nop_handler()
>>> + * functions which were added in Mesa 10.6.
>>> + */
>>> +#if defined(GLX_DIRECT_RENDERING)
>> I'd say that this should be WIN32, similar to the original code. The
>> code is indirectly used by gbm/egl as well, so introducing
>> GLX_{IN,}DIRECT_RENDERING here might not fair so well.
> 
> Sure, that's fine.
> 
> 
>>> +/* Avoid libGL / driver ABI break */
>>> +#define USE_GLAPI_NOP_FEATURES 0
>>> +#else
>>> +#define USE_GLAPI_NOP_FEATURES 1
>>> +#endif
>>> +
>>> +
>>>   /**
>>>    * This function is called by the glapi no-op functions.  For each
>>> OpenGL
>>>    * function/entrypoint there's a simple no-op function.  These "no-op"
>>> @@ -898,6 +911,7 @@ update_default_objects(struct gl_context *ctx)
>>>    *
>>>    * \param name  the name of the OpenGL function
>>>    */
>>> +#if USE_GLAPI_NOP_FEATURES
>>>   static void
>>>   nop_handler(const char *name)
>>>   {
>>> @@ -914,6 +928,7 @@ nop_handler(const char *name)
>>>      }
>>>   #endif
>>>   }
>>> +#endif
>>>
>>>
>>>   /**
>>> @@ -928,6 +943,44 @@ nop_glFlush(void)
>> Seems like the macro guarding nop_glFlush() should be
>> USE_GLAPI_NOP_FEATURES ? Then we can drop the explicit
>> !USE_GLAPI_NOP_FEATURES below and use #else.
> 
> No, nop_glFlush() is specifically a Windows / WGL fix.  See comments
> elsewhere.
> 
I was definitely to stingy on my explanation here, let me elaborate: The
original code had two "implementations" guarded by the _WIN32 macro. As
we bring back a similar distinction, it will be great to consistently
use the new macro USE_GLAPI_NOP_FEATURES.

That made me think that it'll be cleaner if the whole thing is wrapped
as follows

#if USE_GLAPI_NOP_FEATURES

nop_handler()
nop_glFlush()

#else

generic_nop()
new_nop_table()

#endif


Although as you mentioned below you prefer having separate guards. It
was a slightly bike-shed like suggestion :)


> 
>>
>> The comment within nop_glFlush could use an update as well.
> 
> Will do.
> 
> 
>>
>>
>>>   #endif
>>>
>>>
>>> +#if !USE_GLAPI_NOP_FEATURES
>> Fold this and the follow up function new_nop_table() into #if block ?
> 
> My preference is wrap each individual function for readability.
> 
> 
>>
>>> +static int
>>> +generic_nop(void)
>>> +{
>>> +   GET_CURRENT_CONTEXT(ctx);
>>> +   _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +               "unsupported function called "
>>> +               "(unsupported extension or deprecated function?)");
>>> +   return 0;
>>> +}
>>> +#endif
>>> +
>>> +
>>> +/**
>>> + * Create a new API dispatch table in which all entries point to the
>>> + * generic_nop() function.  This will not work on Windows because of
>>> + * the __stdcall convention which requires the callee to clean up the
>>> + * call stack.  That's impossible with one generic no-op function.
>>> + */
>>> +#if !USE_GLAPI_NOP_FEATURES
>>> +static struct _glapi_table *
>>> +new_nop_table(unsigned numEntries)
>>> +{
>>> +   struct _glapi_table *table;
>>> +
>>> +   table = malloc(numEntries * sizeof(_glapi_proc));
>>> +   if (table) {
>>> +      _glapi_proc *entry = (_glapi_proc *) table;
>>> +      unsigned i;
>>> +      for (i = 0; i < numEntries; i++) {
>>> +         entry[i] = (_glapi_proc) generic_nop;
>>> +      }
>> Bikeshed: One could use memset, analogous to the memcpy() in
>> _glapi_new_nop_table.
> 
> How would memset work?  I'm assigning 4 or 8-byte pointers.
> 
Brain fart. Please ignore.

> 
>>
>>> +   }
>>> +   return table;
>>> +}
>>> +#endif
>>> +
>>> +
>>>   /**
>>>    * Allocate and initialize a new dispatch table.  The table will be
>>>    * populated with pointers to "no-op" functions.  In turn, the no-op
>>> @@ -936,12 +989,16 @@ nop_glFlush(void)
>>>   static struct _glapi_table *
>>>   alloc_dispatch_table(void)
>>>   {
>>> +   int numEntries = MAX2(_glapi_get_dispatch_table_size(),
>>> _gloffset_COUNT);
>>> +
>>> +#if !USE_GLAPI_NOP_FEATURES
>>> +   struct _glapi_table *table = new_nop_table(numEntries);
>>> +#else
>>>      /* Find the larger of Mesa's dispatch table and libGL's dispatch
>>> table.
>>>       * In practice, this'll be the same for stand-alone Mesa.  But
>>> for DRI
>>>       * Mesa we do this to accommodate different versions of libGL
>>> and various
>>>       * DRI drivers.
>>>       */
>> Move the comment as well as numEntries.
> 
> I'll move the comment but I don't understand what you mean by moving
> numEntries.  It's used in both clauses of the #if.
> 
I am missing a comma in there somewhere... so that the sentence reads as
"Move the comment just like you did with numEntries."

> 
>>
>>> -   GLint numEntries = MAX2(_glapi_get_dispatch_table_size(),
>>> _gloffset_COUNT);
>>>      struct _glapi_table *table = _glapi_new_nop_table(numEntries);
>>>
>>>   #if defined(_WIN32)
>>> @@ -967,6 +1024,7 @@ alloc_dispatch_table(void)
>>>   #endif
>>>
>>>      _glapi_set_nop_handler(nop_handler);
>>> +#endif
>>>
>>>      return table;
>>>   }
>>>
>> Should we revert commit 627991dbf74(dri: add _glapi_set_nop_handler(),
>> _glapi_new_nop_table() to dri_test.c) now that the new symbols are no
>> longer around ?
> 
> Per the comments, I'd like to remove all this USE_GLAPI_NOP_FEATURES
> stuff in the future.  We'd need the contents of 627991dbf74 then, right?
> 
I'm not against this, although unless we manage to convince Ian
otherwise the ABI will likely stay stable in the foreseeable future.

> 
>>
>> src/mesa/main/tests/dispatch_sanity.cpp should be updated as well imho.
>> In one hand we can restore the original behaviour (!WIN32 only) on the
>> other we can extend it to cover USE_GLAPI_NOP_FEATURES.
> 
> Not sure I follow.  Can you elaborate?
> 

I was thinking about 1) using new_nop_table() in
DispatchSanity_test::SetUp() or 2) having a symmetrical use of
_glapi_new_nop_table and new_nop_table just like we do in
alloc_dispatch_table().

Here is an example of 2)

--- a/src/mesa/main/tests/dispatch_sanity.cpp
+++ b/src/mesa/main/tests/dispatch_sanity.cpp
@@ -96,7 +96,11 @@ DispatchSanity_test::SetUp()
    _mesa_init_driver_functions(&driver_functions);

    const unsigned size = _glapi_get_dispatch_table_size();
+#if USE_GLAPI_NOP_FEATURES // aka WIN32
    nop_table = (_glapi_proc *) _glapi_new_nop_table(size);
+#else
+   nop_table = (_glapi_proc *) new_nop_table(size);
+#endif
 }


Why bother ?
 - We check the same nop dispatch table in the test as the one we use.
 - Things are less likely to go bad (I hope) next time anyone's around.

I might be over-engineering it though :)

Cheers,
Emil