[stage1] Remove conditionals around free()
Checks
Commit Message
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,
Comments
On Wed, 1 Mar 2023 22:28:56 +0100
Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> 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,
On Wed, 1 Mar 2023 22:58:59 +0100
Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
> On Wed, 1 Mar 2023 22:28:56 +0100
> Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> 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,
On Wed, Mar 1, 2023 at 1:31 PM Bernhard Reutner-Fischer via Fortran
<fortran@gcc.gnu.org> 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,
On Wed, 1 Mar 2023 14:59:44 -0800
Andrew Pinski <pinskia@gmail.com> wrote:
> On Wed, Mar 1, 2023 at 1:31 PM Bernhard Reutner-Fischer via Fortran
> <fortran@gcc.gnu.org> 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,
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.
On Wed, Mar 1, 2023 at 3:52 PM Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:
>
> On Wed, 1 Mar 2023 14:59:44 -0800
> Andrew Pinski <pinskia@gmail.com> wrote:
>
> > On Wed, Mar 1, 2023 at 1:31 PM Bernhard Reutner-Fischer via Fortran
> > <fortran@gcc.gnu.org> 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,
>
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
Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> 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
On 2 March 2023 02:23:10 CET, Jerry D <jvdelisle2@gmail.com> 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
> On 3 Mar 2023, at 23:11, Bernhard Reutner-Fischer via Fortran <fortran@gcc.gnu.org> wrote:
>
> On 2 March 2023 02:23:10 CET, Jerry D <jvdelisle2@gmail.com> 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
On 3/3/23 3:32 PM, Iain Sandoe wrote:
>
>
>> On 3 Mar 2023, at 23:11, Bernhard Reutner-Fischer via Fortran <fortran@gcc.gnu.org> wrote:
>>
>> On 2 March 2023 02:23:10 CET, Jerry D <jvdelisle2@gmail.com> 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
>
On Wed, Mar 1, 2023 at 11:31 PM Bernhard Reutner-Fischer via Fortran
<fortran@gcc.gnu.org> 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.
Hi Bernhard!
On 2023-03-01T22:28:56+0100, Bernhard Reutner-Fischer via Gcc-patches <gcc-patches@gcc.gnu.org> 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
On 3/4/23, Janne Blomqvist via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> On Wed, Mar 1, 2023 at 11:31 PM Bernhard Reutner-Fischer via Fortran
> <fortran@gcc.gnu.org> 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
>
On Fri, Mar 3, 2023 at 10:14 PM Jerry D via Fortran <fortran@gcc.gnu.org> 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
On Wed, 1 Mar 2023 16:07:14 -0800
Steve Kargl <sgk@troutmask.apl.washington.edu> 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,
# 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
@@ -481,8 +481,7 @@ __gnat_runtime_initialize (int install_handler)
FindClose (hDir);
- if (dir != NULL)
- free (dir);
+ free (dir);
}
}
else
@@ -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++;
@@ -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)
@@ -199,8 +199,7 @@ get_charset_aliases ()
}
}
- if (file_name != NULL)
- free (file_name);
+ free (file_name);
#else
@@ -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 */
@@ -494,12 +494,9 @@ test_large (struct backtrace_state *state ATTRIBUTE_UNUSED)
printf ("FAIL: zstd 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_ZSTD */
@@ -512,12 +512,9 @@ test_large (struct backtrace_state *state ATTRIBUTE_UNUSED)
printf ("FAIL: inflate 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_ZLIB */
@@ -163,10 +163,8 @@ _gfortran_caf_register (size_t size, caf_register_t type, caf_token_t *token,
/* Freeing the memory conditionally seems pointless, but
caf_internal_error () may return, when a stat is given and then the
memory may be lost. */
- if (local)
- free (local);
- if (*token)
- free (*token);
+ free (local);
+ free (*token);
caf_internal_error (alloc_fail_msg, stat, errmsg, errmsg_len);
return;
}
@@ -71,8 +71,7 @@ update_pdt (st_parameter_dt **old, st_parameter_dt *new) {
NOTE ("Changing pdts, current_unit = %p", (void *) (new->u.p.current_unit));
temp = *old;
*old = new;
- if (temp)
- free (temp);
+ free (temp);
}
/* Destroy an adv_cond structure. */
@@ -106,8 +105,7 @@ async_io (void *arg)
/* Loop over the queue entries until they are finished. */
while (ctq)
{
- if (prev)
- free (prev);
+ free (prev);
prev = ctq;
if (!au->error.has_error)
{
@@ -269,8 +269,7 @@ free_format_data (format_data *fmt)
fnp->format != FMT_NONE; fnp++)
if (fnp->format == FMT_DT)
{
- if (GFC_DESCRIPTOR_DATA(fnp->u.udf.vlist))
- free (GFC_DESCRIPTOR_DATA(fnp->u.udf.vlist));
+ free (GFC_DESCRIPTOR_DATA(fnp->u.udf.vlist));
free (fnp->u.udf.vlist);
}
@@ -4522,8 +4522,7 @@ st_read_done_worker (st_parameter_dt *dtp, bool unlock)
{
free (dtp->u.p.current_unit->filename);
dtp->u.p.current_unit->filename = NULL;
- if (dtp->u.p.current_unit->ls)
- free (dtp->u.p.current_unit->ls);
+ free (dtp->u.p.current_unit->ls);
dtp->u.p.current_unit->ls = NULL;
}
free_newunit = true;
@@ -4619,8 +4618,7 @@ st_write_done_worker (st_parameter_dt *dtp, bool unlock)
{
free (dtp->u.p.current_unit->filename);
dtp->u.p.current_unit->filename = NULL;
- if (dtp->u.p.current_unit->ls)
- free (dtp->u.p.current_unit->ls);
+ free (dtp->u.p.current_unit->ls);
dtp->u.p.current_unit->ls = NULL;
}
free_newunit = true;
@@ -1028,8 +1028,7 @@ mem_flush (unix_stream *s __attribute__ ((unused)))
static int
mem_close (unix_stream *s)
{
- if (s)
- free (s);
+ free (s);
return 0;
}
@@ -72,8 +72,6 @@ setenv_c (String k, String v)
#endif /* !defined(HAVE_SETENV) */
- if (kn != NULL)
- free (kn);
- if (vn != NULL)
- free (vn);
+ free (kn);
+ free (vn);
}
@@ -42,6 +42,5 @@ unsetenv_c (String k)
#endif /* !defined(HAVE_UNSETENV) */
- if (kn != NULL)
- free (kn);
+ free (kn);
}
@@ -1860,8 +1860,7 @@ gomp_remove_splay_tree_key (splay_tree sp, splay_tree_key k)
{
if (k->aux->link_key)
splay_tree_insert (sp, (splay_tree_node) k->aux->link_key);
- if (k->aux->attach_count)
- free (k->aux->attach_count);
+ free (k->aux->attach_count);
free (k->aux);
k->aux = NULL;
}
@@ -187,8 +187,7 @@ reconcat (char *optr, const char *first, ...)
/* Now copy the individual pieces to the result string. */
va_start (args, first);
vconcat_copy (newstr, first, args);
- if (optr) /* Done before VA_CLOSE so optr stays in scope for K&R C. */
- free (optr);
+ free (optr);
va_end (args);
return newstr;
@@ -112,7 +112,7 @@
# define ALLOC(size) (malloc(size))
#endif
#ifndef TRYFREE
-# define TRYFREE(p) {if (p) free(p);}
+# define TRYFREE(p) {free(p);}
#endif
#define SIZECENTRALDIRITEM (0x2e)
@@ -62,7 +62,7 @@
# define ALLOC(size) (malloc(size))
#endif
#ifndef TRYFREE
-# define TRYFREE(p) {if (p) free(p);}
+# define TRYFREE(p) {free(p);}
#endif
/*
@@ -189,10 +189,8 @@ local void cleanup(void)
free(done[n].vec);
free(done);
}
- if (num != NULL)
- free(num);
- if (code != NULL)
- free(code);
+ free (num);
+ free (code);
}
/* Return the number of possible Huffman codes using bit patterns of lengths
@@ -690,7 +690,7 @@ int main(int argc, char **argv)
outname[len] = 0;
}
ret = gunzip(&strm, *argv, outname, test);
- if (outname != NULL) free(outname);
+ free (outname);
if (ret) break;
} while (argv++, --argc);
else
@@ -89,8 +89,7 @@ local void bclose(bin *in)
if (in != NULL) {
if (in->fd != -1)
close(in->fd);
- if (in->buf != NULL)
- free(in->buf);
+ free (in->buf);
free(in);
}
}
@@ -787,8 +787,7 @@ local int log_recover(struct log *log, int op)
log_log(log, op, ret ? "failure" : "complete");
/* clean up */
- if (data != NULL)
- free(data);
+ free (data);
return ret;
}
@@ -1051,8 +1050,7 @@ int gzlog_close(gzlog *logd)
log_close(log);
/* free structure and return */
- if (log->path != NULL)
- free(log->path);
+ free (log->path);
strcpy(log->id, "bad");
free(log);
return 0;