[Mesa-dev] [PATCH 1/5] darwin: Suppress type conversion warnings for GLhandleARB

[Jose Fonseca]

On 25/06/15 23:18, Julien Isorce wrote:
>
>
> On 19 June 2015 at 10:24, Jose Fonseca <jfonseca at vmware.com
> <mailto:jfonseca at vmware.com>> wrote:
>
>     On 19/06/15 04:46, Ian Romanick wrote:
>
>         On 06/17/2015 10:53 PM, Julien Isorce wrote:
>
>             From: Jon TURNEY <jon.turney at dronecode.org.uk
>             <mailto:jon.turney at dronecode.org.uk>>
>
>             On darwin, GLhandleARB is defined as a void *, not the
>             unsigned int it is on
>             linux.
>
>             For the moment, apply a cast to supress the warning
>
>             Possibly this is safe, as for the mesa software renderer the
>             shader program
>             handle is not a real pointer, but a integer handle
>
>             Probably this is not the right thing to do, and we should
>             pay closer attention
>             to how the GLhandlerARB type is used.
>
>
>         In Mesa, glBindAttribLocation (which takes GLuint) and
>         glBindAttribLocationARB (which takes GLhandleARB) are *the same
>         function*.  The same applies to pretty much all the other
>         GLhandleARB
>         functions.
>
>
>
>     Properly fixing this is a nightmare, but I think that short term
>     workaround is feasible.
>
>     This is the generated glapitemp.h:
>
>        KEYWORD1 void KEYWORD2 NAME(BindAttribLocationARB)(GLhandleARB
>     program, GLuint index, const GLcharARB * name)
>        {
>            (void) program; (void) index; (void) name;
>           DISPATCH(BindAttribLocation, (program, index, name), (F,
>     "glBindAttribLocationARB(%d, %d, %p);\n", program, index, (const
>     void *) name));
>        }
>
>     Provided that GLhandlerARB is defined as `unsigned long` during Mesa
>     build on MacOSX
>
>
> Hi, where exactly ? or do you mean we just need to apply the patch [1]
> you pointed ?
>
>     (to avoid these int<->void *) conversions [1], the compiler should
>     implicitly cast the 64bits GLhandlerARB program to an 32-bits GLuint.
>
>     So, when an app calls glBindAttribLocationARB it will be dispatched
>     to _mesa_BindAttribLocation, and the program truncated. So it should
>     all just work.
>
>     Ditto for when GLhandleARB appears as return value.
>
>
>     The only problem is when GLhandleARB appears as a pointer, as there
>     is only one such instance:
>
>        GLAPI void APIENTRY glGetAttachedObjectsARB (GLhandleARB
>     containerObj, GLsizei maxCount, GLsizei *count, GLhandleARB *obj);
>
>     But we do have a separate entry-point for this
>     (_mesa_GetAttachedObjectsARB) so again, we're all good.
>
>
>     So, Jon/Julien's patch seems perfectly workable -- all really left
>     to do is to silence  GLhandleARB <-> GLuint conversions.
>
>
> That's a good news.
> So Jose concretely what needs to be done ? Just apply the patch [1] you
> pointed or apply cast everywhere ?
>
> All conversions are in the 3 files, src/mesa/main/dlist.c,
> src/mesa/main/shaderapi.c and src/mesa/main/shader_query.cpp, am I right ?
>
> I did not notice before, but without any change on upstream code, it
> gives an error only when compiling c++ files. For c files it is a warning:
>
> main/shaderapi.*c*:1148:23: warning: incompatible pointer to integer
> conversion passing 'GLhandleARB' (aka 'void *') to parameter of type
> 'GLuint' (aka 'unsigned int') [-Wint-conversion] attach_shader(ctx,
> program, shader);
>
> main/shader_query.*cpp*:72:7: error: no matching function for call to
> '_mesa_lookup_shader_program_err'**"glBindAttribLocation");../../src/mesa/main/shaderobj.h:89:1:
> note: candidate function not viable: cannot convert argument of
> incomplete type 'GLhandleARB' (aka 'void *') to 'GLuint' (aka 'unsigned
> int')_mesa_lookup_shader_program_err(struct gl_context *ctx, GLuint name,
>
> Thx
>
>
>
>     Jose
>
>
>     [1] Apitrace also defines GLhandleARB as unsigned long internally to
>     avoid this
>     https://github.com/apitrace/apitrace/blob/master/thirdparty/khronos/GL/glext.patch
>
>

I mean something like this:

diff --git a/configure.ac b/configure.ac
index af61aa2..afcfbf6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1353,7 +1353,7 @@ if test "x$enable_dri" = xyes; then
          fi
          ;;
      darwin*)
-        DEFINES="$DEFINES -DGLX_ALIAS_UNSUPPORTED"
+        DEFINES="$DEFINES -DGLX_ALIAS_UNSUPPORTED -DBUILDING_MESA"
          if test "x$with_dri_drivers" = "xyes"; then
              with_dri_drivers="swrast"
          fi
diff --git a/include/GL/glext.h b/include/GL/glext.h
index a3873a6..1f52871 100644
--- a/include/GL/glext.h
+++ b/include/GL/glext.h
@@ -3879,7 +3879,12 @@ GLAPI void APIENTRY glMinSampleShadingARB 
(GLfloat value);
  #ifndef GL_ARB_shader_objects
  #define GL_ARB_shader_objects 1
  #ifdef __APPLE__
+#ifdef BUILDING_MESA
+// Avoid uint <-> void *  warnings
+typedef unsigned long GLhandleARB;
+#else
  typedef void *GLhandleARB;
+#endif
  #else
  typedef unsigned int GLhandleARB;
  #endif


Jose