Re: [PATCH][stage1] Remove conditionals around free()
On Wed, 1 Mar 2023 16:07:14 -0800 Steve Kargl wrote: > On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via > Fortran wrote: > > libgfortran/caf/single.c |6 ++ > > libgfortran/io/async.c |6 ++ > > libgfortran/io/format.c |3 +-- > > libgfortran/io/transfer.c|6 ++ > > libgfortran/io/unix.c|3 +-- > > The Fortran ones are OK. > I've pushed the fortran and libgfortran bits as r14-568-gca2f64d5d08c16 thanks,
Re: [PATCH][stage1] Remove conditionals around free()
On Fri, Mar 3, 2023 at 10:14 PM Jerry D via Fortran wrote: > I am certainly not a C++ expert but it seems to me this all begs for > automatic finalization where one would not have to invoke free at all. > I suspect the gfortran frontend is not designed for such things. +1 for RAII
Re: [PATCH][stage1] Remove conditionals around free()
On 3/4/23, Janne Blomqvist via Gcc-patches wrote: > On Wed, Mar 1, 2023 at 11:31 PM Bernhard Reutner-Fischer via Fortran > wrote: >> >> Hi! >> >> Mere cosmetics. >> >> - if (foo != NULL) >> free (foo); >> >> With the caveat that coccinelle ruins replacement whitespace or i'm >> uneducated enough to be unable to _not_ run the diff through >> sed -e 's/^+\([[:space:]]*\)free(/+\1free (/' >> at least. If anybody knows how to improve replacement whitespace, >> i'd be interrested but didn't look nor ask. ISTM that leading >> whitespace is somewhat ruined, too, so beware (8 spaces versus tab as >> far as i have spot-checked). >> >> Would touch >> gcc/ada/rtinit.c |3 +-- >> intl/bindtextdom.c |3 +-- >> intl/loadmsgcat.c|6 ++ >> intl/localcharset.c |3 +-- >> libbacktrace/xztest.c|9 +++-- >> libbacktrace/zstdtest.c |9 +++-- >> libbacktrace/ztest.c |9 +++-- >> libgfortran/caf/single.c |6 ++ >> libgfortran/io/async.c |6 ++ >> libgfortran/io/format.c |3 +-- >> libgfortran/io/transfer.c|6 ++ >> libgfortran/io/unix.c|3 +-- >> libgo/runtime/go-setenv.c|6 ++ >> libgo/runtime/go-unsetenv.c |3 +-- >> libgomp/target.c |3 +-- >> libiberty/concat.c |3 +-- >> zlib/contrib/minizip/unzip.c |2 +- >> zlib/contrib/minizip/zip.c |2 +- >> zlib/examples/enough.c |6 ++ >> zlib/examples/gun.c |2 +- >> zlib/examples/gzjoin.c |3 +-- >> zlib/examples/gzlog.c|6 ++ >> >> coccinelle script and invocation inline. >> Would need to be split for the respective maintainers and run through >> mklog with subject changelog and should of course be compiled and >> tested before that. >> >> Remarks: >> 1) We should do this in if-conversion (?) on our own. >>I suppose. Independently of -fdelete-null-pointer-checks >> 2) Maybe not silently, but raise language awareness nowadays. >>By now it's been a long time since this was first mandated. >> 3) fallout from looking at something completely different >> 4) i most likely will not remember to split it apart and send proper >>patches, tested patches, in stage 1 to maintainers proper, so if >>anyone feels like pursuing this, be my guest. I thought i'd just >>mention it. >> >> cheers, > > Back in 2011 Jim Meyering applied a patch doing this, see > https://gcc.gnu.org/legacy-ml/gcc-patches/2011-03/msg00403.html , and > the gcc/README.Portability snippet added then which is still there. > > Per analysis done then, it seems SunOS 4 was the last system where > free() of a NULL pointer didn't behave per the spec. > > Also in Jim's patch intl/ and zlib/ directories were not touched as > those are imported from other upstreams. > Speaking of which, this reminded me that I opened bug 80528 to catch code like this as a compiler warning: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80528 > -- > Janne Blomqvist >
Re: [PATCH][stage1] Remove conditionals around free()
Hi Bernhard!
On 2023-03-01T22:28:56+0100, Bernhard Reutner-Fischer via Gcc-patches
wrote:
> // POSIX: free(NULL) is perfectly valid
> // quote: If ptr is a null pointer, no action shall occur.
> @ rule1 @
> expression e;
> @@
>
> - if (e != NULL)
> - { free(e); }
> + free (e);
Nice, Coccinelle/spatch! (Another very interesting tool that I so far
had no chance to actually use.)
> # find ./ \( -name "*.[ch]" -o -name "*.cpp" \) -a \( ! -path
> "./gcc/testsuite/*" -a ! -path "./gcc/contrib/*" \) -exec spatch --sp-file
> ~/coccinelle/free-without-if-null.0.cocci --in-place
Also include '*.cc' if you'd like to find some more in 'gcc/' (and
possibly elsewhere, too) than just the following lonely one. ;-)
> --- a/gcc/ada/rtinit.c
> +++ b/gcc/ada/rtinit.c
> @@ -481,8 +481,7 @@ __gnat_runtime_initialize (int install_handler)
>
>FindClose (hDir);
>
> - if (dir != NULL)
> -free (dir);
> + free (dir);
Grüße
Thomas
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
Re: [PATCH][stage1] Remove conditionals around free()
On Wed, Mar 1, 2023 at 11:31 PM Bernhard Reutner-Fischer via Fortran wrote: > > Hi! > > Mere cosmetics. > > - if (foo != NULL) > free (foo); > > With the caveat that coccinelle ruins replacement whitespace or i'm > uneducated enough to be unable to _not_ run the diff through > sed -e 's/^+\([[:space:]]*\)free(/+\1free (/' > at least. If anybody knows how to improve replacement whitespace, > i'd be interrested but didn't look nor ask. ISTM that leading > whitespace is somewhat ruined, too, so beware (8 spaces versus tab as > far as i have spot-checked). > > Would touch > gcc/ada/rtinit.c |3 +-- > intl/bindtextdom.c |3 +-- > intl/loadmsgcat.c|6 ++ > intl/localcharset.c |3 +-- > libbacktrace/xztest.c|9 +++-- > libbacktrace/zstdtest.c |9 +++-- > libbacktrace/ztest.c |9 +++-- > libgfortran/caf/single.c |6 ++ > libgfortran/io/async.c |6 ++ > libgfortran/io/format.c |3 +-- > libgfortran/io/transfer.c|6 ++ > libgfortran/io/unix.c|3 +-- > libgo/runtime/go-setenv.c|6 ++ > libgo/runtime/go-unsetenv.c |3 +-- > libgomp/target.c |3 +-- > libiberty/concat.c |3 +-- > zlib/contrib/minizip/unzip.c |2 +- > zlib/contrib/minizip/zip.c |2 +- > zlib/examples/enough.c |6 ++ > zlib/examples/gun.c |2 +- > zlib/examples/gzjoin.c |3 +-- > zlib/examples/gzlog.c|6 ++ > > coccinelle script and invocation inline. > Would need to be split for the respective maintainers and run through > mklog with subject changelog and should of course be compiled and > tested before that. > > Remarks: > 1) We should do this in if-conversion (?) on our own. >I suppose. Independently of -fdelete-null-pointer-checks > 2) Maybe not silently, but raise language awareness nowadays. >By now it's been a long time since this was first mandated. > 3) fallout from looking at something completely different > 4) i most likely will not remember to split it apart and send proper >patches, tested patches, in stage 1 to maintainers proper, so if >anyone feels like pursuing this, be my guest. I thought i'd just >mention it. > > cheers, Back in 2011 Jim Meyering applied a patch doing this, see https://gcc.gnu.org/legacy-ml/gcc-patches/2011-03/msg00403.html , and the gcc/README.Portability snippet added then which is still there. Per analysis done then, it seems SunOS 4 was the last system where free() of a NULL pointer didn't behave per the spec. Also in Jim's patch intl/ and zlib/ directories were not touched as those are imported from other upstreams. -- Janne Blomqvist
Re: [PATCH][stage1] Remove conditionals around free()
On 3/3/23 3:32 PM, Iain Sandoe wrote: On 3 Mar 2023, at 23:11, Bernhard Reutner-Fischer via Fortran wrote: On 2 March 2023 02:23:10 CET, Jerry D wrote: On 3/1/23 4:07 PM, Steve Kargl via Fortran wrote: On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via Fortran wrote: libgfortran/caf/single.c |6 ++ libgfortran/io/async.c |6 ++ libgfortran/io/format.c |3 +-- libgfortran/io/transfer.c|6 ++ libgfortran/io/unix.c|3 +-- The Fortran ones are OK. The only question I have: Is free posix compliant on all platforms? For example ming64 or mac? OSX / macOS are [certified] Posix compliant - but to unix03 (and might be missing features declared as optional at that revision, or features from later Posix versions). In the case of free() man says: "The free() function deallocates the memory allocation pointed to by ptr. If ptr is a NULL pointer, no operation is performed.” Iain It seems sometimes we run into things like this once in a while. I think we have the -liberty to cater even for non compliant systems either way, if you please excuse the pun. That's not an excuse on POSIX systems, imho. I am certainly not a C++ expert but it seems to me this all begs for automatic finalization where one would not have to invoke free at all. I suspect the gfortran frontend is not designed for such things. Otherwise I have no issue at all. It is a lot cleaner. Jerry
Re: [PATCH][stage1] Remove conditionals around free()
> On 3 Mar 2023, at 23:11, Bernhard Reutner-Fischer via Fortran > wrote: > > On 2 March 2023 02:23:10 CET, Jerry D wrote: >> On 3/1/23 4:07 PM, Steve Kargl via Fortran wrote: >>> On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via >>> Fortran wrote: libgfortran/caf/single.c |6 ++ libgfortran/io/async.c |6 ++ libgfortran/io/format.c |3 +-- libgfortran/io/transfer.c|6 ++ libgfortran/io/unix.c|3 +-- >>> >>> The Fortran ones are OK. >>> >> >> The only question I have: Is free posix compliant on all platforms? >> >> For example ming64 or mac? OSX / macOS are [certified] Posix compliant - but to unix03 (and might be missing features declared as optional at that revision, or features from later Posix versions). In the case of free() man says: "The free() function deallocates the memory allocation pointed to by ptr. If ptr is a NULL pointer, no operation is performed.” Iain >> It seems sometimes we run into things like this once in a while. > > I think we have the -liberty to cater even for non compliant systems either > way, if you please excuse the pun. That's not an excuse on POSIX systems, > imho. > >> >> Otherwise I have no issue at all. It is a lot cleaner. >> >> Jerry
Re: [PATCH][stage1] Remove conditionals around free()
On 2 March 2023 02:23:10 CET, Jerry D wrote: >On 3/1/23 4:07 PM, Steve Kargl via Fortran wrote: >> On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via >> Fortran wrote: >>> libgfortran/caf/single.c |6 ++ >>> libgfortran/io/async.c |6 ++ >>> libgfortran/io/format.c |3 +-- >>> libgfortran/io/transfer.c|6 ++ >>> libgfortran/io/unix.c|3 +-- >> >> The Fortran ones are OK. >> > >The only question I have: Is free posix compliant on all platforms? > >For example ming64 or mac? It seems sometimes we run into things like this >once in a while. I think we have the -liberty to cater even for non compliant systems either way, if you please excuse the pun. That's not an excuse on POSIX systems, imho. > >Otherwise I have no issue at all. It is a lot cleaner. > >Jerry
Re: [PATCH][stage1] Remove conditionals around free()
On 3/1/23 4:07 PM, Steve Kargl via Fortran wrote: On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via Fortran wrote: libgfortran/caf/single.c |6 ++ libgfortran/io/async.c |6 ++ libgfortran/io/format.c |3 +-- libgfortran/io/transfer.c|6 ++ libgfortran/io/unix.c|3 +-- The Fortran ones are OK. The only question I have: Is free posix compliant on all platforms? For example ming64 or mac? It seems sometimes we run into things like this once in a while. Otherwise I have no issue at all. It is a lot cleaner. Jerry
Re: [PATCH][stage1] Remove conditionals around free()
On Wed, Mar 1, 2023 at 3:52 PM Bernhard Reutner-Fischer wrote: > > On Wed, 1 Mar 2023 14:59:44 -0800 > Andrew Pinski wrote: > > > On Wed, Mar 1, 2023 at 1:31 PM Bernhard Reutner-Fischer via Fortran > > wrote: > > > > > > Hi! > > > > > > Mere cosmetics. > > > > > > - if (foo != NULL) > > > free (foo); > > > > > > With the caveat that coccinelle ruins replacement whitespace or i'm > > > uneducated enough to be unable to _not_ run the diff through > > > sed -e 's/^+\([[:space:]]*\)free(/+\1free (/' > > > at least. If anybody knows how to improve replacement whitespace, > > > i'd be interrested but didn't look nor ask. ISTM that leading > > > whitespace is somewhat ruined, too, so beware (8 spaces versus tab as > > > far as i have spot-checked). > > > > > > Would touch > > > gcc/ada/rtinit.c |3 +-- > > > > > > It's funny how you apparently did not comment that hunk in the end ;) No, I was just trying to make it look seperate from the intl hunk really. > > > > intl/bindtextdom.c |3 +-- > > > intl/loadmsgcat.c|6 ++ > > > intl/localcharset.c |3 +-- > > > > intl is imported from glibc, though I don't know we have updated it in > > recent years from glibc. > > i don't think we did, overdue, as we (probably) all know. > OTOH i'm thankful that we don't have submodules but a plain, manageable > repo. Of course that comes with a burden, which is nil if ignored > throughout. Doesn't always pay out too well longterm if nobody > (voluntarily) is in due charge. I looked and nobody has filed a bug report about merging from recent glibc sources for intl. Most likely because not many folks use code from intl any more as glibc is main libc that people use for development these days in GCC world. > > > > zlib/contrib/minizip/unzip.c |2 +- > > > zlib/contrib/minizip/zip.c |2 +- > > > zlib/examples/enough.c |6 ++ > > > zlib/examples/gun.c |2 +- > > > zlib/examples/gzjoin.c |3 +-- > > > zlib/examples/gzlog.c|6 ++ > > > > zlib is definitely imported from zlib upstream. > > So it might be good to check if we could import a new version and see > > if it still works instead. updating zlib is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105404 . Thanks, Andrew > > From a meta POV, i wonder where the trailing space in the subject comes > from, looking at e.g.: > https://gcc.gnu.org/pipermail/gcc-patches/2023-March/date.html#613110 > I think and hope that the newer(?) ones by > https://inbox.sourceware.org/gcc-patches/?t=xyz do not exhibit these > invented trailing blanks nobody ever wrote for real, does it. > > Thanks for reminding me of intl and it's outdatedness, although i > certainly don't have ambition to do anything about it for sure. > I didn't care 15 or 20 years ago and nowadays i'd call that attitude a > tradition, at least ATM ;) TBH i initially had only considered gcc/ but > somehow found that unfair. Great idea that inclusion was. > > thanks, > > > > 4) i most likely will not remember to split it apart and send proper > > >patches, tested patches, in stage 1 to maintainers proper, so if > > >anyone feels like pursuing this, be my guest. I thought i'd just > > >mention it. > > > > > > cheers, >
Re: [PATCH][stage1] Remove conditionals around free()
On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via Fortran wrote: > libgfortran/caf/single.c |6 ++ > libgfortran/io/async.c |6 ++ > libgfortran/io/format.c |3 +-- > libgfortran/io/transfer.c|6 ++ > libgfortran/io/unix.c|3 +-- The Fortran ones are OK. -- Steve
Re: [PATCH][stage1] Remove conditionals around free()
On Wed, 1 Mar 2023 14:59:44 -0800 Andrew Pinski wrote: > On Wed, Mar 1, 2023 at 1:31 PM Bernhard Reutner-Fischer via Fortran > wrote: > > > > Hi! > > > > Mere cosmetics. > > > > - if (foo != NULL) > > free (foo); > > > > With the caveat that coccinelle ruins replacement whitespace or i'm > > uneducated enough to be unable to _not_ run the diff through > > sed -e 's/^+\([[:space:]]*\)free(/+\1free (/' > > at least. If anybody knows how to improve replacement whitespace, > > i'd be interrested but didn't look nor ask. ISTM that leading > > whitespace is somewhat ruined, too, so beware (8 spaces versus tab as > > far as i have spot-checked). > > > > Would touch > > gcc/ada/rtinit.c |3 +-- > > It's funny how you apparently did not comment that hunk in the end ;) > > intl/bindtextdom.c |3 +-- > > intl/loadmsgcat.c|6 ++ > > intl/localcharset.c |3 +-- > > intl is imported from glibc, though I don't know we have updated it in > recent years from glibc. i don't think we did, overdue, as we (probably) all know. OTOH i'm thankful that we don't have submodules but a plain, manageable repo. Of course that comes with a burden, which is nil if ignored throughout. Doesn't always pay out too well longterm if nobody (voluntarily) is in due charge. > > zlib/contrib/minizip/unzip.c |2 +- > > zlib/contrib/minizip/zip.c |2 +- > > zlib/examples/enough.c |6 ++ > > zlib/examples/gun.c |2 +- > > zlib/examples/gzjoin.c |3 +-- > > zlib/examples/gzlog.c|6 ++ > > zlib is definitely imported from zlib upstream. > So it might be good to check if we could import a new version and see > if it still works instead. From a meta POV, i wonder where the trailing space in the subject comes from, looking at e.g.: https://gcc.gnu.org/pipermail/gcc-patches/2023-March/date.html#613110 I think and hope that the newer(?) ones by https://inbox.sourceware.org/gcc-patches/?t=xyz do not exhibit these invented trailing blanks nobody ever wrote for real, does it. Thanks for reminding me of intl and it's outdatedness, although i certainly don't have ambition to do anything about it for sure. I didn't care 15 or 20 years ago and nowadays i'd call that attitude a tradition, at least ATM ;) TBH i initially had only considered gcc/ but somehow found that unfair. Great idea that inclusion was. thanks, > > 4) i most likely will not remember to split it apart and send proper > >patches, tested patches, in stage 1 to maintainers proper, so if > >anyone feels like pursuing this, be my guest. I thought i'd just > >mention it. > > > > cheers,
Re: [PATCH][stage1] Remove conditionals around free()
On Wed, Mar 1, 2023 at 1:31 PM Bernhard Reutner-Fischer via Fortran wrote: > > Hi! > > Mere cosmetics. > > - if (foo != NULL) > free (foo); > > With the caveat that coccinelle ruins replacement whitespace or i'm > uneducated enough to be unable to _not_ run the diff through > sed -e 's/^+\([[:space:]]*\)free(/+\1free (/' > at least. If anybody knows how to improve replacement whitespace, > i'd be interrested but didn't look nor ask. ISTM that leading > whitespace is somewhat ruined, too, so beware (8 spaces versus tab as > far as i have spot-checked). > > Would touch > gcc/ada/rtinit.c |3 +-- > intl/bindtextdom.c |3 +-- > intl/loadmsgcat.c|6 ++ > intl/localcharset.c |3 +-- intl is imported from glibc, though I don't know we have updated it in recent years from glibc. > libbacktrace/xztest.c|9 +++-- > libbacktrace/zstdtest.c |9 +++-- > libbacktrace/ztest.c |9 +++-- > libgfortran/caf/single.c |6 ++ > libgfortran/io/async.c |6 ++ > libgfortran/io/format.c |3 +-- > libgfortran/io/transfer.c|6 ++ > libgfortran/io/unix.c|3 +-- > libgo/runtime/go-setenv.c|6 ++ > libgo/runtime/go-unsetenv.c |3 +-- > libgomp/target.c |3 +-- > libiberty/concat.c |3 +-- This code is really old and only has gotten some modernization in recent years (post 8 years ago). > zlib/contrib/minizip/unzip.c |2 +- > zlib/contrib/minizip/zip.c |2 +- > zlib/examples/enough.c |6 ++ > zlib/examples/gun.c |2 +- > zlib/examples/gzjoin.c |3 +-- > zlib/examples/gzlog.c|6 ++ zlib is definitely imported from zlib upstream. So it might be good to check if we could import a new version and see if it still works instead. Thanks, Andrew Pinski > > coccinelle script and invocation inline. > Would need to be split for the respective maintainers and run through > mklog with subject changelog and should of course be compiled and > tested before that. > > Remarks: > 1) We should do this in if-conversion (?) on our own. >I suppose. Independently of -fdelete-null-pointer-checks > 2) Maybe not silently, but raise language awareness nowadays. >By now it's been a long time since this was first mandated. > 3) fallout from looking at something completely different > 4) i most likely will not remember to split it apart and send proper >patches, tested patches, in stage 1 to maintainers proper, so if >anyone feels like pursuing this, be my guest. I thought i'd just >mention it. > > cheers,
Re: [PATCH][stage1] Remove conditionals around free()
On Wed, 1 Mar 2023 22:28:56 +0100 Bernhard Reutner-Fischer wrote: > Remarks: > 1) We should do this in if-conversion (?) on our own. >I suppose. Independently of -fdelete-null-pointer-checks and iff we can prove that ptr was NULL when passed to free(ptr) then we can elide the call, of course. Likewise for realloc(ptr, 0), obviously. [or reallocarray -- yikes -- if nmemb == 0 || size == 0] But that would probably be a ranger call in DCE, i guess. Didn't look. thanks,
