Re: [PATCH v3 2/3] win32: allow building with pedantic mode enabled

["René Scharfe" <l.s.r@web.de>]

Am 03.09.21 um 22:13 schrieb Carlo Marcelo Arenas Belón:
> On Fri, Sep 03, 2021 at 08:47:02PM +0200, René Scharfe wrote:
>> Am 03.09.21 um 19:02 schrieb Carlo Marcelo Arenas Belón:
>>> diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
>>> index 1cc31c3502..edb438a777 100644
>>> --- a/compat/nedmalloc/nedmalloc.c
>>> +++ b/compat/nedmalloc/nedmalloc.c
>>> @@ -510,7 +510,7 @@ static void threadcache_free(nedpool *p, threadcache *tc, int mymspace, void *me
>>>  	assert(idx<=THREADCACHEMAXBINS);
>>>  	if(tck==*binsptr)
>>>  	{
>>> -		fprintf(stderr, "Attempt to free already freed memory block %p - aborting!\n", tck);
>>> +		fprintf(stderr, "Attempt to free already freed memory block %p - aborting!\n", (void *)tck);
>>
>> This change is not mentioned in the commit message.
>
> got me there, I was intentionally trying to ignore it since nedmalloc gives
> me PTSD and is obsoleted AFAIK[1], so just adding a casting to void (while
> ugly) was also less intrusive.
>
>> compat/nedmalloc/nedmalloc.c:513:82: error: format specifies type 'void *' but the argument has type 'threadcacheblk *' (aka 'struct threadcacheblk_t *') [-Werror,-Wformat-pedantic]
>>                 fprintf(stderr, "Attempt to free already freed memory block %p - aborting!\n", tck);
>>                                                                             ~~                 ^~~
>> This makes no sense to me, though: Any pointer can be converted to a
>> void pointer without a cast in C.  GCC doesn't require void pointers
>> for %p even with -pedantic.
>
> strange, gcc-11 prints the following in MacOS for me:
>
> compat/nedmalloc/nedmalloc.c: In function 'threadcache_free':
> compat/nedmalloc/nedmalloc.c:522:78: warning: format '%p' expects argument of type 'void *', but argument 3 has type 'threadcacheblk *' {aka 'struct threadcacheblk_t *'} [-Wformat=]
>   522 |                 fprintf(stderr, "Attempt to free already freed memory block %p - aborting!\n", tck);
>       |                                                                             ~^                 ~~~
>       |                                                                              |                 |
>       |                                                                              void *            threadcacheblk * {aka struct threadcacheblk_t *}
>
> I think the rationale is that it is better to be safe than sorry, and since
> the parameter is variadic there is no chance for the compiler to do any
> implicit type casting (unless one is provided explicitly).

True, other pointers could be smaller on some machines.

> clang 14 does also trigger a warning, so IMHO this code will be needed
> until nedmalloc is retired.
>
>> A slightly shorter fix would be to replace "tck" with "mem".  Not as
>> obvious without further context, though.
>
> so something like this on top?

Nah, I like your original version better now that I understand the warning..

Though for upstream it would make more sense to report the caller-supplied
pointer value in the error message than a casted one..

>
> Carlo
> ---- > 8 ----
> diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
> index edb438a777..14e8c4df4f 100644
> --- a/compat/nedmalloc/nedmalloc.c
> +++ b/compat/nedmalloc/nedmalloc.c
> @@ -510,7 +510,15 @@ static void threadcache_free(nedpool *p, threadcache *tc, int mymspace, void *me
>  	assert(idx<=THREADCACHEMAXBINS);
>  	if(tck==*binsptr)
>  	{
> -		fprintf(stderr, "Attempt to free already freed memory block %p - aborting!\n", (void *)tck);
> +		/*
> +		 * Original code used tck instead of mem, but that was changed
> +		 * to workaround a pedantic warning from mingw64 gcc 10.3 that
> +		 * requires %p to have a explicit (void *) as a parameter.
> +		 *
> +		 * This might seem to be a compiler bug or limitation that
> +		 * should be changed back if fixed for maintanability.
> +		 */
> +		fprintf(stderr, "Attempt to free already freed memory block %p - aborting!\n", mem);
>  		abort();
>  	}
>  #ifdef FULLSANITYCHECKS
>