[PATCH][stage1] Remove conditionals around free()
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, # cat ~/coccinelle/free-without-if-null.0.cocci ; echo EOF // 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); EOF # 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 diff --git a/gcc/ada/rtinit.c b/gcc/ada/rtinit.c index f1607b3c8b0..bbf02b1c2ae 100644 --- 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); } } else diff --git a/intl/bindtextdom.c b/intl/bindtextdom.c index 6faac5756a3..c71c728401b 100644 --- a/intl/bindtextdom.c +++ b/intl/bindtextdom.c @@ -204,8 +204,7 @@ set_binding_values (domainname, dirnamep, codesetp) if (__builtin_expect (result != NULL, 1)) { - if (binding->codeset != NULL) - free (binding->codeset); + free (binding->codeset); binding->codeset = result; binding->codeset_cntr++; diff --git a/intl/loadmsgcat.c b/intl/loadmsgcat.c index 536ee12621d..e55d095ec56 100644 --- a/intl/loadmsgcat.c +++ b/intl/loadmsgcat.c @@ -1273,8 +1273,7 @@ _nl_load_domain (domain_file, domainbinding) /* This is an invalid revision. */ invalid: /* This is an invalid .mo file. */ - if (domain->malloced) - free (domain->malloced); + free (domain->malloced); #ifdef HAVE_MMAP if (use_mmap) munmap ((caddr_t) data, size); @@ -1307,8 +1306,7 @@ _nl_unload_domain (domain) _nl_free_domain_conv (domain); - if (domain->malloced) -free (domain->malloced); + free (domain->malloced); # ifdef _POSIX_MAPPED_FILES if (domain->use_mmap) diff --git a/intl/localcharset.c b/intl/localcharset.c index 8ece6e39f69..8d34dcb0918 100644 --- a/intl/localcharset.c +++ b/intl/localcharset.c @@ -199,8 +199,7 @@ get_charset_aliases () } } - if (file_name != NULL) - free (file_name); + free (file_name); #else diff --git a/libbacktrace/xztest.c b/libbacktrace/xztest.c index ed90066470a..6f4a5c73a00 100644 --- a/libbacktrace/xztest.c +++ b/libbacktrace/xztest.c @@ -479,12 +479,9 @@ test_large (struct backtrace_state *state ATTRIBUTE_UNUSED) printf ("FAIL: lzma large\n"); ++failures; - if (orig_buf != NULL) -free (orig_buf); - if (compressed_buf != NULL) -free (compressed_buf); - if (uncompressed_buf != NULL) -free (uncompressed_buf); + free (orig_buf); + free (compressed_buf); + free (uncompressed_buf); #else /* !HAVE_LIBLZMA */ diff --git a/libbacktrace/zstdtest.c b/libbacktrace/zstdtest.c index 1b4158a50eb..8a0b27977b5 100644 --- a/libbackt
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()
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 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 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,
Re: [PATCH][stage1] Remove conditionals around free()
On Wed, 1 Mar 2023 22:58:59 +0100 Bernhard Reutner-Fischer wrote: > 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, And, if under C, we rule out validity of a life ptr as a return value of malloc(0), ---8<--- If size is 0, either: A null pointer shall be returned [CX] [Option Start] and errno may be set to an implementation-defined value, [Option End] or A pointer to the allocated space shall be returned. The application shall ensure that the pointer is not used to access an object. ---8<--- Where CX == "Extension to the ISO C standard" https://pubs.opengroup.org/onlinepubs/9699919799/help/codes.html#CX then one might be baffled that malloc(0) still is underspecified or at least unpleasantly specified WRT realloc(NULL,0) after all these years in the wild. The exact quote from CX is ---8<--- [CX] [Option Start] Extension to the ISO C standard [Option End] The functionality described is an extension to the ISO C standard. Application developers may make use of an extension as it is supported on all POSIX.1-2017-conforming systems. With each function or header from the ISO C standard, a statement to the effect that "any conflict is unintentional" is included. That is intended to refer to a direct conflict. POSIX.1-2017 acts in part as a profile of the ISO C standard, and it may choose to further constrain behaviors allowed to vary by the ISO C standard. Such limitations and other compatible differences are not considered conflicts, even if a CX mark is missing. The markings are for information only. Where additional semantics apply to a function or header, the material is identified by use of the CX margin legend. ---8<--- So, what in the end this all gives is IMHO that natural stuff like valid C or C++ can only borderline elide any of these calls which hinders everyone and helps nobody for real IMHO. Doesn't it. That's how conceptually devirt does it's thing for just a very particular flavour of the family and rightfully leaves the rest alone, which really is a pity IMHO. But that's enough as far as an unfounded rant goes for today and should probably be addressed elsewhere either way. So, please remind me, why, again, does C do it's life ptr thing, short of an optimisation barrier? thanks,
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 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 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, 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 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()
Bernhard Reutner-Fischer writes: > libgo/runtime/go-setenv.c|6 ++ > libgo/runtime/go-unsetenv.c |3 +-- Files in the libgo directory are mirrored from upstream sources, as described in libgo/README.gcc. Please don't change them in the gcc repository. Thanks. Ian
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 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 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 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()
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