<div dir="ltr"><div dir="ltr">I implemented what we discussed here in the following manner: Each checker or checker option must either be in:<br><br>* Alpha: Unfinished, users are strongly discouraged from using it.<div>* Beta: Stable but might produce more false positives and the emitted reports might lack proper explanation<br>* Released: Production ready, very few false positives with easy to understand bug reports.<br><br>Any checker or checker option in addition can be marked as hidden, meaning that it's a developer feature.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 16 May 2019 at 01:24, Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com">noqnoqneo@gmail.com</a>> wrote:<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">
Just wanted to also point out that help is very welcome with
implementing the missing functionality in the checkers that we've
been discussing here.<br>
<br>
(more replies inline)<br>
<br>
<div class="gmail-m_-1071533723269754699moz-cite-prefix">On 5/15/19 5:26 AM, Kristóf Umann
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>How about this:<br>
<br>
<font face="courier new, monospace">-analyzer-checker-help</font>:
Displays production ready (non-modeling) checkers, and beta
checkers with a disclaimer for each description. Don't forget
to patch clang-tidy!</div>
<div><font face="courier new, monospace">-analyzer-checker-help-developer</font>:
Displays only developer (modeling, debug) checkers (renamed
from *-hidden).</div>
<font face="courier new, monospace">-analyzer-checker-help-alpha</font>:
Displays only alpha (non-modeling) checkers with very scary
disclaimers both around the list and for each checker
description.
<div><br>
</div>
<div>Note how each of these is mutually exclusive. I think we
shouldn't make a flag that displays all checkers, but should
allow the following invocation to do so:
<div>
<div><br>
<font face="courier new, monospace">clang -cc1
-analyzer-checker-help -analyzer-checker-help-alpha
-analyzer-checker-help-hidden</font></div>
</div>
</div>
</div>
</blockquote>
<br>
This makes sense to me and i think this is how it should have always
been done.<br>
<br></div></blockquote><div><br></div><div>Note that hidden checkers may only show up in -analyzer-checker-help-developer. Options for implicit checkers are NOT implicitly hidden, but options for alpha checkers are implicitly marked alpha. </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>
<div>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, 15 May 2019 at
12:39, Kristóf Umann <<a href="mailto:dkszelethus@gmail.com" target="_blank">dkszelethus@gmail.com</a>>
wrote:<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 dir="ltr">
<div dir="ltr">+ György Orbán</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, 15 May
2019 at 00:41, Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com" target="_blank">noqnoqneo@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I wanted to
give more visibility to the discussion on the
status of <br>
different Static Analyzer "alpha" (unfinished)
checkers that's going on <br>
on Phabricator (<a href="https://reviews.llvm.org/D57858" rel="noreferrer" target="_blank">https://reviews.llvm.org/D57858</a>).
Story so far: We're <br>
trying to figure out how many of them can be
finished with relatively <br>
little effort (and therefore we should probably
ask around to gather <br>
feedback) and how many are clearly not ready for
any sort of use and <br>
should be hidden until the most glaring bugs are
fixed. For now we <br>
officially treat all alpha checkers as the latter
- so don't use them!<br>
<br>
</blockquote>
<div>Just to add to the summary: despite our
"disapproval" of using alpha checkers, we always
made them very visible, since if you wanted to
list the available checkers, you inevitable came
across them (and they are also lexicographically
greater than the rest of them).</div>
<div><br>
</div>
<div>Due to the lack of branches in our SVN
repository, we used the alpha package to make
development incremental, which inevitable resulted
in some checkers in there being unfinished and
unstable, while others merely need some finishing
touches, and could be used despite being rough
around the patches.</div>
</div>
</div>
</blockquote>
<div>rough around the edges* </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<div>The discussion came up in a patch that plans to
expose checker options, that always existed but
were never listable, unless you read the source
code. However, just like alpha checkers, many of
these also hide features under development, while
other would genuinely be useful to fine tune the
analyzer for a specific project.</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">
This discussion is important because different
people's codebases are <br>
ridiculously different, so it's almost impossible
to estimate the <br>
quality and usefulness of static analysis unless
as many varied <br>
codebases as possible are involved.<br>
<br>
>>! In D57858#1500668, @Szelethus wrote:<br>
> `IteratorChecker` is a prime example that
still suffers from not <br>
ideal FP/TP ratio, but users at Ericsson value it
plenty enough not to <br>
be bothered by it. Many of the changes here and in
private directly <br>
address user bug reports (That's just one, but I
do remember having <br>
others around too!).<br>
<br>
Once it has visitor path notes about where did it
get its iterators <br>
from, some of the iterator checks should
definitely be considered for <br>
being turned on by default. Especially the
mismatched iterator check <br>
that doesn't rely on hardcore constraint solving.
The current upstream <br>
version is not in good shape though; i just tried
it on LLVM and it <br>
currently crashes all over the place with "Symbol
comparison must be a <br>
`SymIntExpr`" assertion (pls ask me for repros if
they aren't obvious). <br>
Also it has false mismatched iterator positives on
`A.insert(B.begin(), <br>
B.end())`.<br>
<br>
<br>
>> In <a href="https://reviews.llvm.org/D57858#1501065" rel="noreferrer" target="_blank">https://reviews.llvm.org/D57858#1501065</a>,
@dkrupp wrote:<br>
> These are the alpha checkers that we are
testing in Ericsson:<br>
<br>
Let me undig my last year's attempt to take a look
at alpha checkers. <br>
The most common "limb" to "miss" in the alpha
checkers is the "bug <br>
visitor" functionality that'd add checker-specific
path notes to the <br>
report, which is almost inevitably necessary for
any path-sensitive <br>
checker. Bug reports without path notes are hard
to understand, but <br>
that's one thing that your users won't tell you:
they often just don't <br>
have their good taste to realize that bug reports
shouldn't be so hard <br>
to understand. The users often take it for granted
that they have to <br>
figure out themselves where do these values come
from, but it's still <br>
our job to not force them to.<br>
<br>
</blockquote>
<div><br>
</div>
<div>The fundamental problem here is, in my opinion,
that "alpha" doesn't describe many of these
checkers too well. I think once a checker is
stable and has a "reasonable" true positive/false
positive ratio, we should move them out of alpha
status: not only would we be able to gather
invaluable feedback for these, but users might
appreciate the feature even with a couple
shortcomings. However, just because they are not
falling apart on their own, these aren't always
production ready -- how about we introduce a
"beta" package?</div>
<div><br>
</div>
<div>Alpha checkers would be incomplete, incorrect
and unstable by definition, and would be hidden
from non-developers. Beta checkers would receive a
disclaimer that they might emit too much false
positives to be production ready and don't emit
ideal bug reports in terms of readability, but are
considered stable and usable.</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">
<br>
> alpha.core.BoolAssignment<br>
<br>
Yes, i agree that this one's pretty useful. It's
currently missing a <br>
visitor that explains why does the analyzer think
that the value is <br>
non-true/false, which is often necessary to
understand the warning.<br>
<br>
</blockquote>
<div>This would be an ideal candidate to be moved to
a beta package.</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I'll be comfy to move it straight to core as long as someone stuffs
a `bugreporter::trackExpressionValue()` into it and we both test it
and see no obvious FP patterns specific to this checker. Unless we
do this, i probably won't be comfy putting it into beta :)<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div>
<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 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">
<br>
> alpha.core.CastSize<br>
<br>
This one's indeed relatively quiet, but i'm seeing
~50 false positives <br>
caused by stuffing metadata at the beginning of a
dynamically allocated <br>
buffer. I.e., allocate a buffer that's 4 bytes
larger than necessary, <br>
use these 4 bytes for our own bookkeeping and
provide a pointer to the <br>
rest of the buffer to be used for the actual
value. I don't see an easy <br>
way to fix these false positives, so i don't see
how to move this out of <br>
alpha.<br>
<br>
<br>
> alpha.core.Conversion<br>
<br>
Interestingly, i haven't seen this one trigger on
our codebases. So i <br>
don't have an opinion here. There's a chance it
might be a good opt-in <br>
check. Do you have an open-source project in mind
on which this check is <br>
actually useful?<br>
<br>
<br>
> alpha.core.DynamicTypeChecker<br>
<br>
I really root for enabling this checker (and
generally improving our <br>
dynamic type-checking), but for now i'm seeing
~600 false positives in <br>
projects that use custom RTTI (something like
`dyn_cast`) and ~300 more <br>
Objective-C-specific false positives. I can take a
look and try to <br>
reduce some of the custom RTTI ones if you're
interested in figuring out <br>
how to fix them; i don't remember if they are easy
to fix.<br>
<br>
<br>
> alpha.core.SizeofPtr<br>
<br>
This checker does indeed find interesting bugs
sometimes, but i'm <br>
overwhelmed by ~300 false positives in which the
sizeof of a pointer is <br>
taken intentionally. This is especially annoying
when the pointer is <br>
hidden behind a typedef and the user doesn't need
to know whether it's a <br>
pointer or not.<br>
<br>
<br>
> alpha.core.TestAfterDivZero<br>
<br>
I don't see any positives of this checker, but
this checker is crazy and <br>
shouldn't have been done this way. It's a
"must-problem" check and we <br>
don't have any sort of infrastructure to even
display this kind of bug <br>
report properly after we find it, let alone to
properly find it. We need <br>
a more-or-less full-featured data flow analysis
engine before we make an <br>
attempt on such checker.<br>
<br>
<br>
> alpha.cplusplus.DeleteWithNonVirtualDtor<br>
<br>
I don't see any positives of this checker. Is it
any better than the <br>
compiler warning that we have for all polymorphic
classes that have no <br>
virtual destructors?<br>
<br>
</blockquote>
<div>Maybe this one too. <br>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Dunno, why duplicate a compiler warning that's already on by default
and more aggressive than the checker?<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div>
<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 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">
<br>
> alpha.security.MallocOverflow<br>
<br>
This one's extremely loud for me (~1500 false
positives). It looks as if <br>
it warns on every `malloc(x * sizeof(int))` (due
to potential integer <br>
overflow during multiplication) so i just don't
see it working as an <br>
AST-based check. We should probably rewrite it on
top of taint analysis <br>
and then it'll need a constraint solver that knows
how to multiply things.<br>
<br>
Like, this is the point where i'd like to ask how
does this happen that <br>
you don't see these false positives. Is this
checker actually quiet for <br>
you? Or are your users addressing these warnings
somehow?<br>
<br>
<br>
> alpha.security.MmapWriteExec<br>
<br>
I don't see any positives of this checker. It
probably needs a visitor <br>
(which is trivial) and it definitely needs a
solution for different <br>
values of macros on different platforms that'd be
better than just <br>
setting them as a config flag.<br>
<br>
<br>
> alpha.security.ReturnPtrRange<br>
<br>
I don't see any positives of this checker. It
definitely needs a visitor <br>
and we might as well join it with the generic
array bound checker. Also <br>
need to figure out how to deal with, say,
vector::end() which is <br>
supposed to return an out-of-bound pointer.<br>
<br>
<br>
> alpha.security.taint.TaintPropagation<br>
<br>
I believe that the generic taint engine is
currently very solid, but <br>
specific checks that we have on top of it are very
much unfinished: they <br>
simply don't react on any sort of validation that
can be used to remove <br>
the taint. Normally that'll involve consulting the
constraint manager <br>
(in case of tainted integers) or modeling
validation routines (in case <br>
of more complicated objects).<br>
<br>
<br>
> alpha.unix.BlockInCriticalSection<br>
<br>
I have ~10 positives and i didn't have a look at
them back then; might <br>
be good. This checker needs a visitor to explain
why do we think we're <br>
in a critical section.<br>
<br>
<br>
> alpha.unix.Chroot<br>
<br>
I don't see any positives of this checker. This
checker needs a visitor <br>
to highlight chroot.<br>
<br>
<br>
> alpha.unix.PthreadLock<br>
<br>
Uhm, i have a few patches on this checker that i
never committed:<br>
- <a href="https://reviews.llvm.org/D37806" rel="noreferrer" target="_blank">https://reviews.llvm.org/D37806</a><br>
- <a href="https://reviews.llvm.org/D37807" rel="noreferrer" target="_blank">https://reviews.llvm.org/D37807</a><br>
- <a href="https://reviews.llvm.org/D37809" rel="noreferrer" target="_blank">https://reviews.llvm.org/D37809</a><br>
- <a href="https://reviews.llvm.org/D37812" rel="noreferrer" target="_blank">https://reviews.llvm.org/D37812</a><br>
- <a href="https://reviews.llvm.org/D37963" rel="noreferrer" target="_blank">https://reviews.llvm.org/D37963</a><br>
And it still needs a visitor. And support for more
APIs, 'cause there <br>
are still false positives caused by unobvious
POSIX APIs that release <br>
the mutex (sometimes conditionally). And once it's
done, i'll be seeing <br>
no positives of this checker; it sounds like a
good checker to have, but <br>
it doesn't seem to find mistakes that are *easy to
make*.<br>
<br>
<br>
> alpha.unix.SimpleStream<br>
> alpha.unix.Stream<br>
<br>
Those need, at least:<br>
- A bug visitor.<br>
- A suppress-on-sink behavior. Otherwise they warn
on every assert <br>
between open and close (~200 false positives for
me).<br>
- Pointer escape support.<br>
Also i vaguely remember that the non-simple
checker is known to cause <br>
more state splits than necessary.<br>
<br>
<br>
> alpha.unix.cstring.NotNullTerminated<br>
<br>
Hmm, this check looks like a walking
.addTransition() bug (unintended <br>
state split) when invoked from getCStringLength().
Also it doesn't seem <br>
to be disabled when the checker is disabled, so i
guess we're kinda <br>
"using" it too. But it's also too quiet to matter,
as it pretty much <br>
only warns when you're trying to compute a
strlen() of a function pointer.<br>
<br>
<br>
> alpha.unix.cstring.OutOfBounds<br>
<br>
<a href="https://bugs.llvm.org/show_bug.cgi?id=41729" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=41729</a>
and it also needs a visitor <br>
for the index. <br>
</blockquote>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote></div></div>