diff mbox series

[stage1] Remove conditionals around free()

Message ID 20230301222856.12300c64@nbbrfq
State New
Headers show
Series [stage1] Remove conditionals around free() | expand

Commit Message

Bernhard Reutner-Fischer March 1, 2023, 9:28 p.m. UTC
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

Bernhard Reutner-Fischer March 1, 2023, 9:58 p.m. UTC | #1
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,
Bernhard Reutner-Fischer March 1, 2023, 10:54 p.m. UTC | #2
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,
Andrew Pinski March 1, 2023, 10:59 p.m. UTC | #3
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,
Bernhard Reutner-Fischer March 1, 2023, 11:52 p.m. UTC | #4
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,
Li, Pan2 via Gcc-patches March 2, 2023, 12:07 a.m. UTC | #5
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.
Andrew Pinski March 2, 2023, 12:39 a.m. UTC | #6
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,
>
Jerry D March 2, 2023, 1:23 a.m. UTC | #7
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
Ian Lance Taylor March 2, 2023, 3:23 a.m. UTC | #8
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
Bernhard Reutner-Fischer March 3, 2023, 11:11 p.m. UTC | #9
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
Iain Sandoe March 3, 2023, 11:32 p.m. UTC | #10
> 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
Jerry D March 4, 2023, 3:14 a.m. UTC | #11
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
>
Janne Blomqvist March 4, 2023, 10:15 a.m. UTC | #12
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.
Thomas Schwinge March 8, 2023, 9:56 p.m. UTC | #13
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
Eric Gallager March 22, 2023, 7:21 a.m. UTC | #14
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
>
NightStrike March 24, 2023, 6:30 a.m. UTC | #15
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
Bernhard Reutner-Fischer May 8, 2023, 6:01 a.m. UTC | #16
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,
diff mbox series

Patch

# 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/libbacktrace/zstdtest.c
+++ b/libbacktrace/zstdtest.c
@@ -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 */
 
diff --git a/libbacktrace/ztest.c b/libbacktrace/ztest.c
index 63f4f58885b..dd518f30426 100644
--- a/libbacktrace/ztest.c
+++ b/libbacktrace/ztest.c
@@ -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 */
 
diff --git a/libgfortran/caf/single.c b/libgfortran/caf/single.c
index bb06bd36db5..fea8b0cc204 100644
--- a/libgfortran/caf/single.c
+++ b/libgfortran/caf/single.c
@@ -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;
     }
diff --git a/libgfortran/io/async.c b/libgfortran/io/async.c
index 81d1d8175aa..01adf8f3f78 100644
--- a/libgfortran/io/async.c
+++ b/libgfortran/io/async.c
@@ -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)
 	    {
diff --git a/libgfortran/io/format.c b/libgfortran/io/format.c
index 9e06902ddb7..66acbf04d08 100644
--- a/libgfortran/io/format.c
+++ b/libgfortran/io/format.c
@@ -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);
 	}
 
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 8bb5d1101ca..19b0b9a9324 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -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;
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index ba12be08252..ef35b85570f 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -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;
 }
 
diff --git a/libgo/runtime/go-setenv.c b/libgo/runtime/go-setenv.c
index 08987def8af..d3eaf6b4251 100644
--- a/libgo/runtime/go-setenv.c
+++ b/libgo/runtime/go-setenv.c
@@ -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);
 }
diff --git a/libgo/runtime/go-unsetenv.c b/libgo/runtime/go-unsetenv.c
index 4b5058a220f..a7cfb364ace 100644
--- a/libgo/runtime/go-unsetenv.c
+++ b/libgo/runtime/go-unsetenv.c
@@ -42,6 +42,5 @@  unsetenv_c (String k)
 
 #endif /* !defined(HAVE_UNSETENV) */
 
-  if (kn != NULL)
-    free (kn);
+  free (kn);
 }
diff --git a/libgomp/target.c b/libgomp/target.c
index 483851c95ac..e643c050bda 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -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;
     }
diff --git a/libiberty/concat.c b/libiberty/concat.c
index 4cb1df3baf3..c22d280fafc 100644
--- a/libiberty/concat.c
+++ b/libiberty/concat.c
@@ -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;
diff --git a/zlib/contrib/minizip/unzip.c b/zlib/contrib/minizip/unzip.c
index bcfb9416ec3..16490c6a9e4 100644
--- a/zlib/contrib/minizip/unzip.c
+++ b/zlib/contrib/minizip/unzip.c
@@ -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)
diff --git a/zlib/contrib/minizip/zip.c b/zlib/contrib/minizip/zip.c
index 44e88a9cb98..7182014193d 100644
--- a/zlib/contrib/minizip/zip.c
+++ b/zlib/contrib/minizip/zip.c
@@ -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
 
 /*
diff --git a/zlib/examples/enough.c b/zlib/examples/enough.c
index b9911443052..3de58c873cd 100644
--- a/zlib/examples/enough.c
+++ b/zlib/examples/enough.c
@@ -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
diff --git a/zlib/examples/gun.c b/zlib/examples/gun.c
index be44fa51ff5..cc3fbb40c94 100644
--- a/zlib/examples/gun.c
+++ b/zlib/examples/gun.c
@@ -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
diff --git a/zlib/examples/gzjoin.c b/zlib/examples/gzjoin.c
index 89e8098441b..dbf08e4382b 100644
--- a/zlib/examples/gzjoin.c
+++ b/zlib/examples/gzjoin.c
@@ -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);
     }
 }
diff --git a/zlib/examples/gzlog.c b/zlib/examples/gzlog.c
index b8c29274e8b..67f0aa98539 100644
--- a/zlib/examples/gzlog.c
+++ b/zlib/examples/gzlog.c
@@ -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;