Bug 66346

Created attachment 81814 [details] [review]
use a function to convert from GLhandleARB to GLuint

Vinson, can you try this patch?  There may be other files to fix similarly...

(In reply to comment #1)
> Created attachment 81814 [details] [review] [review]
> use a function to convert from GLhandleARB to GLuint
> 
> Vinson, can you try this patch?  There may be other files to fix similarly...

  Compiling src/mesa/main/uniform_query.cpp ...
src/mesa/main/uniform_query.cpp: In function ‘void _mesa_GetActiveUniform(void*, GLuint, GLsizei, GLsizei*, GLint*, GLenum*, GLcharARB*)’:
src/mesa/main/uniform_query.cpp:49: error: invalid conversion from ‘void*’ to ‘GLuint’
src/mesa/main/uniform_query.cpp:49: error:   initializing argument 2 of ‘gl_shader_program* _mesa_lookup_shader_program_err(gl_context*, GLuint, const char*)’

mesa: 378c6f2246d66254ce0f88cac6daf25b1c012a41 (master 10.2.0-devel)

Mac OS X build is still broken.

This is in glext.h:

#ifdef __APPLE__
typedef void *GLhandleARB;
#else
typedef unsigned int GLhandleARB;
#endif

Thanks, Apple!

I think Mesa is just broken on Apple, probably forever, because of this.  We assume *everywhere* that GLhandleARB == GLuint... to the point that glGetActiveAttrib and glGetActiveAttribARB (and many, many others) are the same function.

This occurs in shader_query.cpp, uniforms.c, uniform_query.cpp, shaderapi.c, and dlist.c.

I'm honestly inclined to close this as WONTFIX...

Looks like there's about 20 functions which would have be split into ARB (GLhandleARB) and non-ARB (GLuint) flavors.

I might take a crack at it someday...

I'd like to see Mesa compile on MacOS even if it's not a common thing.

I ran into this with epoxy, and in the process of trying to come up with a fix noted that all the GLhandleARBs are in the first 6 args of the functions, so they'll be stored in registers in amd64's calling convention.  That let me actually still treat them as aliases.

Not sure if that will be useful to solving mesa problems, but I thought it was an interesting note.

(In reply to comment #4)
> This is in glext.h:
> 
> #ifdef __APPLE__
> typedef void *GLhandleARB;
> #else
> typedef unsigned int GLhandleARB;
> #endif
> 
> Thanks, Apple!
> 
> I think Mesa is just broken on Apple, probably forever, because of this.  We
> assume *everywhere* that GLhandleARB == GLuint... to the point that
> glGetActiveAttrib and glGetActiveAttribARB (and many, many others) are the
> same function.

Although Apple's GLhandleARB are void *, I recall reading that only 32bits are used. (Unfortunately Apple can't go back in time and replace GLhandleARB with uints, as that would break binary compatibility.)

Furthermore what Apple does is irrelevant, as Mesa is free to do what it wants (ie, it's free to assume using GLhandleARB are always 32bits).

In short, all we have to do we fix the compiler errors, either:

1) typedefing GLhandleARB as uintptr_t internally when

  #ifdef __APPLE__
  #  ifdef BUILDING_MESA
  typedef uintptr_t GLhandleARB;
  #  else
  typedef void *GLhandleARB;
  #  endif
  #else
  typedef unsigned int GLhandleARB;
  #endif

2) add intermediate casts to (uintptr_t) between uint<->GLhandleARB conversions.

> This occurs in shader_query.cpp, uniforms.c, uniform_query.cpp, shaderapi.c,
> and dlist.c.
> 
> I'm honestly inclined to close this as WONTFIX...

Jeez, it seems nobody has patient for non-Linux platform these days.  Yes, cross-platform its often a pain, but not really the end of the world.


