modules: use void* with GetProcAddress()
What matters is the type where we write the function pointer. It has to match the Windows SDK signature otherwise the calls will be bogus.
This is how it's used in most places already in C files.
Merge request reports
Activity
changed milestone to %4.0
added MRStatus::Reviewable label
That looks dubious. POSIX has some history leading to dlsym() returning void* instead of a function pointer, but there's no reason to have
void*
otherwise. The C standard only guarantee that casting a function pointer to another function pointer type still allows casting it back, as long as the function is not called from the wrong pointer type.That's the only way to silence warnings with
-Wcast-function-type-strict
. (or errors with-Werror=cast-function-type-strict
).As said in the commit, that type doesn't matter. What matters is the type of the variable that will be used as a function pointer.
And also "This is how it's used in most places already in C files.".
GCC mentions
void (*)(void)
for function casting but Clang doesn't like it.The function type void (*) (void) is special and matches everything, which can be used to suppress this warning. In a cast involving pointer to member types this warning warns whenever the type cast is changing the pointer to member type.
example (clang 18):
vlc/modules/video_output/win32/wgl.c:98:58: error: cast from 'PROC' (aka 'long long (*)()') to 'void (*)(void)' converts to incompatible function type [-Werror,-Wcast-function-type-strict] 98 | PFNWGLCREATEAFFINITYDCNVPROC fncCreateAffinityDCNV = (void(*)(void))wglGetProcAddress("wglCreateAffinityDCNV"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vlc/modules/video_output/win32/wgl.c:98:34: error: incompatible function pointer types initializing 'PFNWGLCREATEAFFINITYDCNVPROC' (aka 'struct HDC__ *(*)(struct HGPUNV__ *const *)') with an expression of type 'void (*)(void)' [-Wincompatible-function-pointer-types] 98 | PFNWGLCREATEAFFINITYDCNVPROC fncCreateAffinityDCNV = (void(*)(void))wglGetProcAddress("wglCreateAffinityDCNV"); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 errors generated.
with nothing:
vlc/modules/video_output/win32/wgl.c:98:34: error: incompatible function pointer types initializing 'PFNWGLCREATEAFFINITYDCNVPROC' (aka 'struct HDC__ *(*)(struct HGPUNV__ *const *)') with an expression of type 'PROC' (aka 'long long (*)()') [-Wincompatible-function-pointer-types] 98 | PFNWGLCREATEAFFINITYDCNVPROC fncCreateAffinityDCNV = wglGetProcAddress("wglCreateAffinityDCNV"); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.
with the actual target type:
vlc/modules/video_output/win32/wgl.c:98:58: error: cast from 'PROC' (aka 'long long (*)()') to 'PFNWGLCREATEAFFINITYDCNVPROC' (aka 'struct HDC__ *(*)(struct HGPUNV__ *const *)') converts to incompatible function type [-Werror,-Wcast-function-type-strict] 98 | PFNWGLCREATEAFFINITYDCNVPROC fncCreateAffinityDCNV = (PFNWGLCREATEAFFINITYDCNVPROC)wglGetProcAddress("wglCreateAffinityDCNV"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.
with
(void *)
there no error or warning.https://www.frama-c.com/2013/08/24/Function-pointers-in-C.html
Then you might want to avoid using
Wcast-function-type-strict
and report a bug to clang? Casting tovoid*
effectively removes the warning, but I very much prefer a warning over a potential UB effect that could break tooling and architectures.From C17 N2176 6.3.2.3 (Pointers) 8:
A pointer to a function of one type may be converted to a pointer to a function of another type and back again; the result shall compare equal to the original pointer. If a converted pointer is used to call a function whose type is not compatible with the referenced type, the behavior is undefined
GetProcAddress() returns a
FARPROC
which has various function signature based on target Windows version. This means that casting that function pointer to any other signature is Undefined Behavior and Windows programs work by chance.From C17 N2176 J.5.7 (Function pointer casts):
A pointer to an object or to void may be cast to a pointer to a function, allowing data to be invoked as a function (6.5.4).
A pointer to a function may be cast to a pointer to an object or to void, allowing a function to be inspected or modified (for example, by a debugger) (6.5.4)
This is a C17 extension that seems to be in GCC and Clang (and MSVC) which both don't complain when using (void*) to transform back and forth from function pointers (and complain with anything else). We should require that extension for Windows target if we don't want to leave
GetProcAddress()
using an Undefined Behavior.This becomes trickier with C++ where no such extension exists
Any pointer to function can be converted to a pointer to a different function type. The result is unspecified, but converting such pointer back to pointer to the original function type yields the pointer to the original function. The resulting pointer can only be called safely if it function type is call-compatible with the original function type.
And in fact, I have not found a way to convert the
FARPROC
to anything else without Clang complaining (this MR only contains changes for the C files). In such case we need to explicitly disable the warning (while allowing it for the rest of the code we want to check).GetProcAddress() returns a
FARPROC
which has various function signature based on target Windows version. This means that casting that function pointer to any other signature is Undefined Behavior and Windows programs work by chance.I don't see where the UB is. FARPROC is not the original signature.
The only requirements are that the caller of the function has used the correct signature.
Which the compiler doesn't know. It cannot check you're doing it properly or not. You're allowed to do a conversion to another type and back. But if it doesn't know what is the correct "back" it can't verify the code is legit.
The compiler doesn't care.
A pointer to a function of one type may be converted to a pointer to a function of another type and back again; the result shall compare equal to the original pointer
== function pointer cast to another function pointer is not UB
If a converted pointer is used to call a function whose type is not compatible with the pointed-to type, the behavior is undefined
== what matters to qualify the dereferencing call as UB is the pointee, not where the pointer is coming from.
The compiler doesn't care.
It clearly does. And this is not a bug in Clang. The same thing happens with gcc:
modules/hw/amf/amf_helper.c:26:39: warning: cast between incompatible function types from ‘FARPROC’ {aka ‘long long int (*)()’} to ‘AMF_RESULT (*)(amf_uint64 *)’ {aka ‘AMF_RESULT (*)(long long unsigned int *)’} [-Wcast-function-type] 26 | AMFQueryVersion_Fn queryVersion = (AMFQueryVersion_Fn)GetProcAddress(hLib, AMF_QUERY_VERSION_FUNCTION_NAME); | ^
The only thing the compiler knows is the
FARPROC
.I do understand perfectly that from the point of view of the compiler the casting from a function pointer type to another function pointer type is an undefined behavior. And then
If a converted pointer is used to call a function whose type is not compatible with the pointed-to type, the behavior is undefined
And since
not suitable for dynamic loading of symbol
We have to explicitly tell the compiler that this is actually OK.
I do understand perfectly that from the point of view of the compiler the casting from a function pointer type to another function pointer type is an undefined behavior.
The first sentence you quoted from the standard tells the exact opposite, as well as the links I've sent and what Denis said.
A pointer to a function of one type (pt1 of type FARPROC) may be converted to a pointer to a function of another type (pt of type AMFQueryVersion_Fn) and back again (pt2 of type FARPROC); the result (pt1 of type FARPROC) shall compare equal to the original pointer (pt2 of type FARPROC).
This is how the compiler interprets this sentence (pt1 = pt2).
Edited by Steve LhommeThe argument here is that if the function is called with the correct arguments, you can cast it to anything you want and re-cast it back. Which is what AMF expects its users to do.
The clang warning is unsuited for being enabled on the whole codebase for any platform for this specific reason, it should be used to find abnormal casts but does not strictly point at UB. The function calls can be UB if the caller interprets the doc incorrectly.
We have to explicitly tell the compiler that this is actually OK.
I suggest disabling the warning for files interacting with these kinds of APIs.
Edited by Alaric SenatI suggest disabling the warning for files interacting with these kinds of APIs.
I agree. In part because there's no cast that can help in C++. And I started working on that. In the end the void* cast should not be needed for C files.
Function casting to a different signature is one of the nastiest thing there can be in a code after a NULL deference. We should have strong checks against this wherever possible, even in Windows only code.
added MRStatus::InReview label and removed MRStatus::Reviewable label
mentioned in commit robUx4/vlc@81be24da
Casting to
void *
makes sense in generic wrappers that can return any function address. Though perhapsvoid (*)()
would be best, it risks triggering incomplete prototype warnings.But I kinda agree with Alex here. It's just silly if we know what the correct fucntion type is at a given site.
added MRStatus::NotCompliant label and removed MRStatus::InReview label
mentioned in commit robUx4/vlc@506bf92b
mentioned in commit robUx4/vlc@381175ef
mentioned in commit robUx4/vlc@77f7adb9