patchlazyload.h: fix warnings about mismatching function pointer types

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(-)

On Wed, Sep 22, 2021 at 12:56 PM Johannes Sixt <j6t@kdbg.org> wrote:

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.

I have a more complete "fix" which I got stuck testing GGG[1]; you are likely going to also hit -Wcast-function-type otherwise.

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:

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.

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.

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:

the config.mak.uname for MinGW sets that for you.

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.

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

gcc will helpfully raise a -Wcast-function-type warning when casting between functions that might have incompatible return types (ex: GetUserNameExW returns bool which is only half the size of the return type from FARPROC which is long long), so create a new type that could be used as a completely generic function pointer and cast through it instead.

Additionaly remove the -Wno-incompatible-pointer-types temporary flag added in 27e0c3c (win32: allow building with pedantic mode enabled, 2021-09-03), as it will be no longer needed.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 compat/win32/lazyload.h | 9 ++++++---
 config.mak.dev          | 1 -
 2 files changed, 6 insertions(+), 4 deletions(-)

Am 23.09.21 um 08:03 schrieb Carlo Marcelo Arenas Belón:

IIUC, this patch goes on top of mine in origin/js/win-lazyload-buildfix, right?

On Wed, Sep 22, 2021 at 11:33 PM Johannes Sixt <j6t@kdbg.org> wrote:

