[PATCH cb/pedantic-build-for-developers] lazyload.h: fix warnings about mismatching function pointer types

[PATCH cb/pedantic-build-for-developers] lazyload.h: fix warnings about mismatching function pointer types

[Johannes Sixt]

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

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)
 {
-- 
2.33.0.130.g83e2707afc

Re: [PATCH cb/pedantic-build-for-developers] lazyload.h: fix warnings about mismatching function pointer types

[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?

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

Re: [PATCH cb/pedantic-build-for-developers] lazyload.h: fix warnings about mismatching function pointer types

[Johannes Sixt]

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

Re: [PATCH cb/pedantic-build-for-developers] lazyload.h: fix warnings about mismatching function pointer types

[Carlo Arenas]

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

[PATCH] lazyload.h: use an even more generic function pointer than FARPROC

[Carlo Marcelo Arenas Belón]

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

diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
index dc35cf080b..26c80f7833 100644
--- a/compat/win32/lazyload.h
+++ b/compat/win32/lazyload.h
@@ -15,10 +15,12 @@
  *                        source, target);
  */
 
+typedef void (*FARVOIDPROC)(void);
+
 struct proc_addr {
 	const char *const dll;
 	const char *const function;
-	FARPROC pfunction;
+	FARVOIDPROC pfunction;
 	unsigned initialized : 1;
 };
 
@@ -38,7 +40,7 @@ struct proc_addr {
 #define INIT_PROC_ADDR(function) \
 	(function = (proc_type_##function)get_proc_addr(&proc_addr_##function))
 
-static inline FARPROC get_proc_addr(struct proc_addr *proc)
+static inline FARVOIDPROC get_proc_addr(struct proc_addr *proc)
 {
 	/* only do this once */
 	if (!proc->initialized) {
@@ -47,7 +49,8 @@ static inline FARPROC get_proc_addr(struct proc_addr *proc)
 		hnd = LoadLibraryExA(proc->dll, NULL,
 				     LOAD_LIBRARY_SEARCH_SYSTEM32);
 		if (hnd)
-			proc->pfunction = GetProcAddress(hnd, proc->function);
+			proc->pfunction = (FARVOIDPROD)GetProcAddress(hnd,
+							proc->function);
 	}
 	/* set ENOSYS if DLL or function was not found */
 	if (!proc->pfunction)
diff --git a/config.mak.dev b/config.mak.dev
index c080ac0231..cdf043c52b 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -12,7 +12,6 @@ DEVELOPER_CFLAGS += -pedantic
 DEVELOPER_CFLAGS += -Wpedantic
 ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
-DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types
 endif
 endif
 DEVELOPER_CFLAGS += -Wdeclaration-after-statement
-- 
2.33.0.911.gbe391d4e11

Re: [PATCH] lazyload.h: use an even more generic function pointer than FARPROC

[Johannes Sixt]

Am 23.09.21 um 08:03 schrieb Carlo Marcelo Arenas Belón:
> 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.

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

> 
> 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(-)
> 
> diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
> index dc35cf080b..26c80f7833 100644
> --- a/compat/win32/lazyload.h
> +++ b/compat/win32/lazyload.h
> @@ -15,10 +15,12 @@
>   *                        source, target);
>   */
>  
> +typedef void (*FARVOIDPROC)(void);
> +
>  struct proc_addr {
>  	const char *const dll;
>  	const char *const function;
> -	FARPROC pfunction;
> +	FARVOIDPROC pfunction;
>  	unsigned initialized : 1;
>  };
>  
> @@ -38,7 +40,7 @@ struct proc_addr {
>  #define INIT_PROC_ADDR(function) \
>  	(function = (proc_type_##function)get_proc_addr(&proc_addr_##function))
>  
> -static inline FARPROC get_proc_addr(struct proc_addr *proc)
> +static inline FARVOIDPROC get_proc_addr(struct proc_addr *proc)
>  {
>  	/* only do this once */
>  	if (!proc->initialized) {
> @@ -47,7 +49,8 @@ static inline FARPROC get_proc_addr(struct proc_addr *proc)
>  		hnd = LoadLibraryExA(proc->dll, NULL,
>  				     LOAD_LIBRARY_SEARCH_SYSTEM32);
>  		if (hnd)
> -			proc->pfunction = GetProcAddress(hnd, proc->function);
> +			proc->pfunction = (FARVOIDPROD)GetProcAddress(hnd,
> +							proc->function);
>  	}
>  	/* set ENOSYS if DLL or function was not found */
>  	if (!proc->pfunction)
> diff --git a/config.mak.dev b/config.mak.dev
> index c080ac0231..cdf043c52b 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -12,7 +12,6 @@ DEVELOPER_CFLAGS += -pedantic
>  DEVELOPER_CFLAGS += -Wpedantic
>  ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
>  DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
> -DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types
>  endif
>  endif
>  DEVELOPER_CFLAGS += -Wdeclaration-after-statement
> 

Re: [PATCH] lazyload.h: use an even more generic function pointer than FARPROC

[Carlo Arenas]

On Wed, Sep 22, 2021 at 11:33 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> IIUC, this patch goes on top of mine in origin/js/win-lazyload-buildfix,
> right?

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

Carlo

[PATCH v2] lazyload.h: use an even more generic function pointer than FARPROC

[Carlo Marcelo Arenas Belón]

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

diff --git a/compat/mingw.c b/compat/mingw.c
index 9e0cd1e097..d96fc39bf7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2183,10 +2183,10 @@ 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;
diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
index dc35cf080b..c688e545ad 100644
--- a/compat/win32/lazyload.h
+++ b/compat/win32/lazyload.h
@@ -15,10 +15,12 @@
  *                        source, target);
  */
 
+typedef void (*FARVOIDPROC)(void);
+
 struct proc_addr {
 	const char *const dll;
 	const char *const function;
-	FARPROC pfunction;
+	FARVOIDPROC pfunction;
 	unsigned initialized : 1;
 };
 
@@ -38,7 +40,7 @@ struct proc_addr {
 #define INIT_PROC_ADDR(function) \
 	(function = (proc_type_##function)get_proc_addr(&proc_addr_##function))
 
-static inline FARPROC get_proc_addr(struct proc_addr *proc)
+static inline FARVOIDPROC get_proc_addr(struct proc_addr *proc)
 {
 	/* only do this once */
 	if (!proc->initialized) {
@@ -47,7 +49,8 @@ static inline FARPROC get_proc_addr(struct proc_addr *proc)
 		hnd = LoadLibraryExA(proc->dll, NULL,
 				     LOAD_LIBRARY_SEARCH_SYSTEM32);
 		if (hnd)
-			proc->pfunction = GetProcAddress(hnd, proc->function);
+			proc->pfunction = (FARVOIDPROC)GetProcAddress(hnd,
+							proc->function);
 	}
 	/* set ENOSYS if DLL or function was not found */
 	if (!proc->pfunction)
diff --git a/config.mak.dev b/config.mak.dev
index c080ac0231..cdf043c52b 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -12,7 +12,6 @@ DEVELOPER_CFLAGS += -pedantic
 DEVELOPER_CFLAGS += -Wpedantic
 ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
-DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types
 endif
 endif
 DEVELOPER_CFLAGS += -Wdeclaration-after-statement
diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c
index 7b4278462b..9dccda95b6 100644
--- a/t/helper/test-drop-caches.c
+++ b/t/helper/test-drop-caches.c
@@ -86,9 +86,9 @@ 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");
-- 
2.33.0.911.gbe391d4e11

[PATCH v3 0/2] js/win-lazyload-buildfix

[Carlo Marcelo Arenas Belón]

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");
-- 
2.33.0.955.gee03ddbf0e

[PATCH v3 1/2] lazyload.h: fix warnings about mismatching function pointer types

[Carlo Marcelo Arenas Belón]

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

diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
index d2056cdadf..121ee24ed2 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)
 {
-- 
2.33.0.955.gee03ddbf0e

Re: [PATCH v3 1/2] lazyload.h: fix warnings about mismatching function pointer types

[Eric Sunshine]

On Sun, Sep 26, 2021 at 6:05 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> Here, GCC warns about every use of the INIT_PROC_ADDR macro, for example:
>
> In file included from compat/mingw.c:8:
> [...]
> (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.

s/wrapper/wrapped/

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

[PATCH v3 2/2] lazyload.h: use an even more generic function pointer than FARPROC

[Carlo Marcelo Arenas Belón]

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

diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
index 121ee24ed2..2b3637135f 100644
--- a/compat/win32/lazyload.h
+++ b/compat/win32/lazyload.h
@@ -15,10 +15,12 @@
  *                        source, target);
  */
 
+typedef void (*FARVOIDPROC)(void);
+
 struct proc_addr {
 	const char *const dll;
 	const char *const function;
-	FARPROC pfunction;
+	FARVOIDPROC pfunction;
 	unsigned initialized : 1;
 };
 
@@ -38,7 +40,7 @@ struct proc_addr {
 #define INIT_PROC_ADDR(function) \
 	(function = (proc_type_##function)get_proc_addr(&proc_addr_##function))
 
-static inline FARPROC get_proc_addr(struct proc_addr *proc)
+static inline FARVOIDPROC get_proc_addr(struct proc_addr *proc)
 {
 	/* only do this once */
 	if (!proc->initialized) {
@@ -47,7 +49,8 @@ static inline FARPROC get_proc_addr(struct proc_addr *proc)
 		hnd = LoadLibraryExA(proc->dll, NULL,
 				     LOAD_LIBRARY_SEARCH_SYSTEM32);
 		if (hnd)
-			proc->pfunction = GetProcAddress(hnd, proc->function);
+			proc->pfunction = (FARVOIDPROC)GetProcAddress(hnd,
+							proc->function);
 	}
 	/* set ENOSYS if DLL or function was not found */
 	if (!proc->pfunction)
diff --git a/config.mak.dev b/config.mak.dev
index c080ac0231..cdf043c52b 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -12,7 +12,6 @@ DEVELOPER_CFLAGS += -pedantic
 DEVELOPER_CFLAGS += -Wpedantic
 ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
-DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types
 endif
 endif
 DEVELOPER_CFLAGS += -Wdeclaration-after-statement
-- 
2.33.0.955.gee03ddbf0e

Re: [PATCH v3 2/2] lazyload.h: use an even more generic function pointer than FARPROC

[Junio C Hamano]

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

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

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

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

Thanks.

Re: [PATCH v3 2/2] lazyload.h: use an even more generic function pointer than FARPROC

[Carlo Arenas]

On Mon, Sep 27, 2021 at 9:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
>
> > 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.
>
> Is it clear to everybody what this paragraph is referring to?  It is
> not, at least to me.

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/

Re: [PATCH v3 2/2] lazyload.h: use an even more generic function pointer than FARPROC

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Mon, Sep 27, 2021 at 9:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
>>
>> > 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.
>>
>> Is it clear to everybody what this paragraph is referring to?  It is
>> not, at least to me.
>
> It is not, and it is no longer needed after the fixup was applied to
> the previous
> patch.

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.

[PATCH v4 0/3] js/win-lazyload-buildfix

[Carlo Marcelo Arenas Belón]

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
-- 
2.33.0.955.gee03ddbf0e

[PATCH v4 1/3] lazyload.h: fix warnings about mismatching function pointer types

[Carlo Marcelo Arenas Belón]

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

diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
index d2056cdadf..121ee24ed2 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)
 {
-- 
2.33.0.955.gee03ddbf0e

[PATCH v4 2/3] lazyload.h: use an even more generic function pointer than FARPROC

[Carlo Marcelo Arenas Belón]

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

diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
index 121ee24ed2..2b3637135f 100644
--- a/compat/win32/lazyload.h
+++ b/compat/win32/lazyload.h
@@ -15,10 +15,12 @@
  *                        source, target);
  */
 
+typedef void (*FARVOIDPROC)(void);
+
 struct proc_addr {
 	const char *const dll;
 	const char *const function;
-	FARPROC pfunction;
+	FARVOIDPROC pfunction;
 	unsigned initialized : 1;
 };
 
@@ -38,7 +40,7 @@ struct proc_addr {
 #define INIT_PROC_ADDR(function) \
 	(function = (proc_type_##function)get_proc_addr(&proc_addr_##function))
 
-static inline FARPROC get_proc_addr(struct proc_addr *proc)
+static inline FARVOIDPROC get_proc_addr(struct proc_addr *proc)
 {
 	/* only do this once */
 	if (!proc->initialized) {
@@ -47,7 +49,8 @@ static inline FARPROC get_proc_addr(struct proc_addr *proc)
 		hnd = LoadLibraryExA(proc->dll, NULL,
 				     LOAD_LIBRARY_SEARCH_SYSTEM32);
 		if (hnd)
-			proc->pfunction = GetProcAddress(hnd, proc->function);
+			proc->pfunction = (FARVOIDPROC)GetProcAddress(hnd,
+							proc->function);
 	}
 	/* set ENOSYS if DLL or function was not found */
 	if (!proc->pfunction)
diff --git a/config.mak.dev b/config.mak.dev
index c080ac0231..cdf043c52b 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -12,7 +12,6 @@ DEVELOPER_CFLAGS += -pedantic
 DEVELOPER_CFLAGS += -Wpedantic
 ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
-DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types
 endif
 endif
 DEVELOPER_CFLAGS += -Wdeclaration-after-statement
-- 
2.33.0.955.gee03ddbf0e

[PATCH v4 3/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

[Carlo Marcelo Arenas Belón]

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

diff --git a/config.mak.dev b/config.mak.dev
index cdf043c52b..7673fed114 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -6,14 +6,19 @@ ifeq ($(filter no-error,$(DEVOPTS)),)
 DEVELOPER_CFLAGS += -Werror
 SPARSE_FLAGS += -Wsparse-error
 endif
+
 DEVELOPER_CFLAGS += -Wall
 ifeq ($(filter no-pedantic,$(DEVOPTS)),)
 DEVELOPER_CFLAGS += -pedantic
+ifneq (($or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
 DEVELOPER_CFLAGS += -Wpedantic
-ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
+ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)
+ifeq ($(uname_S),MINGW)
 DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
 endif
 endif
+endif
+endif
 DEVELOPER_CFLAGS += -Wdeclaration-after-statement
 DEVELOPER_CFLAGS += -Wformat-security
 DEVELOPER_CFLAGS += -Wold-style-definition
diff --git a/config.mak.uname b/config.mak.uname
index 76516aaa9a..124ddfce36 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -11,6 +11,10 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
 uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
 uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
 
+ifneq ($(findstring MINGW,$(uname_S)),)
+	uname_S := MINGW
+endif
+
 ifdef MSVC
 	# avoid the MingW and Cygwin configuration sections
 	uname_S := Windows
@@ -588,7 +592,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
 	SHELL_PATH = /usr/coreutils/bin/bash
 endif
-ifneq (,$(findstring MINGW,$(uname_S)))
+ifeq ($(uname_S),MINGW)
 	pathsep = ;
 	HAVE_ALLOCA_H = YesPlease
 	NO_PREAD = YesPlease
@@ -705,6 +709,8 @@ ifeq ($(uname_S),QNX)
 	NO_STRLCPY = YesPlease
 endif
 
+export uname_S
+
 vcxproj:
 	# Require clean work tree
 	git update-index -q --refresh && \
-- 
2.33.0.955.gee03ddbf0e

Re: [PATCH v4 3/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

[Ramsay Jones]

On 29/09/2021 01:48, Carlo Marcelo Arenas Belón wrote:
> 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(-)
> 
> diff --git a/config.mak.dev b/config.mak.dev
> index cdf043c52b..7673fed114 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -6,14 +6,19 @@ ifeq ($(filter no-error,$(DEVOPTS)),)
>  DEVELOPER_CFLAGS += -Werror
>  SPARSE_FLAGS += -Wsparse-error
>  endif
> +
>  DEVELOPER_CFLAGS += -Wall
>  ifeq ($(filter no-pedantic,$(DEVOPTS)),)
>  DEVELOPER_CFLAGS += -pedantic
> +ifneq (($or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
>  DEVELOPER_CFLAGS += -Wpedantic
> -ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
> +ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)
> +ifeq ($(uname_S),MINGW)
>  DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
>  endif
>  endif
> +endif
> +endif
>  DEVELOPER_CFLAGS += -Wdeclaration-after-statement
>  DEVELOPER_CFLAGS += -Wformat-security
>  DEVELOPER_CFLAGS += -Wold-style-definition
> diff --git a/config.mak.uname b/config.mak.uname
> index 76516aaa9a..124ddfce36 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -11,6 +11,10 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
>  uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
>  uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
>  
> +ifneq ($(findstring MINGW,$(uname_S)),)
> +	uname_S := MINGW
> +endif
> +
>  ifdef MSVC
>  	# avoid the MingW and Cygwin configuration sections
>  	uname_S := Windows
> @@ -588,7 +592,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>  	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
>  	SHELL_PATH = /usr/coreutils/bin/bash
>  endif
> -ifneq (,$(findstring MINGW,$(uname_S)))
> +ifeq ($(uname_S),MINGW)
>  	pathsep = ;
>  	HAVE_ALLOCA_H = YesPlease
>  	NO_PREAD = YesPlease
> @@ -705,6 +709,8 @@ ifeq ($(uname_S),QNX)
>  	NO_STRLCPY = YesPlease
>  endif
>  
> +export uname_S
> +

This export seems to be unnecessary.

ATB,
Ramsay Jones

>  vcxproj:
>  	# Require clean work tree
>  	git update-index -q --refresh && \
> 

[PATCH v5 0/3] js/win-lazyload-buildfix

[Carlo Marcelo Arenas Belón]

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 && \
-- 
2.33.0.955.gee03ddbf0e

[PATCH v5 1/3] lazyload.h: fix warnings about mismatching function pointer types

[Carlo Marcelo Arenas Belón]

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

diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
index d2056cdadf..121ee24ed2 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)
 {
-- 
2.33.0.955.gee03ddbf0e

[PATCH v5 2/3] lazyload.h: use an even more generic function pointer than FARPROC

[Carlo Marcelo Arenas Belón]

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

diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
index 121ee24ed2..2b3637135f 100644
--- a/compat/win32/lazyload.h
+++ b/compat/win32/lazyload.h
@@ -15,10 +15,12 @@
  *                        source, target);
  */
 
+typedef void (*FARVOIDPROC)(void);
+
 struct proc_addr {
 	const char *const dll;
 	const char *const function;
-	FARPROC pfunction;
+	FARVOIDPROC pfunction;
 	unsigned initialized : 1;
 };
 
@@ -38,7 +40,7 @@ struct proc_addr {
 #define INIT_PROC_ADDR(function) \
 	(function = (proc_type_##function)get_proc_addr(&proc_addr_##function))
 
-static inline FARPROC get_proc_addr(struct proc_addr *proc)
+static inline FARVOIDPROC get_proc_addr(struct proc_addr *proc)
 {
 	/* only do this once */
 	if (!proc->initialized) {
@@ -47,7 +49,8 @@ static inline FARPROC get_proc_addr(struct proc_addr *proc)
 		hnd = LoadLibraryExA(proc->dll, NULL,
 				     LOAD_LIBRARY_SEARCH_SYSTEM32);
 		if (hnd)
-			proc->pfunction = GetProcAddress(hnd, proc->function);
+			proc->pfunction = (FARVOIDPROC)GetProcAddress(hnd,
+							proc->function);
 	}
 	/* set ENOSYS if DLL or function was not found */
 	if (!proc->pfunction)
diff --git a/config.mak.dev b/config.mak.dev
index c080ac0231..cdf043c52b 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -12,7 +12,6 @@ DEVELOPER_CFLAGS += -pedantic
 DEVELOPER_CFLAGS += -Wpedantic
 ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
-DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types
 endif
 endif
 DEVELOPER_CFLAGS += -Wdeclaration-after-statement
-- 
2.33.0.955.gee03ddbf0e

[PATCH v5 3/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

[Carlo Marcelo Arenas Belón]

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

diff --git a/config.mak.dev b/config.mak.dev
index cdf043c52b..7673fed114 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -6,14 +6,19 @@ ifeq ($(filter no-error,$(DEVOPTS)),)
 DEVELOPER_CFLAGS += -Werror
 SPARSE_FLAGS += -Wsparse-error
 endif
+
 DEVELOPER_CFLAGS += -Wall
 ifeq ($(filter no-pedantic,$(DEVOPTS)),)
 DEVELOPER_CFLAGS += -pedantic
+ifneq (($or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
 DEVELOPER_CFLAGS += -Wpedantic
-ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)
+ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)
+ifeq ($(uname_S),MINGW)
 DEVELOPER_CFLAGS += -Wno-pedantic-ms-format
 endif
 endif
+endif
+endif
 DEVELOPER_CFLAGS += -Wdeclaration-after-statement
 DEVELOPER_CFLAGS += -Wformat-security
 DEVELOPER_CFLAGS += -Wold-style-definition
diff --git a/config.mak.uname b/config.mak.uname
index 76516aaa9a..2b178bad58 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -11,6 +11,10 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
 uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
 uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
 
+ifneq ($(findstring MINGW,$(uname_S)),)
+	uname_S := MINGW
+endif
+
 ifdef MSVC
 	# avoid the MingW and Cygwin configuration sections
 	uname_S := Windows
@@ -588,7 +592,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
 	SHELL_PATH = /usr/coreutils/bin/bash
 endif
-ifneq (,$(findstring MINGW,$(uname_S)))
+ifeq ($(uname_S),MINGW)
 	pathsep = ;
 	HAVE_ALLOCA_H = YesPlease
 	NO_PREAD = YesPlease
-- 
2.33.0.955.gee03ddbf0e

Re: [PATCH cb/pedantic-build-for-developers] lazyload.h: fix warnings about mismatching function pointer types

[Carlo Arenas]

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