<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>