(In reply to comment #5)
> Looks like there's about 20 functions which would have be split into ARB
> (GLhandleARB) and non-ARB (GLuint) flavors.
> 
> I might take a crack at it someday...
> 
> I'd like to see Mesa compile on MacOS even if it's not a common thing.

Mesa and MacOS in general no, but compatibility with Gallium and MacOS is important for VMware down the road.

(In reply to comment #7)
> Furthermore what Apple does is irrelevant, as Mesa is free to do what it
> wants (ie, it's free to assume using GLhandleARB are always 32bits).

To be perfectly clear, Mesa is free to assume GLhandleARB are always 32bits integer *values*.  But sizeof(GLhandleARB) must match sizeof(void *) to avoid breaking binary compatibility of 64 bits MacOSX apps.

(In reply to comment #4)
> We
> assume *everywhere* that GLhandleARB == GLuint... to the point that
> glGetActiveAttrib and glGetActiveAttribARB (and many, many others) are the
> same function.

(In reply to comment #5)
> Looks like there's about 20 functions which would have be split into ARB
> (GLhandleARB) and non-ARB (GLuint) flavors.

Er.. Yes, I glossed over the aliasing issue.  So a proper fix is not as simple as I painted earlier.

It would however fix the build -- and leave it no less no more broken than before.

(In reply to comment #9)
> (In reply to comment #4)
> > We
> > assume *everywhere* that GLhandleARB == GLuint... to the point that
> > glGetActiveAttrib and glGetActiveAttribARB (and many, many others) are the
> > same function.
> 
> (In reply to comment #5)
> > Looks like there's about 20 functions which would have be split into ARB
> > (GLhandleARB) and non-ARB (GLuint) flavors.
> 
> Er.. Yes, I glossed over the aliasing issue.  So a proper fix is not as
> simple as I painted earlier.
> 
> It would however fix the build -- and leave it no less no more broken than
> before.

I've started a patch series to handle GLuint vs. GLhandleARB properly. I'll try to wrap it up tomorrow.

(In reply to comment #7)
> > Thanks, Apple!

You should thank the lack of standards regarding what GLhandleARB should've been defined as back in 2005.  It's no more Apple's fault than it is Mesa's.

Furthermore, we had been dealing with this in src/glx/apple without major issues through 8.0.5 (which was the last version before src/glx/apple stopped building in mesa due to the transition to autotools-only builds).  GLhandleARB was always 'void *' in mesa on OS X.

> > I think Mesa is just broken on Apple, probably forever, because of this.  We
> > assume *everywhere* that GLhandleARB == GLuint... 

Well, that's certainly incorrect and worthy of being fixed.  You can't make that assumption and should fix any code which does.

> > to the point that
> > glGetActiveAttrib and glGetActiveAttribARB (and many, many others) are the
> > same function.

An obvious bug that should be straight forward to address.
 
> Although Apple's GLhandleARB are void *, I recall reading that only 32bits
> are used.

I don't think you should not assume anything about the implementation.

> (Unfortunately Apple can't go back in time and replace GLhandleARB
> with uints, as that would break binary compatibility.)

Yes, but fixing this doesn't really have any binary compatibility impact in mesa since GLhandleARB has always been void * on darwin anyways.  Most of this is about fixing build failures and fixing correctness for other architectures to make mesa more robust in genera.

> Furthermore what Apple does is irrelevant, as Mesa is free to do what it
> wants (ie, it's free to assume using GLhandleARB are always 32bits).

Except for the fact that all the OpenGL functions in OpenGL.framework need to match the signature of the corresponding functions in mesa, since they're passed straight through.  That's why GLhandleARB has always been void * in mesa on darwin.

> In short, all we have to do we fix the compiler errors, either:
> 
> 1) typedefing GLhandleARB as uintptr_t internally when
> 
>   #ifdef __APPLE__
>   #  ifdef BUILDING_MESA
>   typedef uintptr_t GLhandleARB;
>   #  else
>   typedef void *GLhandleARB;
>   #  endif
>   #else
>   typedef unsigned int GLhandleARB;
>   #endif

Ugly, and this just continues to mask underlying bugs.

> 2) add intermediate casts to (uintptr_t) between uint<->GLhandleARB
> conversions.

Ugly and also just masks underlying bugs =/

That being said, #2 is possibly the safest option because we are at least forced to look at every problem point in the API.

Regression introduced with commit 9a14e412d6de93349a490a9c4534b52c3b524ee9 during mesa 9.1.

commit 9a14e412d6de93349a490a9c4534b52c3b524ee9
Author: Brian Paul <brianp@vmware.com>
Date:   Tue Jun 25 10:35:37 2013 -0600

    mesa: update glext.h to version 20130624
    
    In glapi_priv.h we always need the typedef for the GLclampx type
    since GL_OES_fixed_point is now defined in glext.h but the
    GLclampx type is not.  GLclampx is not used by anything in glext.h
    but we need it for GL ES dispatch.
    
    This is a huge patch because the structure of the file has been
    changed.
    
    The following extensions are new, however:
    
    GL_AMD_interleaved_elements
    GL_AMD_shader_trinary_minmax
    GL_IBM_static_data
    GL_INTEL_map_texture
    GL_NV_compute_program5
    GL_NV_deep_texture3D
    GL_NV_draw_texture
    GL_NV_shader_atomic_counters
    GL_NV_shader_storage_buffer_object
    GL_NVX_conditional_render
    GL_OES_byte_coordinates
    GL_OES_compressed_paletted_texture
    GL_OES_fixed_point
    GL_OES_query_matrix
    GL_OES_single_precision
    
    And these extensions were removed:
    
    GL_FfdMaskSGIX
    GL_INGR_palette_buffer
    GL_INTEL_texture_scissor
    GL_SGI_depth_pass_instrument
    GL_SGIX_fog_scale
    GL_SGIX_impact_pixel_texture
    GL_SGIX_texture_select
    
    Reviewed-by: José Fonseca <jfonseca@vmware.com>

Problem still exists:

CXX      main/shader_query.lo
main/shader_query.cpp:71:7: error: no matching function for call to '_mesa_lookup_shader_program_err'
      _mesa_lookup_shader_program_err(ctx, program, "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,
^
main/shader_query.cpp:139:13: error: no matching function for call to '_mesa_lookup_shader_program_err'
   shProg = _mesa_lookup_shader_program_err(ctx, program, "glGetActiveAttrib");
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../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,
^
main/shader_query.cpp:253:7: error: no matching function for call to '_mesa_lookup_shader_program_err'
      _mesa_lookup_shader_program_err(ctx, program, "glGetAttribLocation");
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../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,
^
3 errors generated.

----

gcc --version
Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 6.1.0 (clang-602.0.49) (based on LLVM 3.6.0svn)
Target: x86_64-apple-darwin14.3.0
Thread model: posix

---

OSX 10.10.3 / Dev Command Line Tools v6.3

Created attachment 115279 [details] [review]
use a function to convert from GLhandleARB to GLuint

Brian's patch rebased. Works for me.

Created attachment 115280 [details] [review]
shader_query: convert GLhandleARB to uintptr_t before troncating to GLuint

Other suggestion.

Created attachment 115566 [details] [review]
darwin: Suppress type conversion warnings for GLhandleARB

Patch from macports: https://trac.macports.org/browser/trunk/dports/x11/mesa/files/5002-darwin-Suppress-type-conversion-warnings-for-GLhandl.patch

A slightly different fix has landed as below. Perhaps one day we'll either get this upstreamed with Khronos or have our build apply/revert it on demand.
Not a huge deal either way :-)


 #ifdef __APPLE__
+#ifdef BUILDING_MESA
+/* Avoid uint <-> void* warnings */
+typedef unsigned long GLhandleARB;
+#else
 typedef void *GLhandleARB;
+#endif

mesa: 0166b4c165271bd7525a91049e58e390cb596c60 (master 10.7.0-devel)

Still see this build error. BUILDING_MESA is only defined for darwin DRI enabled builds.

(In reply to Vinson Lee from comment #18)
> mesa: 0166b4c165271bd7525a91049e58e390cb596c60 (master 10.7.0-devel)
> 
> Still see this build error. BUILDING_MESA is only defined for darwin DRI
> enabled builds.

It should be a matter of adding the define on SCons builds too.

But I don't know if there's much point for building Mesa w/ SCons on Mac -- we don't actually build an usable libGL there.

(In reply to José Fonseca from comment #19)
> (In reply to Vinson Lee from comment #18)
> > mesa: 0166b4c165271bd7525a91049e58e390cb596c60 (master 10.7.0-devel)
> > 
> > Still see this build error. BUILDING_MESA is only defined for darwin DRI
> > enabled builds.
> 
> It should be a matter of adding the define on SCons builds too.
> 
> But I don't know if there's much point for building Mesa w/ SCons on Mac --
> we don't actually build an usable libGL there.

BUILDING_MESA is also not defined on automake dri-disabled build.

(In reply to Vinson Lee from comment #20)
> (In reply to José Fonseca from comment #19)
> > (In reply to Vinson Lee from comment #18)
> > > mesa: 0166b4c165271bd7525a91049e58e390cb596c60 (master 10.7.0-devel)
> > > 
> > > Still see this build error. BUILDING_MESA is only defined for darwin DRI
> > > enabled builds.
> > 
> > It should be a matter of adding the define on SCons builds too.
> > 
> > But I don't know if there's much point for building Mesa w/ SCons on Mac --
> > we don't actually build an usable libGL there.
> 
> BUILDING_MESA is also not defined on automake dri-disabled build.
It took me a second read to understand what you meant here. Esp. since the code just does not work, with indirect rendering under MacOS/Darwin.

Please post the complete configure line that you're using. Thanks.

Should be fixed by:

commit a087a09fa86f9617af98f6294dd2228555a4891c
Author: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Date:   Wed Jan 20 16:59:45 2016 -0800

    mesa: Fix some function prototype mismatching
    
    main/api_exec.c:543:36: warning: incompatible pointer types passing 'void (GLhandleARB, GLuint, const GLcharARB *)' (aka 'void (unsigned long, unsigned int, const char *)') to
    parameter of
          type 'void (*)(GLuint, GLuint, const GLchar *)' (aka 'void (*)(unsigned int, unsigned int, const char *)') [-Wincompatible-pointer-types]
          SET_BindAttribLocation(exec, _mesa_BindAttribLocation);
                                       ^~~~~~~~~~~~~~~~~~~~~~~~
    ./main/dispatch.h:7590:88: note: passing argument to parameter 'fn' here
    static inline void SET_BindAttribLocation(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint, GLuint, const GLchar *)) {
                                                                                           ^
    main/api_exec.c:547:31: warning: incompatible pointer types passing 'void (GLhandleARB)' (aka 'void (unsigned long)') to parameter of type 'void (*)(GLuint)' (aka 'void (*)(unsigned
    int)')
          [-Wincompatible-pointer-types]
          SET_CompileShader(exec, _mesa_CompileShader);
                                  ^~~~~~~~~~~~~~~~~~~
    ./main/dispatch.h:7612:83: note: passing argument to parameter 'fn' here
    static inline void SET_CompileShader(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint)) {
                                                                                      ^
    main/api_exec.c:568:33: warning: incompatible pointer types passing 'void (GLhandleARB, GLuint, GLsizei, GLsizei *, GLint *, GLenum *, GLcharARB *)' (aka 'void (unsigned long,
    unsigned int,
          int, int *, int *, unsigned int *, char *)') to parameter of type 'void (*)(GLuint, GLuint, GLsizei, GLsizei *, GLint *, GLenum *, GLchar *)' (aka 'void (*)(unsigned int,
    unsigned int,
          int, int *, int *, unsigned int *, char *)') [-Wincompatible-pointer-types]
          SET_GetActiveAttrib(exec, _mesa_GetActiveAttrib);
                                    ^~~~~~~~~~~~~~~~~~~~~
    ./main/dispatch.h:7711:85: note: passing argument to parameter 'fn' here
    static inline void SET_GetActiveAttrib(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint, GLuint, GLsizei , GLsizei *, GLint *, GLenum *, GLchar *)) {
                                                                                        ^
    main/api_exec.c:571:35: warning: incompatible pointer types passing 'GLint (GLhandleARB, const GLcharARB *)' (aka 'int (unsigned long, const char *)') to parameter of type
          'GLint (*)(GLuint, const GLchar *)' (aka 'int (*)(unsigned int, const char *)') [-Wincompatible-pointer-types]
          SET_GetAttribLocation(exec, _mesa_GetAttribLocation);
                                      ^~~~~~~~~~~~~~~~~~~~~~~
    ./main/dispatch.h:7744:88: note: passing argument to parameter 'fn' here
    static inline void SET_GetAttribLocation(struct _glapi_table *disp, GLint (GLAPIENTRYP fn)(GLuint, const GLchar *)) {
                                                                                           ^
    main/api_exec.c:585:33: warning: incompatible pointer types passing 'void (GLhandleARB, GLsizei, GLsizei *, GLcharARB *)' (aka 'void (unsigned long, int, int *, char *)') to
    parameter of
          type 'void (*)(GLuint, GLsizei, GLsizei *, GLchar *)' (aka 'void (*)(unsigned int, int, int *, char *)') [-Wincompatible-pointer-types]
          SET_GetShaderSource(exec, _mesa_GetShaderSource);
                                    ^~~~~~~~~~~~~~~~~~~~~
    ./main/dispatch.h:7788:85: note: passing argument to parameter 'fn' here
    static inline void SET_GetShaderSource(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint, GLsizei, GLsizei *, GLchar *)) {
                                                                                        ^
    main/api_exec.c:597:29: warning: incompatible pointer types passing 'void (GLhandleARB)' (aka 'void (unsigned long)') to parameter of type 'void (*)(GLuint)' (aka 'void (*)(unsigned
    int)')
          [-Wincompatible-pointer-types]
          SET_LinkProgram(exec, _mesa_LinkProgram);
                                ^~~~~~~~~~~~~~~~~
    ./main/dispatch.h:7909:81: note: passing argument to parameter 'fn' here
    static inline void SET_LinkProgram(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint)) {
                                                                                    ^
    main/api_exec.c:628:30: warning: incompatible pointer types passing 'void (GLhandleARB, GLsizei, const GLcharARB *const *, const GLint *)' (aka
          'void (unsigned long, int, const char *const *, const int *)') to parameter of type 'void (*)(GLuint, GLsizei, const GLchar *const *, const GLint *)' (aka 'void (*)(unsigned
    int, int,
          const char *const *, const int *)') [-Wincompatible-pointer-types]
          SET_ShaderSource(exec, _mesa_ShaderSource);
                                 ^~~~~~~~~~~~~~~~~~
    ./main/dispatch.h:7920:82: note: passing argument to parameter 'fn' here
    static inline void SET_ShaderSource(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint, GLsizei, const GLchar * const *, const GLint *)) {
                                                                                     ^
    main/api_exec.c:653:28: warning: incompatible pointer types passing 'void (GLhandleARB)' (aka 'void (unsigned long)') to parameter of type 'void (*)(GLuint)' (aka 'void (*)(unsigned
    int)')
          [-Wincompatible-pointer-types]
          SET_UseProgram(exec, _mesa_UseProgram);
                               ^~~~~~~~~~~~~~~~
    ./main/dispatch.h:8173:80: note: passing argument to parameter 'fn' here
    static inline void SET_UseProgram(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint)) {
                                                                                   ^
    main/api_exec.c:655:33: warning: incompatible pointer types passing 'void (GLhandleARB)' (aka 'void (unsigned long)') to parameter of type 'void (*)(GLuint)' (aka 'void (*)(unsigned
    int)')
          [-Wincompatible-pointer-types]
          SET_ValidateProgram(exec, _mesa_ValidateProgram);
                                    ^~~~~~~~~~~~~~~~~~~~~
    ./main/dispatch.h:8184:85: note: passing argument to parameter 'fn' here
    static inline void SET_ValidateProgram(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint)) {
    
    main/dlist.c:9457:26: warning: incompatible pointer types passing 'void (GLhandleARB)' (aka 'void (unsigned long)') to parameter of type 'void (*)(GLuint)' (aka 'void (*)(unsigned
    int)')
          [-Wincompatible-pointer-types]
       SET_UseProgram(table, save_UseProgramObjectARB);
                             ^~~~~~~~~~~~~~~~~~~~~~~~
    ./main/dispatch.h:8173:80: note: passing argument to parameter 'fn' here
    static inline void SET_UseProgram(struct _glapi_table *disp, void (GLAPIENTRYP fn)(GLuint)) {
                                                                                   ^
    1 warning generated.
    
    Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
    Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>