<div dir="ltr"><div>Hi!</div><div><br></div><div>Speaking of suppressions, in an ideal world the user would never need that. I.e. when the analyzer misunderstands the code, the user would be able to add an assert or rewrite the code slightly and the result would be easier to understand both for humans and the analyzer. Unfortunately, this is not the case. We do have false positives when there is no clear way of rewriting the code or adding asserts to make the warning disappear. While it is useful to have an annotation for those cases, there are also some risks involved. Such an annotation can bitrot and redundant annotations can linger even after the code is changed or the analyzer is improved. Those annotations can suppress true positive results. Moreover, users might start to rely on those annotations instead of trying to make the code clearer first, amplifying the effects I mentioned earlier. <br></div><div><br></div><div>I think an ideal suppress annotation would:</div><div>* be applicable to a wide range of scopes, not just function scope</div><div>* have clear documentation indicating this is the last resort and refer to some guidelines how to suppress false positives by improving the code<br></div><div>* come with an easy mechanism to check whether the annotation makes any difference, so we can easily get rid of redundant ones</div><div>* come with an easy mechanism to check how many issues are suppressed by the same annotation (or even expect the user to specify this number as an argument and warn if that does not match the reality?)</div><div><br></div><div>We could also warn for checks when we do believe rewriting the code in a cleaner way should always be possible. I think overall those problems that would require the use of suppression annotations should be treated as a high priority.<br></div><div><br></div><div>Cheers,</div><div>Gabor<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 20 Oct 2020 at 20:24, Valeriy Savchenko via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</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>Hi folks,<div><br></div><div>@Gabor thank you for opening the conversation, I think it is extremely important!</div><div><br></div><div>TBH, at the moment, I am more interested in the first item on the list, so it will be mostly about that item from me.</div><div><br></div><div>I want to start off with one common thing for all the items on the list.  It is actually something that we should always keep in mind as the top priority, - how easy and pleasant it is to use.  If we ask users to add something in their code, it should not look like a gimmick.  If it looks elegant and adds value to the code, the users will be happy to add those annotations.  In the perfect scenario, it makes code more clear even for users who don’t use the analyzer (one example here is Python and type annotations for Mypy).</div><div><br></div><div>So, what’s my take on suppressions (aka item #1).  I believe that Aaron’s suggestion is better for two reasons:</div><div><ul><li>It is more clear in its intentions.  From the very first glance you see that this is a suppression.</li><li>We already have “suppress” attribute!  We just need to adopt it (and maybe add another possible spelling).</li></ul><div><br></div><div>However, the hardest choice here is the string argument for the attribute.  Again, it would be perfect if we can avoid weird identifiers like “<a href="http://java.com" target="_blank">java.com</a>.warnings.alpha.whatever.2020”, it would be good if that string will make it clear for people who don’t use the analyzer what’s going on here.</div><div>Checker names that we use in command line are a bit too verbose, error messages are prone to changes, bug categories are too broad and abstract (the intention is pretty vague if you suppress “Logic error”).  </div><div><br></div><div>My suggestion would be using so-called “Bug types” (we already show them in the summary table in index.html).  Each diagnostic has one, it’s unique and more-or-less descriptive.  If we revise all the existing bug types and try to make them concise and clear, I think it can work!</div><div><br></div><div>As for other items, summaries… Oof that's a tough one.  It looks like we need to parse raw strings anyway, right?  So, keeping in mind UX, maybe it should be something entirely different?  Something like a DSL specifically for summaries.  I don’t have one direct suggestion, but I believe that summaries of all things can be something that all developers can benefit from.</div><div><br></div><div>Here is another thought here though.  Summaries is not something magical, one summary can not and should not describe the entirety of a particular function.  Summaries usually model specific aspects interesting for one particular checker.  So one function, can have a good number of checker-specific summaries.  It can state that it frees the first argument, dereferences the second argument when the third argument is not equal to 42, and returns tainted data.  Expressing everything in one annotation is way too much.  Having a bunch of summaries directly in the code can be fine, but can be terrifying (so many lines before the actual function).  One way to deal with it is APINotes, but that is a completely different topic.</div><div><br></div><div>Honestly, I think the third item is essentially the same thing as summaries, but maybe I don’t see some sort of peculiarity here.</div><div><br></div><div>Phew, I got a bit wordy!</div><div><br></div><div>Cheers!</div><div><br></div><div>Valeriy</div><div><br></div><div><blockquote type="cite"><div>On 20 Oct 2020, at 18:38, Gábor Márton <<a href="mailto:martongabesz@gmail.com" target="_blank">martongabesz@gmail.com</a>> wrote:</div><br><div><div dir="ltr">My example is a bit flawed, so it could look like this:<div><br></div><div><div>1) </div><div>[[clang::csa("supress.somecheck.somefunctionality")]] void foo();<br></div><div><br></div><div>2)</div><div>[[clang::csa("summary.BufferSize.Buffer(0).BufSize(1).BufSizeMultiplier(2)")]]<br></div><div>std::size_t fread( void* buffer, std::size_t size, std::size_t count, std::FILE* stream );<br></div><div><br></div><div>3) </div><div>namespace myNamespace {</div><div>[[clang::csa("taint.sink")]]</div></div><div>void mySink(int x);</div><div>} // myNamespace</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 20, 2020 at 5:34 PM Gábor Márton <<a href="mailto:martongabesz@gmail.com" target="_blank">martongabesz@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">Hi,<br><div><br></div><div>There is an evolving need to configure the Clang Static Analyzer within the analyzed source code itself. We'd like to</div><div>1) suppress specific checkers (we already have an ongoing discussion at <a href="https://reviews.llvm.org/D89638" target="_blank">D89638</a>)</div><div>2) express summaries (mainly argument constraints)</div><div>3) express taint propagation rules for functions (or for global variables like std::cin)</div><div><br></div><div>What if we had one attribute for CSA with a StringArgument?<br>(Actually, we already have that with the `annotate` attribute.)<br></div><div><br></div><div>So we'd have something like this:</div><div>1) [[clang::csa("supress.somecheck.somefunctionality")]]</div><div>2) [[clang::csa("summary.std::fread.BufferSize.Buffer(0).BufSize(1).BufSizeMultiplier(2)")]]<br></div><div>3) [[clang::csa("taint.sink.myNamespace::mySink")]]</div><div><br></div><div>Disadvantages: we must process strings whenever a node has the 'csa' attr attached, we have to come up with a "DSL".<br>Advantages: total flexibility.<br></div><div><br></div><div>I'd like to explore the possible approaches that we could have. For example, Aaron suggested alternatively for the suppression:</div><div>[[clang::suppress("analyzer.somecheck.somefunctionality")]]<br>[[clang::suppress("compiler.warning.12345")]]<br>[[clang::suppress("tidy.check-name.whatever")]]<br></div><div><br></div><div>Thanks,</div><div>Gabor</div></div>
</blockquote></div>
</div></blockquote></div><br></div></div>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div>