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

[Brian Paul]

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.


>
> 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.


>
>> +   }
>> +   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.


>
>> -   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?


>
> 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?

-Brian