[PATCH 0/4] analyzer: add class function_set and use in various places

[David Malcolm <dmalcolm at redhat dot com>]

On Wed, 2019-12-11 at 14:48 -0500, David Malcolm wrote:
> On Sat, 2019-12-07 at 08:01 -0700, Jeff Law wrote:
> > On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> [...]
> > > diff --git a/gcc/analyzer/analyzer.cc b/gcc/analyzer/analyzer.cc
> > > new file mode 100644
> > > index 0000000..399925c
> > > --- /dev/null
> > > +++ b/gcc/analyzer/analyzer.cc
> [...]
> > > +
> > > +/* Helper function for checkers.  Is the CALL to the given
> > > function
> > > name?  */
> > > +
> > > +bool
> > > +is_named_call_p (const gcall *call, const char *funcname)
> > > +{
> > > +  gcc_assert (funcname);
> > > +
> > > +  tree fndecl = gimple_call_fndecl (call);
> > > +  if (!fndecl)
> > > +    return false;
> > > +
> > > +  return 0 == strcmp (IDENTIFIER_POINTER (DECL_NAME (fndecl)),
> > > funcname);
> > > +}
> > > +
> > > +/* Helper function for checkers.  Is the CALL to the given
> > > function
> > > name,
> > > +   and with the given number of arguments?  */
> > > +
> > > +bool
> > > +is_named_call_p (const gcall *call, const char *funcname,
> > > +		 unsigned int num_args)
> > > +{
> > > +  gcc_assert (funcname);
> > > +
> > > +  if (!is_named_call_p (call, funcname))
> > > +    return false;
> > > +
> > > +  if (gimple_call_num_args (call) != num_args)
> > > +    return false;
> > > +
> > > +  return true;
> > These seem generic enough to live elsewhere.
> 
> There's a potential can of worms here: these functions are used by
> the
> checkers to detect fndecls by name, so that checkers can associate
> behavior with them.  Examples:
> 
> * sm-malloc.c recognizes "malloc" and "free" as being special
> 
> * sm-signal.c knows that fprintf is async-signal-unsafe (but
> currently
> doesn't know about any other fns; it's a proof-of-concept)
> 
> * sm-file.c currently doesn't know anything about "__fsetlocking",
> which is one of the reasons the analyzer currently doesn't detect the
> leak in intl/localealias.c reported as PR 47170, as it assumes that
> the
> FILE * might have been closed and stops tracking state for it.
> 
> etc.
> 
> So we're going to have a lot more "recognize this function by name"
> just to finish the existing proof-of-concept checkers.
> 
> 
> In a perfect world, perhaps we'd have attributes for all of this, and
> the user's code and their system headers would have dutifully marked
> the pertinent decls with attributes, and the use of is_named_call_p
> could be thought of as a bug (or wart)...  but we don't live in that
> perfect world, and a good static analyzer shouldn't need to rely on
> the
> user marking their code.
> 
> There's also integration and chicken-and-egg issues with attributes
> where if we rely e.g. on the user's libc headers having attributes
> for
> the checker, then we need to coordinate with e.g. glibc to add the
> attributes, and implement them, and then the checker doesn't work if
> someone is using a different libc, etc, etc.
> 
> So I think we want a concept of "known functions" in the analyzer,
> where the analyzer can have its own "on the side" model of APIs -
> perhaps a mixture of baked-in (e.g. for malloc/free), perhaps from an
> overridable config file, but I'm not quite sure what form it should
> take.  Maybe even a pragma that lets us tag named functions with
> attributes, or somesuch.
> 
> For now, however, I want to take the pragmatic approach, and use
> these
> functions as needed (and to review this as we gain experience with
> the
> analyzer).
> 
> So I think I prefer to keep them in the analyzer subdir (but I'm
> happy
> to move them if you'd prefer that)
> 
> Does the above sound sane?
 
FWIW I followed up on the above ideas with the following patches.

They add a simple "function_set" class to the analyzer, based on
hardcoded arrays of strings (but they could be loaded from files if need
be), and then use the mechanism in a few places (adding a new
-Wanalyzer-use-of-closed-file warning to sm-file.cc for good
measure).

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
I've pushed these patches to dmalcolm/analyzer on the GCC git mirror.

Dave

David Malcolm (4):
  analyzer: add function-set.cc/h
  analyzer: introduce a set of known async-signal-unsafe functions
  analyzer: add known stdio functions to sm-file.cc (PR analyzer/58237)
  analyzer: add -Wanalyzer-use-of-closed-file

 gcc/Makefile.in                              |   1 +
 gcc/analyzer/analyzer-selftests.cc           |   3 +
 gcc/analyzer/analyzer-selftests.h            |   3 +
 gcc/analyzer/analyzer.opt                    |   4 +
 gcc/analyzer/function-set.cc                 | 191 ++++++++++++++++++
 gcc/analyzer/function-set.h                  |  46 +++++
 gcc/analyzer/sm-file.cc                      | 192 ++++++++++++++++++-
 gcc/analyzer/sm-signal.cc                    |  57 +++++-
 gcc/doc/invoke.texi                          |  13 ++
 gcc/testsuite/gcc.dg/analyzer/file-1.c       |  20 ++
 gcc/testsuite/gcc.dg/analyzer/file-pr58237.c |  72 +++++++
 gcc/testsuite/gcc.dg/analyzer/signal-5.c     |  21 ++
 12 files changed, 615 insertions(+), 8 deletions(-)
 create mode 100644 gcc/analyzer/function-set.cc
 create mode 100644 gcc/analyzer/function-set.h
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/file-pr58237.c
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/signal-5.c

-- 
2.21.0