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
