<div dir="ltr"><div dir="ltr"></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 22 May 2019 at 23:04, 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">
Mmm, i still don't understand what's the *purpose* of the beta
package.<br>
<br>
I can understand the idea of "here's a new checker, we addressed all
issues we know about but there may be other issues that we might
have missed, so please try it out if you're into this sort of
stuff". For now such checkers are usually outright turned on by
default.<br>
<br>
But you're proposing to put checkers into beta when they have known
classes of false positives that are relatively easy to fix. So it's
not for gathering feedback (we already have enough) and there's no
plan on addressing feedback rapidly either (because if you have the
capability to address feedback rapidly, there's no reason to start
with feedback not addressed). It sounds as if we'll still bounce
back all the bug reports against beta checkers with "hey, this is
beta, we know it's broken, right?". If that's the case, then they're
not really different from alpha checkers and i wouldn't be
comfortable providing them to the users.<br></div></blockquote><div><br></div><div>Hmm. I guess you do have a valid point here. My initial thoughts were to move alpha checkers we are actually using and found useful (which in itself imo is a good reason) to beta in order to display them under -analyzer-checker-help, and hide the ugliest stuff deep in the depths of -analyzer-checker-help-alpha. As you said very nicely, "let clang be the single source of truth".</div><div><br></div><div>At the end of the day, we could just make CodeChecker gather the list of checkers by using both -analyzer-checker-help and -analyzer-checker-help-alpha, which would include the really nasty stuff as well, but we've managed it so far.</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">
<div class="gmail-m_3051307291698322375moz-cite-prefix">On 5/22/19 10:57 AM, Kristóf Umann
wrote:<br>
</div>
<blockquote type="cite">
<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" 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">
<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_3051307291698322375gmail-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>
</blockquote>
<br>
</div>
</blockquote></div></div>