Bug 80528 - reimplement gnulib's "useless-if-before-free" script as a compiler warning
Summary: reimplement gnulib's "useless-if-before-free" script as a compiler warning
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 8.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: new-warning, new_warning
  Show dependency treegraph
 
Reported: 2017-04-26 13:07 UTC by Eric Gallager
Modified: 2024-06-27 03:40 UTC (History)
9 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-08-18 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Gallager 2017-04-26 13:07:42 UTC
Split off from Bug 80519

Gnulib has a script called "useless-if-before-free" that detects code like:
if (p) free(p);
and warns about it, since the "if" test is usually unneeded nowadays. It'd be nice if GCC did this warning instead, so users wouldn't have to import a separate script from gnulib just to check for this sort of thing.

Source for script:
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=build-aux/useless-if-before-free;h=4e3f3a2658b105e8949f14ef85d733192c29be2c;hb=HEAD
Comment 1 Jakub Jelinek 2017-04-26 13:46:22 UTC
Well, if the user knows that the pointer is usually NULL, then if (p) free (p); is a significant optimization over free (p).  So as code size optimization, removing it is fine, but otherwise, it isn't at all clear.
Comment 2 Martin Sebor 2017-08-18 15:51:12 UTC
I think a warning like this might be useful, although I probably wouldn't want to enable under -Wall or -Wextra.   (GCC could also use branch probabilities to decide whether or not to warn.)

That said, I would expect GCC to get rid of the tests with -fdelete-null-pointer-checks.  As it is, it eliminates the call to free when the pointer is known to be null but not the test itself (see below).  That seems quite suboptimal to me since in the absence of any other information a pointer being passed to free is far more likely to be non-null than not.  Clang and ICC both optimize the function below into an unconditional jump to free.  So while I think implementing the optimization is the better solution here, the warning could also help improve the efficiency of the emitted code in its absence.

$ cat b.c && gcc -O2 -S -Wall -Wextra -fdump-tree-optimized=/dev/stdout  b.c
void free (void*);

void g (void *p)
{
  if (p)        // test retained
    free (p);
  else
    free (p);   // eliminated
}


;; Function g (g, funcdef_no=0, decl_uid=1817, cgraph_uid=0, symbol_order=0)

Removing basic block 5
g (void * p)
{
  <bb 2> [100.00%] [count: INV]:
  if (p_2(D) != 0B)
    goto <bb 3>; [53.47%] [count: INV]
  else
    goto <bb 4>; [46.53%] [count: INV]

  <bb 3> [53.47%] [count: INV]:
  free (p_2(D)); [tail call]

  <bb 4> [100.00%] [count: INV]:
  return;

}
Comment 3 Eric Gallager 2018-08-27 03:09:29 UTC
cc-ing Jim Meyering, the author of the original gnulib script, to see if he has any input
Comment 4 jim 2018-08-27 08:18:37 UTC
Thanks for considering the addition. IME, the vast majority (probably "all I've seen") of such "useless" if stmts have been attempts to avoid what used to be UB/segfault on old systems, or simply due to people not realizing that free(NULL) is now known to be ok on all reasonable portability targets.
Comment 5 Eric Gallager 2022-05-14 19:13:10 UTC
One thing to consider now that GCC has a static analyzer is that putting the kind of useless "if" in question before a "free" can make the analyzer shut up about possible double-free warnings, so if this were to become its own diagnostic, there might be separate diagnostics that prompt users to do opposite things...
Comment 6 Eric Gallager 2023-03-22 07:26:46 UTC
Recent patchset for fixing problems in GCC based on this same principle: https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613116.html
Comment 7 Eric Gallager 2024-06-26 09:20:33 UTC
I see Collin Funk has been working on improving the gnulib version; cc-ing: 
https://lists.gnu.org/archive/html/bug-gnulib/2024-06/msg00193.html
Comment 8 Collin Funk 2024-06-27 03:40:48 UTC
Thanks for the add. My change was just adding nullptr since it can be used in C code (per C23). I agree with most of the other comments. Probably excessive for -Wall and -Wextra since the idiom exists in old code still. But a warning that is explicitly enabled would be nice.