<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 8/13/19 4:15 PM, Kristóf Umann
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAGcXOD60uyi5s_0dNS6poh_=doxukkUD2mWtu1jN--ZB8HF2rA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div>On Wed, 14 Aug 2019 at 00:12, Artem Dergachev <<a
href="mailto:noqnoqneo@gmail.com" moz-do-not-send="true">noqnoqneo@gmail.com</a>>
wrote:<br>
</div>
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF"> <br>
<br>
<div class="gmail-m_257165592039856256moz-cite-prefix">On
8/12/19 7:20 AM, Kristóf Umann via cfe-dev wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi!
<div><br>
</div>
<div>Our stance has long been that despite being
visible to the user, core checkers shouldn't be
enabled/disabled by hand, because they do important
modeling that any pathsensitive checker may depend
on, potentially causing incorrect reports and
crashes. However, since then, a series of patches
gave us the ability to express dependencies and hide
modeling checkers.</div>
<div><br>
</div>
<div>The core doesn't only do modeling, however, it
emits diagnostics as well, and these diagnostics may
contain false positives, sometimes to the degree
where getting rid of them is desirable, yet we
explicitly state that it shouldn't be disabled. And
this problem doesn't only affect the core itself:
disabling any checker that emits diagnostics and
also is a dependency of some another checker will
disable dependent checkers, which isn't always the
intent.</div>
<div><br>
</div>
<div>When I originally implemented the checker
dependency system, my immediate goal was to fix a
bug causing inconsistent checker names, but I firmly
believe its time to make the it even more rigid:</div>
<div><br>
</div>
<div>* Don't allow dependency checkers to emit
diagnostics. Since the list of these checkers can
easily be retrieved through Checkers.td, assertions
could be used to enforce this. This would solve the
issue of forgetting to create
CheckerProgramPointTags for subcheckers for good,
and a helpful assertion message could guide checker
developers about it.</div>
<div>* Make all dependency checkers hidden. If no
dependent checkers are enabled, let the analyzer
disable them. Disabling a non-hidden checker should
only mean that the diagnostics from it are
undesired.</div>
</div>
</blockquote>
<br>
Back when we were just discussing checker dependencies, i
wasn't sure we need them. My point was that if we instead
split all checkers into checkers that emit actual warnings
but don't do modeling (and therefore don't need to depend
on each other) and checkers that only do modeling but
never do checking (and therefore never need to be turned
off), our hierarchy of checkers becomes fairly flat and
there's no need to write down dependencies at all.<br>
</div>
</blockquote>
<div><br>
</div>
<div>Sounds about right! In this case, just as the const
keyword in a disciplined codebase, dependencies wouldn't be
needed. However, we could use it to enforce this, among
other things, like the use of checker tags. And now that I
think about it, we do really need some sort of dependency
system to keep the checker naming bug in the abyss, though I
can't confidently say this is the only solution. I'll keep
this in the back of my mind and either try to prove that we
need it or mention alternatives.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF"> With these first two bullets we get
closer than ever to that solution, right?<br>
</div>
</blockquote>
<div><br>
</div>
<div>Yup. We would only be able to enforce it with asserts,
but I suspect we have at least a single testcase for each
checker that emits a report, so theoretically, its not even
"getting closer", but rather actually nailing it!
Theoretically.</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF"> Even if we ever need to turn off
modeling-checkers (eg., they have horrible bugs in them
and we suggest disabling them as a workaround), in most
cases it's fine to keep their respective checking-checkers
on (they'll simply not find any bugs: say, MallocChecker
will be unable to find bugs if the memory is not ever
marked as allocated or freed). If it's not fine to keep
them on - well, just turn them off manually, given that
you are already smart enough to turn off the
modeling-checker.<br>
</div>
</blockquote>
<div><br>
</div>
<div>I vaguely remember reading in your workbook that checkers
are relatively lightweight compared to what the core is
doing (I also remember something like a double-digit
percentage of time being spent in SymbolReaper alone), but
why keep it enabled when we know it'll do nothing? Also, if
we know it wouldn't do anything,
-analyzer-list-enabled-checkers would be more precise by not
displaying it there.</div>
<div><br>
</div>
<div>That said, I agree that these are low level issues, and I
wouldn't worry much about them.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF"> Situations when we truly need
dependencies are currently fairly exotic. We may run into
more such situations when we have more complicated checker
interactions (say when we seriously try to model the C++
standard library - i'm pretty sure that's gonna be quite a
mess), but for now our hierarchy of checkers is fairly
flat in practice.<br>
</div>
</blockquote>
<div><br>
</div>
<div>I agree, but I guess when the time comes, an already
mature dependency system would be one less thing to worry
about, right? :)</div>
</div>
</div>
</blockquote>
<br>
Absolutely!!<br>
<br>
<blockquote type="cite"
cite="mid:CAGcXOD60uyi5s_0dNS6poh_=doxukkUD2mWtu1jN--ZB8HF2rA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>Also, we currently have 46 dependencies registered, and
have a total of 164 checkers, meaning that a good percentage
of them is affected -- would you say that a significant
portion of these shouldn't depend on one another? If its not
too much trouble, do you have an example on top of your head
where you believe its appropriate, and one where its
unnecessary?</div>
<div><br>
</div>
<div>(You can always see the list of dependencies in <build
directory>/tools/clang/include/clang/StaticAnalyzer/Checkers/Checkers.inc)</div>
</div>
</div>
</blockquote>
<br>
I mean, let's take, say, MallocChecker. It only does modeling within
its own isolated GDM trait; it's invisible to other families of
checkers. In this case if the model is turned off, the checker has
no effect - it effectively gets silently disabled, we don't need an
additional facility to get it disabled. Most of the time we also
don't need an additional facility to make the model enabled when the
checker is enabled - because it doesn't make much sense to disable
the model in the first place. Such dependencies should still be
there, just for the sake of discipline and sanity (and also for
sorting out problems with checker names), but they aren't an
indication that our dependency trees are actually complex enough to
motivate a facility for dependency management.<br>
<br>
On the contrary, the dependency between MoveChecker and
SmartPtrModel is non-trivial: we made the latter specifically to fix
common false positives of the former, so we recommend enabling the
SmartPtrModel when the user wants MoveChecker. This is an example
where things truly get complicated, and it will get even more
complicated in the future. I think this is the actual motivation we
have for checker dependencies and i'm very happy that it's in place.<br>
<br>
<blockquote type="cite"
cite="mid:CAGcXOD60uyi5s_0dNS6poh_=doxukkUD2mWtu1jN--ZB8HF2rA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<div>* Allow checkers to depend on packages.</div>
<div>* 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.</div>
</div>
</blockquote>
This is equivalent to annotating path-sensitive checkers
as such. This would allow, say, clang-tidy enable
path-insensitive checkers without bringing in
path-sensitive core checkers.<br>
<br>
I'm worried that i need to remember to add a dependency
every time i make a path-sensitive check. Can we enforce
it somehow, or even do that automatically? 'Cause at
run-time we already know whether any path-sensitive
checkers are enabled (by looking at how many subscribers
do path-sensitive callbacks have). Can we use that
information to automatically bring in path-sensitive core
checkers when path-sensitive analysis was requested, or is
it too late at this point?</div>
</blockquote>
<div><br>
</div>
<div>Aye, it sounds totally possible! Resolving dependencies
has to be done in order, but coreModeling would be a special
case -- no checker should directly depend on it, so we would
totally be fine registering them after every other checker,
depending on CheckerManager::hasPathSensitiveCheckers().</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<div>I think this would allow users to opt out of any
undesired diagnostics, without the fear of causing
instabilities. It would simultaneously create a far
more precise representation of dependencies, since
no checker depends on another's diagnostics. Also
note that while this API change is significant, its
also totally backward-compatible, as nothing would
change on the user-facing side.</div>
<div><br>
</div>
<div>Would love to hear your feedback on this!</div>
<div><br>
</div>
<div>Cheers,<br>
Kristóf</div>
</div>
<br>
<fieldset
class="gmail-m_257165592039856256mimeAttachmentHeader"></fieldset>
<pre class="gmail-m_257165592039856256moz-quote-pre">_______________________________________________
cfe-dev mailing list
<a class="gmail-m_257165592039856256moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org" target="_blank" moz-do-not-send="true">cfe-dev@lists.llvm.org</a>
<a class="gmail-m_257165592039856256moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a>
</pre>
</blockquote>
<br>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
</body>
</html>