<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Sounds good! Let's delay this until we have a clear vision of what
should this package be about.<br>
<br>
<div class="moz-cite-prefix">On 5/22/19 4:56 PM, Kristóf Umann
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAGcXOD7E949dXV+F-=eMZXb8jD0WnWbbuTQbybxc+yPWxgYmOw@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">Should I abandon
<a href="https://reviews.llvm.org/D62092" moz-do-not-send="true">https://reviews.llvm.org/D62092</a> and
remove the beta category in
<a href="https://reviews.llvm.org/D62094" moz-do-not-send="true">https://reviews.llvm.org/D62094</a>,
would be fine with me closing this project for good?
<div><br>
</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, 23 May 2019 at 00:24,
Kristóf Umann <<a href="mailto:dkszelethus@gmail.com"
moz-do-not-send="true">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"><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" target="_blank"
moz-do-not-send="true">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_5883736503230981326gmail-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" moz-do-not-send="true">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_5883736503230981326gmail-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"
moz-do-not-send="true">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"
moz-do-not-send="true">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"
moz-do-not-send="true">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"
moz-do-not-send="true">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"
moz-do-not-send="true">https://reviews.llvm.org/D37806</a><br>
- <a
href="https://reviews.llvm.org/D37807"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">https://reviews.llvm.org/D37807</a><br>
- <a
href="https://reviews.llvm.org/D37809"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">https://reviews.llvm.org/D37809</a><br>
- <a
href="https://reviews.llvm.org/D37812"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">https://reviews.llvm.org/D37812</a><br>
- <a
href="https://reviews.llvm.org/D37963"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">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"
moz-do-not-send="true">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>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>