| Message ID | 20210903170232.57646-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 27e0c3c6cfead8fccd16105be497dba7ccd0ec6b |
| Headers | show |
| Series | support pedantic in developer mode | expand |
Am 03.09.21 um 19:02 schrieb Carlo Marcelo Arenas Belón: > In preparation to building with pedantic mode enabled, change a couple > of places where the current mingw gcc compiler provided with the SDK > reports issues. > > A full fix for the incompatible use of (void *) to store function > pointers has been punted, with the minimal change to instead use a > generic function pointer (FARPROC), and therefore the (hopefully) > temporary need to disable incompatible pointer warnings. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > This is all that is needed to build cleanly once merged to maint/master/next > > There is at least one fix needed on top for seen, that was sent already > and is expected as part of a different reroll as well of several more for > git-for-windows/main that will be send independently. > > compat/nedmalloc/nedmalloc.c | 2 +- > compat/win32/lazyload.h | 2 +- > config.mak.dev | 13 ++++++++----- > 3 files changed, 10 insertions(+), 7 deletions(-) > > 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. Clang on MacOS doesn't like the original code either and report if USE_NED_ALLOCATOR is enabled it reports: 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. A slightly shorter fix would be to replace "tck" with "mem". Not as obvious without further context, though. René
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). 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? 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
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: >> A slightly shorter fix would be to replace "tck" with "mem". Not as >> obvious without further context, though. > > so something like this on top? > 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(); > } The new comment explains why the original (i.e. unadorned 'tck'), which should work fine, needs to be changed. The reason is because a version of compiler wants an explict (void *) cast to go with the placeholder "%p". Given that, it would be much better to pass (void *)tck instead of mem, no? Especially since the comment does not say tck and mem have the same pointer value. Having said lal that, I have to wonder if how much help the developer who is hunting for allocation bug is getting out of a raw pointer value in this message, though.
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 >
Am 03.09.21 um 22:38 schrieb René Scharfe: > 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. Expected your [1] to stand for a footnote, and got confused when I found none. The last commit in https://github.com/ned14/nedmalloc is from seven years ago and this repository is archived, with the author still being active on GitHub. Seems like nedmalloc reached its end of life. Has there been an official announcement? >> 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 don't have GCC installed, only checked with https://godbolt.org/z/jc356vqb4 René
On Sat, Sep 4, 2021 at 2:37 AM René Scharfe <l.s.r@web.de> wrote: > > Am 03.09.21 um 22:38 schrieb René Scharfe: > > 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. > > Expected your [1] to stand for a footnote, and got confused when I found none. > The last commit in https://github.com/ned14/nedmalloc is from seven years ago > and this repository is archived, with the author still being active on GitHub. > Seems like nedmalloc reached its end of life. Has there been an official > announcement? Apologies; this is the [1] I was referring to: [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.1908082213400.46@tvgsbejva qbjf.bet/ TLDR; nedmalloc works but is only stable in Windows, and indeed shows other warnings in macOS that would have broken a DEVELOPER=1 build as well which I am ignoring. compat/nedmalloc/nedmalloc.c:326:8: warning: address of array 'p->caches' will always evaluate to 'true' [-Wpointer-bool-conversion] if(p->caches) ~~ ~~~^~~~~~ 1 warning generated. Carlo
> +ifneq ($(filter gcc5,$(COMPILER_FEATURES)),) > +DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types > +endif I noticed today that I wasn't warned about some incompatible function pointer signatures (that I expected to be warned about) due to this line - could the condition of adding this compiler flag be further narrowed down? gcc -v says: gcc version 10.3.0 (Debian 10.3.0-9+build2) On my system, if I remove that line, "make DEVELOPER=1" is still successful.
On Mon, Sep 27, 2021 at 4:04 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > > +ifneq ($(filter gcc5,$(COMPILER_FEATURES)),) > > +DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types > > +endif > > I noticed today that I wasn't warned about some incompatible function > pointer signatures (that I expected to be warned about) due to this > line - could the condition of adding this compiler flag be further > narrowed down? gcc -v says: Apologies; it is gone already in "seen" (and hopefully soon in "next") by merging js/win-lazyload-buildfix[1] > gcc version 10.3.0 (Debian 10.3.0-9+build2) > > On my system, if I remove that line, "make DEVELOPER=1" is still > successful. Correct; it was only needed in Windows, will narrow it further. Carlo [1] https://github.com/gitster/git/tree/js/win-lazyload-buildfix
> Apologies; it is gone already in "seen" (and hopefully soon in "next") > by merging js/win-lazyload-buildfix[1] Ah, thanks for having fixed this.
Carlo Arenas <carenas@gmail.com> writes: > On Mon, Sep 27, 2021 at 4:04 PM Jonathan Tan <jonathantanmy@google.com> wrote: >> >> > +ifneq ($(filter gcc5,$(COMPILER_FEATURES)),) >> > +DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types >> > +endif >> >> I noticed today that I wasn't warned about some incompatible function >> pointer signatures (that I expected to be warned about) due to this >> line - could the condition of adding this compiler flag be further >> narrowed down? gcc -v says: > > Apologies; it is gone already in "seen" (and hopefully soon in "next") > by merging js/win-lazyload-buildfix[1] I will mark it not ready for 'next', while waiting for a fix-up. Thanks for stopping me.
> Carlo Arenas <carenas@gmail.com> writes: > > > On Mon, Sep 27, 2021 at 4:04 PM Jonathan Tan <jonathantanmy@google.com> wrote: > >> > >> > +ifneq ($(filter gcc5,$(COMPILER_FEATURES)),) > >> > +DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types > >> > +endif > >> > >> I noticed today that I wasn't warned about some incompatible function > >> pointer signatures (that I expected to be warned about) due to this > >> line - could the condition of adding this compiler flag be further > >> narrowed down? gcc -v says: > > > > Apologies; it is gone already in "seen" (and hopefully soon in "next") > > by merging js/win-lazyload-buildfix[1] > > I will mark it not ready for 'next', while waiting for a fix-up. > Thanks for stopping me. Just checking - which branch is not ready for next? The issue I noticed is already in master, and js/win-lazyload-buildfix contains the fix for the issue (which ideally would be merged as soon as possible, but merging according to the usual schedule is fine).
On Tue, Sep 28, 2021 at 1:16 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > > Carlo Arenas <carenas@gmail.com> writes: > > > > > Apologies; it is gone already in "seen" (and hopefully soon in "next") > > > by merging js/win-lazyload-buildfix[1] > > > > I will mark it not ready for 'next', while waiting for a fix-up. > > Thanks for stopping me. > > Just checking - which branch is not ready for next? The issue I noticed > is already in master, and js/win-lazyload-buildfix contains the fix for > the issue (which ideally would be merged as soon as possible, but > merging according to the usual schedule is fine). My guess was that he meant to have also the windows specific check added as part of this branch, and that would have prevented the issue you reported originally as well. Either way, a v4 has been posted[1] (sorry, forgot to CC you), and hopefully that is now ready for next ;) Carlo [1] https://lore.kernel.org/git/20210929004832.96304-1-carenas@gmail.com/
Carlo Arenas <carenas@gmail.com> writes: > Either way, a v4 has been posted[1] (sorry, forgot to CC you), and > hopefully that is now ready for next ;) Thanks, both.
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); abort(); } #ifdef FULLSANITYCHECKS diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h index 9e631c8593..d2056cdadf 100644 --- a/compat/win32/lazyload.h +++ b/compat/win32/lazyload.h @@ -37,7 +37,7 @@ struct proc_addr { #define INIT_PROC_ADDR(function) \ (function = get_proc_addr(&proc_addr_##function)) -static inline void *get_proc_addr(struct proc_addr *proc) +static inline FARPROC get_proc_addr(struct proc_addr *proc) { /* only do this once */ if (!proc->initialized) { diff --git a/config.mak.dev b/config.mak.dev index 41d6345bc0..5424db5c22 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -1,11 +1,18 @@ +ifndef COMPILER_FEATURES +COMPILER_FEATURES := $(shell ./detect-compiler $(CC)) +endif + ifeq ($(filter no-error,$(DEVOPTS)),) DEVELOPER_CFLAGS += -Werror SPARSE_FLAGS += -Wsparse-error endif +DEVELOPER_CFLAGS += -Wall ifneq ($(filter pedantic,$(DEVOPTS)),) DEVELOPER_CFLAGS += -pedantic +ifneq ($(filter gcc5,$(COMPILER_FEATURES)),) +DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types +endif endif -DEVELOPER_CFLAGS += -Wall DEVELOPER_CFLAGS += -Wdeclaration-after-statement DEVELOPER_CFLAGS += -Wformat-security DEVELOPER_CFLAGS += -Wold-style-definition @@ -16,10 +23,6 @@ DEVELOPER_CFLAGS += -Wunused DEVELOPER_CFLAGS += -Wvla DEVELOPER_CFLAGS += -fno-common -ifndef COMPILER_FEATURES -COMPILER_FEATURES := $(shell ./detect-compiler $(CC)) -endif - ifneq ($(filter clang4,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare endif
In preparation to building with pedantic mode enabled, change a couple of places where the current mingw gcc compiler provided with the SDK reports issues. A full fix for the incompatible use of (void *) to store function pointers has been punted, with the minimal change to instead use a generic function pointer (FARPROC), and therefore the (hopefully) temporary need to disable incompatible pointer warnings. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- This is all that is needed to build cleanly once merged to maint/master/next There is at least one fix needed on top for seen, that was sent already and is expected as part of a different reroll as well of several more for git-for-windows/main that will be send independently. compat/nedmalloc/nedmalloc.c | 2 +- compat/win32/lazyload.h | 2 +- config.mak.dev | 13 ++++++++----- 3 files changed, 10 insertions(+), 7 deletions(-)