[cfe-dev] [analyzer] Alpha checker statuses.

[Kristóf Umann via cfe-dev]

Should I abandon  https://reviews.llvm.org/D62092 and remove the beta
category in  https://reviews.llvm.org/D62094, would be fine with me closing
this project for good?



On Thu, 23 May 2019 at 00:24, Kristóf Umann <dkszelethus at gmail.com> wrote:

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