On Fri, 2017-08-04 at 21:00 -0400, Eric Gallager wrote:
> On 8/4/17, David Malcolm <dmalc...@redhat.com> wrote:
> > This patch kit clearly isn't ready yet as-is (see e.g. the
> > "known unknowns" below), but I'm posting it now in the hope of
> > getting early feedback.
> >
> > Summary
> > =======
> >
> > This patch kit provides an easy way to make integrate 3rd-party
> > static
> > analysis tools into gcc, and have them:
> > (a) report through gcc's diagnostic subsystem, and
> > (b) "watermark" the generated binaries with queryable data on what
> > checkers
> > were run, and what the results were.
> >
> > Here's an example showing gcc running a bank of 3rd-party checkers
> > on this
> > source file:
> >
> > #include <stdlib.h>
> >
> > void test ()
> > {
> > void *ptr_1;
> > void *ptr_2;
> >
> > ptr_1 = malloc (64);
> > if (!ptr_1)
> > return;
> > ptr_2 = malloc (64);
> > if (!ptr_2)
> > return;
> >
> > free (ptr_2);
> > free (ptr_1);
> > }
> >
> > via a simple command-line:
> >
> > $ ./xgcc -B. -c conditional-leak.c -Wrun-analyzers=policy.json
> > conditional-leak.c:13:5: error: Potential leak of memory pointed
> > to by
> > 'ptr_1' [clang-analyzer:Memory leak]
> > return;
> > ^
> > conditional-leak.c:8:11: note: state 1 of 4: Memory is allocated
> > ptr_1 = malloc (64);
> > ^
> > conditional-leak.c:9:7: note: state 2 of 4: Assuming 'ptr_1' is
> > non-null
> > if (!ptr_1)
> > ^
> > conditional-leak.c:12:7: note: state 3 of 4: Assuming 'ptr_2' is
> > null
> > if (!ptr_2)
> > ^
> > conditional-leak.c:13:5: note: state 4 of 4: Potential leak of
> > memory
> > pointed to by 'ptr_1'
> > return;
> > ^
> > conditional-leak.c:13:0: error: Memory leak: ptr_1
> > [cppcheck:memleak]
> > return;
> >
> > Of the checkers, clang's static analyzer and cppcheck both identify
> > the
> > memory leak; the former also identifies the control flow (the other
> > checkers didn't report anything).
> >
> > The idea is to provide a mechanism to make it easy for developers
> > and
> > projects to impose policy on what checkers should be run, and to
> > gate
> > the build if certain tests fail.
> >
> > In this case, the results are treated as hard errors and block the
> > build,
> > but policy could allow them to be warnings.
> >
> > Extensive metadata is captured about what checkers were run, and
> > what
> > they emitted, using the "Firehose" interchange format:
> >
> > http://firehose.readthedocs.io/en/latest/index.html
> >
> > In the case where this doesn't block the build, this can be queried
> > via a
> > contrib/get-static-analysis.py
> > script, so e.g. you can verify that a setuid binary was indeed
> > compiled
> > using all the checkers that you expect it to be.
> >
> > This can also be used to embed data about the code into the
> > watermark.
> > For example, checkers/ianal.py embeds information about "Copyright"
> > lines in the source code into the generated binaries, from where it
> > can be queried (this example is intended as a proof-of-concept
> > rather
> > than as a real license-tracking solution...)
> >
> >
> > Statement of the problem
> > ========================
> >
> > Static analysis is IMHO done too late, if at all: static analysis
> > tools are
> > run
> > as an optional extra, "on the side", rather than in developers'
> > normal
> > workflow, with some kind of "override the compiler and do extra
> > work" hook,
> > which may preclude running more than one analyzer at
> > once. Analysis
> > results
> > are reviewed (if at all) in some kind of on-the-side tool, rather
> > than when
> > the
> > code is being edited, or patches being prepared.
> >
> > It would be better to have an easy way for developers to run
> > analyzer(s)
> > as they're doing development, as part of their edit-compile-test
> > cycle
> > - analysis problems are reported immediately, and can be acted on
> > immediately (e.g. by treating some checker tests as being hard
> > errors).
> >
> > It would also be good to have a way to run analyzer(s) when
> > packages are
> > built, with a variety of precanned policies for analyzers. For
> > example,
> > setuid binaries and network-facing daemons could each be built with
> > a
> > higher strictness of checking.
> >
> > It would also be good to tag binaries with information on what
> > analyzers
> > were run, what options they were invoked with, etc.
> > Potentially have "dump_file" information from optimization passes
> > stored
> > in the metadata also. Have a tool to query all of this.
> >
> > This way a distribution can perform a query like:
> >
> > "show me all setuid binaries that contain code that wasn't
> > checked
> > with $CHECKER with $TEST set to be a hard error"
> >
> > Can/should we break the build if there are issues?
> > Yes: but have a way to opt-in easily: if the tool is well-
> > integrated with
> > the
> > compiler: e.g.
> >
> > -Wrun-analyzers=/usr/share/analyzers/userspace/network-facing-
> > service
> > then upstream developers and packagers can turn on the setting, and
> > see
> > what
> > breaks, and fix it naturally within an compile-edit-test cycle
> >
> > This gives a relatively painless way to opt-in to increasing levels
> > of
> > strictness (e.g. by an upstream project, or by an individual
> > developer).
> >
> > Does this slow the build down?
> > Yes: but you can choose which analyzers run, and can choose to turn
> > them
> > off.
> > It ought to parallelize well. I believe users will prefer to turn
> > them on,
> > and have builders burn up the extra CPU cycles.
> > This may make much more sense for binary distributions (e.g.
> > Fedora,
> > Debian)
> > that it does for things like Gentoo.
> >
> > Example policy files/options might be:
> > -Wrun-analyzers=/usr/share/analyzers/userspace/network-facing-
> > service
> > -Wrun-analyzers=/usr/share/analyzers/userspace/application
> > -Wrun-analyzers=/usr/share/analyzers/userspace/setuid-binary
> > -Wrun-analyzers=/usr/share/analyzers/userspace/default
> > -Wrun-analyzers=/usr/share/analyzers/kernel
> >
> > or whatnot.
> >
> > Idea is to provide mechanism, and for the distribution to decide on
> > some
> > standard policies.
> >
> > This may also allow us to sandbox a gcc plugin by running the
> > plugin inside
> > another cc1, for plugins that add warnings - if the plugin ICEs,
> > then the
> > main
> > cc1 isn't affected (useful for doing mass rebuilds of code using an
> > experimental plugin).
> >
> >
> > Known unknowns
> > ==============
> >
> > How does one suppress a specific false-positive site?
> > Do we need a pragma for it? (though pragmas ought to already
> > affect some
> > of
> > the underlying checkers...)
> >
> > Do we really want .json for the policy format?
> > If we're expecting users to edit this, we need great error
> > messages,
> > and probably support for comments. Would YAML or somesuch be
> > better?
> > Or have them as individual command-line flags, and the policy files
> > are
> > "@" files for gcc.
> >
> > How to mark which checkers are appropriate for which languages?
> >
> > (etc; see also all the FIXMEs in the code...)
> >
> >
> > Dependencies
> > ============
> >
> > The "checkers" subdirectory uses Python 2 or 3, and has a few
> > Python
> > dependencies, including "firehose" and "gccinvocation".
> >
> >
> > How it works
> > ============
> >
> > If enabled, toplev.c starts each of the various checkers from
> > separate
> > threads from near the start of toplev.c, so that the checkers run
> > in
> > parallel with each other, and with the bulk of cc1. Near the end
> > of
> > toplev.c it waits for each thread to finish, and reads the stdout,
> > which is expected to be in Firehose JSON format. This is then sent
> > through the diagnostic subsystem.
> >
> > Each "checker" is a harness script, which "knows" how to invoke
> > the particular 3rd-party tool, and coerce the output from the tool
> > into the common JSON format.
> >
> > Some notes on the data model can be seen here:
> > http://firehose.readthedocs.io/en/latest/data-model.html
> > (though that's expressed as Python objects and XML, rather than
> > the JSON format).
> >
> >
> > Successfully bootstrapped®rtested the combination of the patches
> > on x86_64-pc-linux-gnu (though the only testcases are selftest
> > based
> > unit-tests, rather than DejaGnu tests).
> >
> >
>
> General questions:
>
> 1. When bootstrapping, did you try adding the new -Wrun-analyzers to
> the build system to see what it reports for GCC's own source code?
> It'd be worthwhile to do some dogfooding to determine what kind of
> results it produces
I haven't done that yet; the bootstrapping purely exercised the
selftests (and ensured a lack of warnings on the new code).
> 2. Since -Wrun-analyzers is a warning option, can I turn messages
> from
> it into errors with -Werror=run-analyzers? Will it work with #pragma
> GCC diagnostic push/pop?
Neither of those will work as-is.
The option was actually -frun-analyzers until the day before I posted
the kit; changed it to be a "-W" option was a relatively last-minute
thing, and I think I now prefer it to be -f.
I don't like the patch kit's "monolithic" nature of loading a .json
policy file. I think my preferred approach now is to pass in a -f
option locating a checker script, and to have extra options controlling
it, and for these to be in @ files. So something like:
$ gcc -c foo.c @some-policy.opt
[etc]
# Policy file can be built up from policies for individual checkers:
$ cat some-policy.opt
@clang-analyzer.opt
@cppcheck.opt
# An "@"-file for one checker:
$ cat clang-analyzer.opt
-frun-checker=/path/to/checker/clang-analyzer.py
-fchecker-clang-analyzer=error
-fchecker-clang-analyzer:"something else"=log
or somesuch. I'd prefer things to default to errors.
I agree that we'd need some kind of interaction with pragmas, for more
fine-grained control over test ids.
Maybe have e.g.:
-frun-checker=/path/to/checker/some-checker.py
-Wchecker-clang-analyzer=error
-Wchecker-clang-analyzer:"some specific test"=log
-Wchecker-clang-analyzer:"some specific test"=ignore
so the option to add a checker is "-f", but then the tests within the
checker are controlled by "-W".
(I'm thinking aloud here).
> 3. Do we care about duplicated warnings between the analyzers and GCC
> at all? I'm just thinking, if I put in a bug report requesting a new
> warning for GCC after -Wrun-analyzers is added, will people just
> close
> it saying "Oh you can already get that with -Wrun-analyzers" or
> something? That would be disappointing.
There's plenty of duplication between e.g. clang-analyzer and cppcheck;
the more tests the better IMHO.
> 4. Along those lines, how responsible exactly will GCC be for issues
> with the analyzers it runs? For example, I know splint (from [20/22])
> at least is pretty buggy, it crashed 5 times on me when running it
> manually over my fork of gdb. Does GCC really want to encourage the
> use of potentially buggy external tools that might lead to issues?
Although the patch kit has 7 checkers, I found that the only Free ones
that seemed useful in my (limited) testing were clang-analyzer and
cppcheck.
My thinking was to provide the mechanism to allow people to run 3rd-
party analysis tools; whether those tools are any good is for the users
to decide. But yeah, I agree that there's a potential issue with us
shipping "support" for tools that might not be up-to-scratch.
> ...I thought I had a 5th one but I forget... Anyways, I like the idea
> overall! Keep up the good work!
Thanks
Dave