yes, but it has a typo :( please use v2; apologies

Carlo

gcc will helpfully raise a -Wcast-function-type warning when casting between functions that might have incompatible return types (ex: GetUserNameExW returns bool which is only half the size of the return type from FARPROC which is long long), so create a new type that could be used as a completely generic function pointer and cast through it instead.

Because of the way the function declaration was done in the previous patch the order of variables that use it had to be adjusted so that it is the last variable declared, as well.

Additionaly remove the -Wno-incompatible-pointer-types temporary flag added in 27e0c3c (win32: allow building with pedantic mode enabled, 2021-09-03), as it will be no longer needed.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2:
* fixes a silly typo in the previous one
* adds more places where -Wdeclaration-after-statement will trigger

 compat/mingw.c              | 4 ++--
 compat/win32/lazyload.h     | 9 ++++++---
 config.mak.dev              | 1 -
 t/helper/test-drop-caches.c | 2 +-
 4 files changed, 9 insertions(+), 7 deletions(-)

To easy the maintainer reroll of this branch with the fixup[1] squashed and the suggested[2] cleanup in the second patch to minimize change and readiness for next.

Carlo Marcelo Arenas Belón (1):
  lazyload.h: use an even more generic function pointer than FARPROC

Johannes Sixt (1):
  lazyload.h: fix warnings about mismatching function pointer types

 compat/win32/lazyload.h | 14 +++++++++-----
 config.mak.dev          |  1 -
 2 files changed, 9 insertions(+), 6 deletions(-)

[1] https://lore.kernel.org/git/3f963bba-3197-8c52-9828-6d78ef1d25b1@kdbg.org/T/#u [2] https://lore.kernel.org/git/CAPUEspivB=07OponiMrfXFBrC+L7qjSUuZEV9q-Ug5Z_ShnFNA@mail.gmail.com/

Range-diff against v2:
1:  79354f549e ! 1:  c389b7ad9d lazyload.h: fix warnings about mismatching function pointer types
    @@ Commit message
         happy. A cast is fine in these cases because they are generic function
         pointer values that have been looked up in a DLL.
     
    +    Helped-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
         Signed-off-by: Johannes Sixt <j6t@kdbg.org>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## compat/win32/lazyload.h ##
     @@ compat/win32/lazyload.h: struct proc_addr {
    @@ compat/win32/lazyload.h: struct proc_addr {
      	{ #dll, #function, NULL, 0 }; \
     -	static rettype (WINAPI *function)(__VA_ARGS__)
     +	typedef rettype (WINAPI *proc_type_##function)(__VA_ARGS__); \
    -+	static proc_type_##function function;
    ++	static proc_type_##function function
      
      /*
       * Loads a function from a DLL (once-only).
2:  912c443bde ! 2:  b1efbe2c89 lazyload.h: use an even more generic function pointer than FARPROC
    @@ Commit message
         enabled, 2021-09-03), as it will be no longer needed.
     
         Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
    -
    - ## compat/mingw.c ##
    -@@ compat/mingw.c: enum EXTENDED_NAME_FORMAT {
    - 
    - static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type)
    - {
    --	DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW,
    --		enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG);
    - 	static wchar_t wbuffer[1024];
    - 	DWORD len;
    -+	DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW,
    -+		enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG);
    - 
    - 	if (!INIT_PROC_ADDR(GetUserNameExW))
    - 		return NULL;
     
      ## compat/win32/lazyload.h ##
     @@
    @@ config.mak.dev: DEVELOPER_CFLAGS += -pedantic
      endif
      endif
      DEVELOPER_CFLAGS += -Wdeclaration-after-statement
    -
    - ## t/helper/test-drop-caches.c ##
    -@@ t/helper/test-drop-caches.c: static int cmd_dropcaches(void)
    - {
    - 	HANDLE hProcess = GetCurrentProcess();
    - 	HANDLE hToken;
    --	DECLARE_PROC_ADDR(ntdll.dll, DWORD, NtSetSystemInformation, INT, PVOID, ULONG);
    - 	SYSTEM_MEMORY_LIST_COMMAND command;
    - 	int status;
    -+	DECLARE_PROC_ADDR(ntdll.dll, DWORD, NtSetSystemInformation, INT, PVOID, ULONG);
    - 
    - 	if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, &hToken))
    - 		return error("Can't open current process token");

From: Johannes Sixt <j6t@kdbg.org>

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.

Helped-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
v3:
- squashes fixup, acked by Hannes

 compat/win32/lazyload.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On Sun, Sep 26, 2021 at 6:05 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

s/wrapper/wrapped/

gcc will helpfully raise a -Wcast-function-type warning when casting between functions that might have incompatible return types (ex: GetUserNameExW returns bool which is only half the size of the return type from FARPROC which is long long), so create a new type that could be used as a completely generic function pointer and cast through it instead.

Because of the way the function declaration was done in the previous patch the order of variables that use it had to be adjusted so that it is the last variable declared, as well.

Additionaly remove the -Wno-incompatible-pointer-types temporary flag added in 27e0c3c (win32: allow building with pedantic mode enabled, 2021-09-03), as it will be no longer needed.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v3
- removes unnecessary variable moving after fixup in previous patch

 compat/win32/lazyload.h | 9 ++++++---
 config.mak.dev          | 1 -
 2 files changed, 6 insertions(+), 4 deletions(-)

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

Is it clear to everybody what this paragraph is referring to? It is not, at least to me.

Thanks.

On Mon, Sep 27, 2021 at 9:35 AM Junio C Hamano <gitster@pobox.com> wrote:

It is not, and it is no longer needed after the fixup was applied to the previous patch. Do you want me to send another series removing it or can be done while applying?

It was slightly better explained in the fixup[1] commit message.

Because of the double ';', When the macro was used to declare a function variable and it was not the last variable declared, then it will trigger -Wdeclaration-after-statement.

Carlo

[1] https://lore.kernel.org/git/3f963bba-3197-8c52-9828-6d78ef1d25b1@kdbg.org/

Carlo Arenas <carenas@gmail.com> writes:

Ah, yes, the previous one's definition is quite awkward because you had to omit the terminating ';' to use it.

I'll drop the paragraph with "commit --amend".

THanks.

This series includes all changes from v3 and an additional patch that is based on the one that was originally[1] published as patch 1 from a different series that depended on this one.

Range-diff looks weird, but that might be just another bug in format-patch as it looked fine when calling range-diff directly.

[1] https://lore.kernel.org/git/20210928091054.78895-2-carenas@gmail.com/

Carlo Marcelo Arenas Belón (2):
  lazyload.h: use an even more generic function pointer than FARPROC
  Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

Johannes Sixt (1):
  lazyload.h: fix warnings about mismatching function pointer types

 compat/win32/lazyload.h | 14 +++++++++-----
 config.mak.dev          |  8 ++++++--
 config.mak.uname        |  8 +++++++-
 3 files changed, 22 insertions(+), 8 deletions(-)

Range-diff against v3: -: ---------- > 1: d2c470f9bc lazyload.h: fix warnings about mismatching function pointer types -: ---------- > 2: 2d84c4ed57 lazyload.h: use an even more generic function pointer than FARPROC 1: a731848d01 = 3: a731848d01 Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

From: Johannes Sixt <j6t@kdbg.org>

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 wrapped 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.

Helped-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 compat/win32/lazyload.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

gcc will helpfully raise a -Wcast-function-type warning when casting between functions that might have incompatible return types (ex: GetUserNameExW returns bool which is only half the size of the return type from FARPROC which is long long), so create a new type that could be used as a completely generic function pointer and cast through it instead.

Additionaly remove the -Wno-incompatible-pointer-types temporary flag added in 27e0c3c (win32: allow building with pedantic mode enabled, 2021-09-03), as it will be no longer needed.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/win32/lazyload.h | 9 ++++++---
 config.mak.dev          | 1 -
 2 files changed, 6 insertions(+), 4 deletions(-)

6a8cbc41ba (developer: enable pedantic by default, 2021-09-03) enables pedantic mode in as many compilers as possible to help gather feedback on future tightening, so lets do so.

-Wpedantic is missing in some really old gcc 4 versions so lets restrict it to gcc5 and clang4 (it does work in clang3 AFAIK, but it will be unlikely that a developer will use such an old compiler anyway).

MinGW gcc is the only one which has -Wno-pedantic-ms-format, and while that is available also in older compilers, the Windows SDK provides gcc10 so let's aim for that.

Note that in order to target the flag to only Windows, additional changes were needed in config.mak.uname to propagate the OS detection which also did some minor refactoring, but which is functionaly equivalent.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 config.mak.dev   | 7 ++++++-
 config.mak.uname | 8 +++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

On 29/09/2021 01:48, Carlo Marcelo Arenas Belón wrote:

This export seems to be unnecessary.

ATB, Ramsay Jones

This is a reroll of v3, merged in seen at 906546091d (Merge branch 'js/win-lazyload-buildfix' into jch, 2021-09-28) and that provides a buildfix for a non standard setup found and provided by Hannes, plus cleanup for confusing issues reported by Ævar and Jonathan Tan due to the original implementation of cb/pedantic-build-for-developers now in master.

v5
- remove unnecessary export of a make variable as suggested by Ramsay

v4
- add a third patch based on feedback from cb/pedantic-build-for-developers
  in master and including suggestions by Ævar on a similar patch posted as
  part of a different series.

v3
- fixup to patch1 to help usability and typo fixes from Eric and Junio.

v2
- add a second patch to avoid CI build issues caused by the first and retire
  now obsoleted -Wno-incompatible-pointer-types.

v1
- first patch with a build fix when -Wno-incompatible-pointer-types (included
  with 6a8cbc41ba (developer: enable pedantic by default, 2021-09-03)) was
  not in effect.

Carlo Marcelo Arenas Belón (2):
  lazyload.h: use an even more generic function pointer than FARPROC
  Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

Johannes Sixt (1):
  lazyload.h: fix warnings about mismatching function pointer types

 compat/win32/lazyload.h | 14 +++++++++-----
 config.mak.dev          |  8 ++++++--
 config.mak.uname        |  6 +++++-
 3 files changed, 20 insertions(+), 8 deletions(-)

Range-diff against v4:
1:  d2c470f9bc = 1:  d2c470f9bc lazyload.h: fix warnings about mismatching function pointer types
2:  2d84c4ed57 = 2:  2d84c4ed57 lazyload.h: use an even more generic function pointer than FARPROC
3:  a731848d01 ! 3:  0d4f097db9 Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better
    @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
      	pathsep = ;
      	HAVE_ALLOCA_H = YesPlease
      	NO_PREAD = YesPlease
    -@@ config.mak.uname: ifeq ($(uname_S),QNX)
    - 	NO_STRLCPY = YesPlease
    - endif
    - 
    -+export uname_S
    -+
    - vcxproj:
    - 	# Require clean work tree
    - 	git update-index -q --refresh && \

From: Johannes Sixt <j6t@kdbg.org>

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 wrapped 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.

Helped-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/win32/lazyload.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

gcc will helpfully raise a -Wcast-function-type warning when casting between functions that might have incompatible return types (ex: GetUserNameExW returns bool which is only half the size of the return type from FARPROC which is long long), so create a new type that could be used as a completely generic function pointer and cast through it instead.

Additionaly remove the -Wno-incompatible-pointer-types temporary flag added in 27e0c3c (win32: allow building with pedantic mode enabled, 2021-09-03), as it will be no longer needed.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/win32/lazyload.h | 9 ++++++---
 config.mak.dev          | 1 -
 2 files changed, 6 insertions(+), 4 deletions(-)

6a8cbc41ba (developer: enable pedantic by default, 2021-09-03) enables pedantic mode in as many compilers as possible to help gather feedback on future tightening, so lets do so.

-Wpedantic is missing in some really old gcc 4 versions so lets restrict it to gcc5 and clang4 (it does work in clang3 AFAIK, but it will be unlikely that a developer will use such an old compiler anyway).

MinGW gcc is the only one which has -Wno-pedantic-ms-format, and while that is available also in older compilers, the Windows SDK provides gcc10 so lets aim for that.

Note that in order to target the flag to only Windows, additional changes were needed in config.mak.uname to propagate the OS detection which also did some minor refactoring, but which is functionaly equivalent.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 config.mak.dev   | 7 ++++++-
 config.mak.uname | 6 +++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

On Wed, Sep 22, 2021 at 12:56 PM Johannes Sixt <j6t@kdbg.org> wrote:

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