This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• Patch to use SDL_FunctionPointer everywhere. (bug4131.patch, text/plain, 2018-06-07 02:06:36 +0000, 31641 bytes)

Reported in version: 2.0.8
Reported for operating system, platform: Other, All

Comments on the original bug report:

On 2018-04-12 03:06:18 +0000, Mark Callow wrote:

SDL_GL_GetProcAddress is defined as returning void*, which is an object pointer not a function pointer.

This leads to C++ code like

glDrawTexsOES =
(PFNGLDRAWTEXSOESPROC)SDL_GL_GetProcAddress("glDrawTexsOES");

getting compile warnings or, in earlier versions of C++, errors. E.g., clang warns

Cast between pointer-to-function and pointer-to-object is an extension

SDL_GL_GetProcAddress should be defined something like:

typedef void (*voidfuncptr)(void);
voidfuncptr SDL_GL_GetProcAddress(...);

to indicate it returns a function pointer. It will be then portable, not to mention correct.

On 2018-04-12 16:52:40 +0000, Ryan C. Gordon wrote:

typedef void (*voidfuncptr)(void);
voidfuncptr SDL_GL_GetProcAddress(...);

to indicate it returns a function pointer. It will be then portable, not to
mention correct.

I'm not against doing something here if there's a notation that lets us dictate "this is a void * that is meant to point at a code segment and not data"--some attribute keyword that makes Clang happy or whatnot--but I think it's less correct to change this to a type of (void ()(void)). With a void, you know you have to cast this thing before you can call it. With that typedef, you can (with likely-disastrous results) call it directly.

If this is strictly a C++ compiler problem, I'd say the appropriate solution is for the calling C++ code to not use a C-style cast, but rather a reinterpret_cast<>, but I don't know if this warning is also leaking into modern C compilers too.

--ryan.

On 2018-04-12 22:48:26 +0000, Mark Callow wrote:

1. reinterpret_cast gets the warning too.

2. The Vulkan API uses

typedef void (VKAPI_PTR *PFN_vkVoidFunction)(void);

typedef PFN_vkVoidFunction (VKAPI_PTR PFN_vkGetInstanceProcAddr)(VkInstance instance, const char pName);

VKAPI_ATTR PFN_vkVoidFunction VKAPI_CALL vkGetInstanceProcAddr(
VkInstance instance,
const char* pName);

VKAPI_ATTR PFN_vkVoidFunction VKAPI_CALL vkGetDeviceProcAddr(
VkDevice device,
const char* pName);

It doesn't seem to have caused any problems. Since all the function pointers to which the results of vkGet{Instance,Device}ProcAddress are assigned have different types from PFN_vkVoidFunction, you will get a compile warning if you don't cast appropriately. If you assign the result to a variable of type PFN_vkVoidFunction and then call through that, you kind of get what you deserve and the error is extremely unlikely to leak into released code.

3. It has been written that very recent versions of C++ support casting object poinetrs to function pointers. I have not tried to verify this.

On 2018-04-13 01:48:57 +0000, Mark Callow wrote:

recent versions of C++

I refer here to versions of the C++ standard not compilers.

On 2018-04-13 21:50:22 +0000, Daniel Gibson wrote:

I think it's less correct to change this to a type of (void ()(void)).
With a void, you know you have to cast this thing before you can call it.
With that typedef, you can (with likely-disastrous results) call it directly.

Also: I can imagine that casting from "void ()(void)" (or similar) to "int ()(SDL_Window*)" or whatever violates strict aliasing rules.

