[cfe-dev] [analyzer][RFC] Our stance on checker dependencies and disabling core checkers

[Kristóf Umann via cfe-dev]

On Fri, 16 Aug 2019 at 01:42, Kristóf Umann <dkszelethus at gmail.com> wrote:

> We seem to agree on that an even more thought-out dependency system would
> be good, and that separating checkers into modeling and diagnostics
> portions would be ideal, so I'll detail my proposal further:
>
> I googled it, apparently "modelling" is British, and "modeling" is
> American, so for this mail I'm just gonna roll with American. :^)
>
> 1. Don't allow dependency checkers (and implicitly regard them as modeling
> checkers) and modeling checkers to emit diagnostics
>   a.) This implies that if we imagine checkers dependencies as a directed
> graph where any given checker's parent is the checkers it depends on, any
> given checker that emits diagnostics is a leaf and has either no parent, or
> it's parent is a modeling checker, and the path leading to the root of the
> directed graph contains only modeling checkers. Since Checkers.inc contains
> every checker that is a dependency, assert()ing whether the ProgramPointTag
> associated with the report is a dependency checker (given that we have at
> least one testcase for each checker that emits a report) would enforce that
> any report is emitted with the correct tag.
>   b.) The problem that this wouldn't solve is modeling subcheckers, Making
> addTransition take the tag as a mandatory argument would help on this, but
> wouldn't *enforce* it, and woud result in a significant code duplication.
> We define subcheckers as checkers that do not have their own checker
> objects (they are basically glorified checker options), and I don't even
> have any ideas how we could catch these cases. There are two things that
> make this a tad bit better: For one, we don't have a checker like that just
> yet. For another, this would only affect developers when they are
> inspecting the ExplodedGraph, and with some experience, they may be able to
> catch and fix this.
>

Realistically, this is such a low level issue, I don't think it's worth
wasting development time on.


>   c.) This would pretty much make it so that disabling a checker would
> mean suppressing it's diagnostics, nothing else.
>
> 2. Making all modeling checkers hidden
>   a.) Hidden checkers are defined as development only checkers. Riding on
> this excuse, I envision no expectations from end users regarding this. We
> mentioned that maybe if a dependency checker gets disabled but if later in
> the invocation a dependent checker is enabled it should reenable the
> dependency, but I think there is no need to implement this functionality if
> we enforce this rule along with the first one. Anybody who disables a
> development-only checker should know what that means, and any non-developer
> advised to do so is probably doing it to get rid of a crash or similar bad
> behavior.
>   b.) Modeling checkers really are implementation details, and this move
> would make sure that whatever the user enables/disables, the analyzer would
> be responsible for any undesired side effects.
>
> 3. Allow checkers to depend on packages
> Does this need any justification? We should be able to depend on whether
> the sun is out or not, but definitely on packages.
>
> 4. Create the hidden(!) coreModeling package, separate all the modeling
> from core to it, leaving core as a set of highly recommended, but no longer
> mandatory checkers. coreModeling would be a dependency of all pathsensitive
> checkers.
>   a.) Remember how we need to remember to enable core in all test cases
> where at least a single pathsensitive checker is enabled? Well, from now
> on, **not** enabling core would be an even better idea, because we would
> have all the necessary modeling without the obstruction of core
> diagnostics. Also, one less thing to keep in mind!
>   b.) "Make an interface easy to use correctly, and hard to use
> incorrectly" (Scott Meyers): This would be a drastic improvement in that
> department.
>   c.) Don't allow any checker to directly depend on a coreModeling
> checker. This would allow us to evaluate whether there are any
> pathsensitive checkers enabled (which we can only do after we loaded
> everything else into CheckerManager) and enable coreModeling according to
> that. This would of course increase the pressure on codeModeling: if an
> error in this package slips into a release, we're screwed, though this has
> pretty much been the case ever since, my proposal only formalizes it a
> little better.
>
> 5. Regard some operation modifying the ProgramState as modeling
> operations, and regard checkers doing modeling operations as modeling
> checkers.
>   a.) This would both make sure that the ExplodedGraph would remain the
> same aside from the diagnostic reports regardless of what non-developer,
> non-alpha checkers are enabled. We could come up with other heuristics as
> well, such as regarding checkers that add regular range constraints to a
> value to be regarded as modeling checkers.
>

Oops, meant to delete the second sentence add leave it as a separate point.
Should proof read my emails better :)


>   b.) Regard the creation of sink nodes as modeling operations. I bet we
> could somehow combine the function to create regular sink nodes and sink
> error nodes to allow bug reports to be both sink nodes and not be regarded
> as modeling checkers.
>   c.) Modifications of regular range constraints are also modeling
> operations.
>   d.) Document this well, make adding new modeling operations simple, and
> easy to remember for reviewers.
>

Let's actually internalize this point, because know that I think about it,
it rather talks about detecting whether a checker should reside in
apiModeling, rather then simply categorizing checkers into
modeling/diagnostic portions. Like, I plan to split
UninitializedObjectChecker into a checker that emits the current default
diagnostics, and turn the pointer chasing option into its own subchecker,
while the actual logic would be moved to the hidden
UninitializedObjectModeling checker. It's clear that the modeling portion
wouldn't affect the rest of the analysis at all (hence we can disable it if
both diagnostic checkers are also disabled), while a checker like
DynamicMemoryModeling creates sinks and affects the analysis even when none
of its subcheckers is enabled.

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190816/e75d75f7/attachment.html>