| Message ID | 4381472f-a9db-b8a7-a395-81c3935309ae@kdbg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [cb/pedantic-build-for-developers] lazyload.h: fix warnings about mismatching function pointer types | expand |
On Wed, Sep 22, 2021 at 12:56 PM Johannes Sixt <j6t@kdbg.org> wrote: > > Here, GCC warns about every use of the INIT_PROC_ADDR macro, for example: > > In file included from compat/mingw.c:8: > compat/mingw.c: In function 'mingw_strftime': > compat/win32/lazyload.h:38:12: warning: assignment to > 'size_t (*)(char *, size_t, const char *, const struct tm *)' > {aka 'long long unsigned int (*)(char *, long long unsigned int, > const char *, const struct tm *)'} from incompatible pointer type > 'FARPROC' {aka 'long long int (*)()'} [-Wincompatible-pointer-types] > 38 | (function = get_proc_addr(&proc_addr_##function)) > | ^ > compat/mingw.c:1014:6: note: in expansion of macro 'INIT_PROC_ADDR' > 1014 | if (INIT_PROC_ADDR(strftime)) > | ^~~~~~~~~~~~~~ did you have CFLAGS adding -Wincompatible-pointer-types explicitly? This is the reason why the code that got merged to master had -Wno for this case. > (message wrapper for convenience). Insert a cast to keep the compiler > happy. A cast is fine in these cases because they are generic function > pointer values that have been looked up in a DLL. I have a more complete "fix" which I got stuck testing GGG[1]; you are likely going to also hit -Wcast-function-type otherwise. > > Signed-off-by: Johannes Sixt <j6t@kdbg.org> > --- > How can this have worked ever without a warning? POSIX have a specific exception that allows (void *) for this, it is incorrect though in platforms where pointers to code and data might be different (ex DOS with its segmented model) Carlo [1] https://github.com/git/git/pull/1094
Am 22.09.21 um 22:16 schrieb Carlo Arenas: > On Wed, Sep 22, 2021 at 12:56 PM Johannes Sixt <j6t@kdbg.org> wrote: >> >> Here, GCC warns about every use of the INIT_PROC_ADDR macro, for example: >> >> In file included from compat/mingw.c:8: >> compat/mingw.c: In function 'mingw_strftime': >> compat/win32/lazyload.h:38:12: warning: assignment to >> 'size_t (*)(char *, size_t, const char *, const struct tm *)' >> {aka 'long long unsigned int (*)(char *, long long unsigned int, >> const char *, const struct tm *)'} from incompatible pointer type >> 'FARPROC' {aka 'long long int (*)()'} [-Wincompatible-pointer-types] >> 38 | (function = get_proc_addr(&proc_addr_##function)) >> | ^ >> compat/mingw.c:1014:6: note: in expansion of macro 'INIT_PROC_ADDR' >> 1014 | if (INIT_PROC_ADDR(strftime)) >> | ^~~~~~~~~~~~~~ > > did you have CFLAGS adding -Wincompatible-pointer-types explicitly? I don't know of the top of my head (am not at that Windows box right now). I am fairly certain that I do not have DEVELOPER set. > This is the reason why the code that got merged to master had -Wno > for this case. > >> (message wrapper for convenience). Insert a cast to keep the compiler >> happy. A cast is fine in these cases because they are generic function >> pointer values that have been looked up in a DLL. > > I have a more complete "fix" which I got stuck testing GGG[1]; you are likely > going to also hit -Wcast-function-type otherwise. I think that the correct solution is that get_proc_addr() returns void*, not FARPROC. Then either no cast is needed (because void* can be converted to function pointer type implicitly) or a cast is needed and that is then not between incompatible function pointer types and should not trigger -Wcast-function-type, theoretically. >> --- >> How can this have worked ever without a warning? > > POSIX have a specific exception that allows (void *) for this,... Sure, but as you can see in the warning message, FARPROC is not void*, but a somewhat generic function pointer type. I was not questioning the assignment of function pointer values of different types, but the absence of a warning. -- Hannes
On Wed, Sep 22, 2021 at 2:21 PM Johannes Sixt <j6t@kdbg.org> wrote: > > Am 22.09.21 um 22:16 schrieb Carlo Arenas: > > On Wed, Sep 22, 2021 at 12:56 PM Johannes Sixt <j6t@kdbg.org> wrote: > >> > >> Here, GCC warns about every use of the INIT_PROC_ADDR macro, for example: > >> > >> In file included from compat/mingw.c:8: > >> compat/mingw.c: In function 'mingw_strftime': > >> compat/win32/lazyload.h:38:12: warning: assignment to > >> 'size_t (*)(char *, size_t, const char *, const struct tm *)' > >> {aka 'long long unsigned int (*)(char *, long long unsigned int, > >> const char *, const struct tm *)'} from incompatible pointer type > >> 'FARPROC' {aka 'long long int (*)()'} [-Wincompatible-pointer-types] > >> 38 | (function = get_proc_addr(&proc_addr_##function)) > >> | ^ > >> compat/mingw.c:1014:6: note: in expansion of macro 'INIT_PROC_ADDR' > >> 1014 | if (INIT_PROC_ADDR(strftime)) > >> | ^~~~~~~~~~~~~~ > > > > did you have CFLAGS adding -Wincompatible-pointer-types explicitly? > > I don't know of the top of my head (am not at that Windows box right > now). I am fairly certain that I do not have DEVELOPER set. the config.mak.uname for MinGW sets that for you. > > This is the reason why the code that got merged to master had -Wno > > for this case. > > > >> (message wrapper for convenience). Insert a cast to keep the compiler > >> happy. A cast is fine in these cases because they are generic function > >> pointer values that have been looked up in a DLL. > > > > I have a more complete "fix" which I got stuck testing GGG[1]; you are likely > > going to also hit -Wcast-function-type otherwise. Sadly, as predicted your fix, broke the build [1], so submitted[2] and adaptation of the original fix on top of yours with the rest of the code that will be needed so it can be added in top of js/win-lazyload-buildfix and merged into "seen" to fix that. > I think that the correct solution is that get_proc_addr() returns void*, > not FARPROC. Then either no cast is needed (because void* can be > converted to function pointer type implicitly) or a cast is needed and > that is then not between incompatible function pointer types and should > not trigger -Wcast-function-type, theoretically. The reasons behind why won't work are fairly academic IMHO, but there is an obvious clash between POSIX and C specs here and "pedantic" just reflects that. Note that the return type for GetProcAddress[3] is FARPROC, and the C standard says that any function pointer can be assigned to any other so your code should work, but gcc has no way to know that FARPROC is not the "real" function type and therefore warns that you might crash if using it. Carlo [1] https://github.com/git/git/runs/3680695336 [2] https://lore.kernel.org/git/20210923060306.21073-1-carenas@gmail.com/ [3] https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getprocaddress
On Wed, Sep 22, 2021 at 12:56 PM Johannes Sixt <j6t@kdbg.org> wrote: > diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h > index d2056cdadf..dc35cf080b 100644 > --- a/compat/win32/lazyload.h > +++ b/compat/win32/lazyload.h > @@ -26,7 +26,8 @@ struct proc_addr { > #define DECLARE_PROC_ADDR(dll, rettype, function, ...) \ > static struct proc_addr proc_addr_##function = \ > { #dll, #function, NULL, 0 }; \ > - static rettype (WINAPI *function)(__VA_ARGS__) > + typedef rettype (WINAPI *proc_type_##function)(__VA_ARGS__); \ > + static proc_type_##function function; dropping the trailing ";" here will also make this macro easier to use IMHO; feel free to drop all the hunks moving declarations around from my patch when you do if included in a reroll or let me know if I can help otherwise. Carlo
diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h index d2056cdadf..dc35cf080b 100644 --- a/compat/win32/lazyload.h +++ b/compat/win32/lazyload.h @@ -26,7 +26,8 @@ struct proc_addr { #define DECLARE_PROC_ADDR(dll, rettype, function, ...) \ static struct proc_addr proc_addr_##function = \ { #dll, #function, NULL, 0 }; \ - static rettype (WINAPI *function)(__VA_ARGS__) + typedef rettype (WINAPI *proc_type_##function)(__VA_ARGS__); \ + static proc_type_##function function; /* * Loads a function from a DLL (once-only). @@ -35,7 +36,7 @@ struct proc_addr { * This function is not thread-safe. */ #define INIT_PROC_ADDR(function) \ - (function = get_proc_addr(&proc_addr_##function)) + (function = (proc_type_##function)get_proc_addr(&proc_addr_##function)) static inline FARPROC get_proc_addr(struct proc_addr *proc) {
Here, GCC warns about every use of the INIT_PROC_ADDR macro, for example: In file included from compat/mingw.c:8: compat/mingw.c: In function 'mingw_strftime': compat/win32/lazyload.h:38:12: warning: assignment to 'size_t (*)(char *, size_t, const char *, const struct tm *)' {aka 'long long unsigned int (*)(char *, long long unsigned int, const char *, const struct tm *)'} from incompatible pointer type 'FARPROC' {aka 'long long int (*)()'} [-Wincompatible-pointer-types] 38 | (function = get_proc_addr(&proc_addr_##function)) | ^ compat/mingw.c:1014:6: note: in expansion of macro 'INIT_PROC_ADDR' 1014 | if (INIT_PROC_ADDR(strftime)) | ^~~~~~~~~~~~~~ (message wrapper for convenience). Insert a cast to keep the compiler happy. A cast is fine in these cases because they are generic function pointer values that have been looked up in a DLL. Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- How can this have worked ever without a warning? compat/win32/lazyload.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)