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

[Emil Velikov]

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.


> +/* 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.

The comment within nop_glFlush could use an update as well.


>  #endif
>  
>  
> +#if !USE_GLAPI_NOP_FEATURES
Fold this and the follow up function new_nop_table() into #if block ?

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

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

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

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.

Cheers,
Emil