Message-ID: <ZkVWCOZDb7iewLxL@gallifrey>
Date: Thu, 16 May 2024 00:40:40 +0000
From: "Dr. David Alan Gilbert" <linux@...blig.org>
To: Kees Cook <keescook@...omium.org>
Cc: linux-kernel@...r.kernel.org, nathan@...nel.org,
	luc.vanoostenryck@...il.com
Subject: Re: thoughts wanted on dead code hunting?

* Kees Cook (keescook@...omium.org) wrote:
> On Thu, May 09, 2024 at 12:08:56PM +0000, Dr. David Alan Gilbert wrote:
> >   That's found me ~200 candidates; where I guess 150ish are probably
> > real; but my hacky script is, well trivial and hacky, so they each
> > need eyeballing, then a git lookup to see why they're unused, and a
> > compile just to make there's not some subtle macro somewhere.
> 
> Nice finds! People are usually big fans of code removal patches. :)

Thanks; removing the LIST_HEADs actually saves bytes in the binary;
just removing the structs themselves still cleans up the source and
occasionally it's noticing something else left along with it.

> >  ** Questions:
> >   a) Can anyone think of a better tool than my script (see bottom)?
> >    The simplicity is a blessing & a curse - it doesn't know about
> >    #ifdef's so I don't need to try lots of configs, but at the same
> >    time, it can't tell if the struct actually gets used in a macro
> >    and I have to eyeball for a struct which is assigned to as
> >    a variable at declaration time.
> 
> I'm not sure I've seen anything better.
> 
> I tend to use stuff like Coccinelle (spatch) for finding specific struct
> usage, but it can sometimes be slow when trying to process headers
> recursively. e.g.:
> 
> // Options: --recursive-includes
> @find@
> struct to_be_removed INSTANCE;
> struct to_be_removed *POINTER;
> 
> (
> *       INSTANCE
> |
> *       POINTER
> )
> 
> 
> (I bet this could be improved, but it should be a usable example.)

Hmm, now if I could use coccinelle it would be more tolerant of coding
style and slight variations than my script.
However, trying that tiny example, I get:
  File "play.cocci", line 10, column 1, charpos = 141
    around = '',
    whole content = )

so it seems to be objecting to something at the end of the file?
I ran that with:

make coccicheck COCCI=play.cocci M=arch/x86
with Fedora 40's coccinelle-1.1.1-30.20230624git0afff7f.fc40.x86_64

> So this might very a given struct isn't used.
> 
> >   b) The dead structs are all over; so they've mostly been individual
> >   patches rather than a big patch series - how do people feel about
> >   another 150ish similar patches ?
> 
> Generally the smaller patches are preferred. For this kind of thing,
> though, I'd probably collect them by individual header files, rather
> than one-patch-per-struct.

Yeh; although note so far I've only been looking for unused structs
that are defined in a .c file rather than ones in headers.
Those are relatively easy to find, because I'm only looking in one .c
at a time (although that does hit corner cases like:

  header:
       struct foo;
       struct baa {
          struct foo *p;
       };

  .c file:
       struct foo {
         stuff
       };

       func(struct baa *b) {
          b->p  something
       }

  so foo is defined in the C file but the symbol 'foo' is never
used again in it.

> If you have one giant patch, this tool can help break it up into
> per-subsystem patches (it isn't perfect, but does its best):
> https://github.com/kees/kernel-tools/blob/trunk/split-on-maintainer

Thanks.

> >   * There's a few cases where people have added 'static' to a variable
> >    to cleanup compiler warnings, but actually they just needed to
> >    delete the variable.
> 
> Hah. Yeah, these are nice to find and remove.
> 
> >   * A harder problem is unused structure members; some I've spotted
> >    by accident, some follow from what else I delete; e.g. if you
> >    delete a LIST_HEAD, there's a good chance there's a struct somewhere
> >    with the list entry in it that's no longer used.
> 
> This is especially tricky because a giant amount of structs in the
> kernel actually describe over-the-wire or on-hardware structures that
> maybe the kernel doesn't care about all the members, but they're still
> needed to keep the layout correct.

Oh yeh; and also I'm not deleting unused struct's if they look like they're
describing some firmware or hardware struct, even if the kernel doesn't currently
use it.

Dave

> -Kees
> 
> -- 
> Kees Cook
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Powered by blists - more mailing lists