(No idea if that's really the case, /maybe/ function pointers have different aliasing rules than pointers to "normal" types? But before changing anything that might trade a theoretical "pointing void* to a function pointer is implementation defined and works on all relevant platforms, but -Wpedantic complains" problem for a very real strict-aliasing problem, this should probably be looked up by someone familiar with the C and/or C++ standards)

I think in C you might be able to use "void () ()" so at least the types of the function arguments are unspecified (=> could be anything), but in C++ it's equivalent to "void ()(void)" (=> no arguments) - and this still doesn't solve the problem of the return value type.

related: https://gcc.gnu.org/ml/gcc-help/2006-11/msg00301.html
"You have aked the compiler to warn you about non-ISO operations, and
you then want to perform non-ISO operations without warnings!"
which reminds me that DLLs/libs, and thus dlsym() and similar, are not part of the C standard, so when using them you probably shouldn't expect to conform to pedantic ISO C.

On 2018-04-14 01:32:07 +0000, Mark Callow wrote:

(In reply to Daniel Gibson from comment # 4)

Also: I can imagine that casting from "void ()(void)" (or similar) to "int
()(SDL_Window*)" or whatever violates strict aliasing rules.

What's violating strict aliasing rules is casting from an object pointer to a function pointer. It's not been allowed in C++ until, I've seen claimed, C++11.
Clang supports it, hence I'm seeing a warning not an error.

Furthermore, as I stated, this is working fine for Vulkan. Both vulkan.h and vulkan.hpp use void (*)(void) as the return type for ProcAddress getters. And so does EGL/GL. In egl.h you will find

typedef void (*__eglMustCastToProperFunctionPointerType)(void);

which is the return type for eglGetProcAddress. It's been that way for going on 16 years and hasn't caused any problems I am aware of. This is the function that underpins SDL_GL_GetProAddress on a large number of platforms.

Even Windows has wglGetProcAddress returning a "PROC". I'm not on Windows at present and can't find the definition of PROC on the web but I'm pretty sure with a name like that it is not void*

Using a verbose typename like EGL's will satisfy Ryan's concern.

So hey, let's trade off theoretical concerns not born out in practice in favor of not fixing an actual problem. :-)

On 2018-04-18 13:55:24 +0000, Mark Callow wrote:

Does anybody object to me fixing this as described. I'll happily use a verbose type name like EGL if that makes people feel easier.

On 2018-04-18 17:53:23 +0000, Sam Lantinga wrote:

PROC is defined as: int (WINAPI *PROC)();

I'm okay with making this change internally (SDL_FUNCTION_PTR typedef?) and then making it public for API functions in SDL 2.1 when we can change the ABI.

On 2018-04-22 13:13:21 +0000, Mark Callow wrote:

(In reply to Sam Lantinga from comment # 7)

I'm okay with making this change internally (SDL_FUNCTION_PTR typedef?) and
then making it public for API functions in SDL 2.1 when we can change the
ABI.

I'll be off for a couple of days. I'll look at this when I return. A couple of questions in the meantime:

• When do you expect to release SDL 2.1?
• Does this really change the ABI? IIUC C function names used by the linker don't provide return type info.
• Is there a tool for comparing ABIs and checking for changes?

Okay, so there are actually 3 questions.

On 2018-05-19 10:42:56 +0000, Mark Callow wrote:

Some other works took more time that expected but I am ready to take a look at this now. So I'd appreciate answers to my questions:

(In reply to Mark Callow from comment # 8)

...

• When do you expect to release SDL 2.1?
• Does this really change the ABI? IIUC C function names used by the linker
  don't provide return type info.
• Is there a tool for comparing ABIs and checking for changes?

On 2018-05-20 14:18:08 +0000, Mark Callow wrote:

Which do you prefer for the typedef name:

1. SDL_PFN_VoidFunction
2. PFN_SDL_VoidFunction
3. SDL_VoidFunctionPtr
4. Something else?

I am leaning towards # 1.

The Vulkan support has the same issue. I hadn't noticed because I am not using SDL_Vulkan_GetVkGetInstanceProcAddr in my apps. I let the linker resolve calls to vkGetInstanceProcAddress when linking with libvulkan.

Should I use the already defined Vulkan typedef, PFN_vkGetInstanceProcAddr, for that function or should I use whichever of 1 ~ 4 above is chosen? I think I prefer to use the Vulkan typedef since it already exists.

On 2018-05-21 00:53:09 +0000, Mark Callow wrote:

Another choice for the list:

SDL_MustCastToProperFunctionPointerType

Looking forward to hearing opinions.

On 2018-05-21 02:54:38 +0000, Sam Lantinga wrote:

Actually, I think void* and void(*)() are the same size on all supported platforms. If that's true, then we can safely change the API in a minor revision.

I'm leaning towards SDL_FunctionPointer, but that's just me.

If the vulkan support requires the vulkan includes in the SDL public header and returns vulkan typedefs, then we should return those in the API. If we don't require vulkan includes in the public SDL header then we should return SDL_FunctionPointer.

On 2018-05-21 10:08:09 +0000, Mark Callow wrote:

(In reply to Sam Lantinga from comment # 12)

Actually, I think void* and void(*)() are the same size on all supported
platforms. If that's true, then we can safely change the API in a minor
revision.
Okay.

I'm leaning towards SDL_FunctionPointer, but that's just me.
Okay. Let's see if there are any other opinions.

If the vulkan support requires the vulkan includes in the SDL public header
and returns vulkan typedefs, then we should return those in the API. If we
don't require vulkan includes in the public SDL header then we should return
SDL_FunctionPointer.

If we make the return type PFN_vkGetInstanceProcAddr, which is the pointer defined for the purpose in vulkan.h no cast is necessary. We don't require Vulkan includes because the public SDL_vulkan.h declares the few things the SDL_Vulkan API currently needs which happen to include a typedef for VkInstance. So it is trivial to add a typedef there for PFN_vkGetInstanceProcAddr.

On 2018-05-21 20:51:08 +0000, Sam Lantinga wrote:

Sounds good.

Sleeping on it, I like SDL_FunctionPointer more. It's in line with our other types and sounds generic.

Do you want to whip up a patch that makes that change?

On 2018-05-23 04:26:09 +0000, Mark Callow wrote:

Yes, I'll create a patch. Is there a particular header I should use for declaring the typedef? I was thinking of putting it in SDL_video.h.

On 2018-05-23 15:51:21 +0000, Daniel Gibson wrote:

If this is changed, it should be in SDL_loadso.h, and SDL_LoadFunction() should also use it

On 2018-05-23 23:07:00 +0000, Sam Lantinga wrote:

Yes, I agree with Daniel

On 2018-05-23 23:49:12 +0000, Daniel Gibson wrote:

By the way, regarding

Actually, I think void* and void(*)() are the same size on all supported
platforms. If that's true, then we can safely change the API in
a minor revision.

yes, it's probably the same size, but it will at least generate warnings if assigned to a function pointer of different type without a cast (in C, in C++ it needs an explicit cast either way)

if, on Linux with GCC or clang and -Wall you do sth like:
int (* fnPtrVar)(float) = SDL_LoadFunction(handle, "SomeFunction");
it currently, i.e. with SDL_LoadFunction() returning void*, compiles without warning (in plain C).

If SDL_LoadFunction() returns SDL_FunctionPointer (defined with "typedef void(*SDL_FunctionPointer)();"), you get a warning like

test.c:23:8: warning: incompatible pointer types initializing 'int ()(float)'
with an expression of type 'SDL_FunctionPointer' (aka 'void ()()')
[-Wincompatible-pointer-types]
int (* fnPtrVar)(float) = SDL_LoadFunction(handle, "SomeFunction");

This might break builds of software building with -Werror (or -Werror=incompatible-pointer-types), so I'm not sure it's really a good idea to put this in a minor revision.

On 2018-05-24 02:26:58 +0000, Mark Callow wrote:

(In reply to Daniel Gibson from comment # 18)

This might break builds of software building with -Werror (or
-Werror=incompatible-pointer-types), so I'm not sure it's really a good idea
to put this in a minor revision.

I was already starting to use the definition within SDL and I've found a few places in the SDL source that lack casts so this warning starts to happen. E.g in SDL_GL_ExtensionSupported

const GLubyte *(APIENTRY * glGetStringiFunc) (GLenum, GLuint);

glGetStringiFunc = SDL_GL_GetProcAddress("glGetStringi");

So I agree with Daniel this change should wait for 2.1.

BTW, is there any reason for the explicit declaration of the type of glGetStringiFunc. It would be much easier to use PFNGLGETSTRINGIPROC, which is already defined in SDL_opengl_glext.h, especially if adding the cast.

When I'm done, I'll attach a patch here as there doesn't seem to be a place in the repo for 2.1 specific changes.

On 2018-05-31 10:01:50 +0000, Mark Callow wrote:

My work on this has been delayed due to issues cropping up in another project. Just wanted to let everyone know I am still working on this. I have fixed the platform independent parts, the Cocoa video driver and testgl2.

On 2018-06-07 02:06:36 +0000, Mark Callow wrote:

Created attachment 3256
Patch to use SDL_FunctionPointer everywhere.

I've only compiled and tested on iOS & macOS but as it is a simple fix, barring a stupid typo, everything should work.

The patch is against changeset 12010.

I also fixed SDL_LoadFunction.

I have made SDL_Vulkan_GetVkGetInstanceProcAddr also return a type of SDL_FunctionPointer (a) for consistency and (b) because Android requires an attribute setting and I didn't fancy copying in the ifdefs that set VK_APIPTR, where the attribute is set in the Android case, from vulkan.h. The definition of PFN_vkGetInstanceProcAddr that I see I've left in SDL_vulkan.h should probably be removed.