<div dir="ltr"><div><div><div><div><div><div><div><div><div><div><div><div><div>Hi!<br><br></div>I will try to summarize the discussion, let me know if I forgot something.<br><br></div>So far the consensus is that, people are not convinced that false positive suppression is useful for compiler warnings or clang tidy. For both the warnings and clang tidy checks the false positives can usually be suppressed by changing the code (often in a natural way). In case it is not possible to do so, or the warning makes no sense, one should report a bug. (And clang tidy already has a way to suppress results using comments, however that is not general enough for the static analyzer.) However most of us thinks that it is useful to have false positive suppression for the clang static analyzer. The reason is that, there are cases, where code can not be changed in a way that suppresses the warning. One can not use asserts to assert that something is freed. And one can not use the __clang_analyzer__ macro to suppress some of the warnings. <br><br></div>There are some false positive suppression code for the static analyzer available privately and the might be open sourced.<br><br></div>There are two main approaches for suppression.<br><br></div>Hashes:<br></div><div>* Good for 3rd party code or when teams share the same codebase and have different preferences regarding how to handle suppressions<br></div><div>* It is better to store it in json/yaml/textual format, so it can be committed to source control systems<br></div><div>* The user do not need to think about how to suppress an issue<br></div><div><br></div>Comments or pragmas:<br></div>* For path sensitive checks either the uniqueing location must be suppressed or multiple locations which is noisy. In the former case the user might need to read the documentation to be able to properly suppress an issue<br></div>* Using pragmas one can introduce convenience macros, it might also be better to suppress code regions<br></div>* It is better to suppress based on bug type or something like that rather than using the name of the checker (which is not intended to leak out)<br></div>* It is good idea to have line offsets to avoid too much noise when reading code<br></div><div>* Anything against pragmas?<br></div><div><br></div>In both cases:<br></div>* It is crucial to think about suppression lifetime management, so make it easy to review false positives later on (e.g.: a flag to report a note when a suppression comment or hash is unused? also, it is crucial to be able to add comments to suppressions, why is it a false positive, what is the criterion to remove the suppression)<br></div><div>* We should not mix baselining with supression, we should think about them as separate problems.<br></div><div>* Suppression inside clang increases the compatibility between 3rd party tools<br></div><div>* In the long run clang might no longer provide HTML output, it might be generated by a 3rd party tool (or another tool in the repository). It might not be crucial to feed suppressions back to text output.<br></div><div><div><div><div><div><div><div><br></div><div>What about continuing the analysis after a sink node was suppressed?<br></div><div>* It is more likely to find more false positives on such paths.<br></div><div>* In some cases however (if the checker fails to understand the domain specific logic, but the analyzer still has a good understanding of the code), it might make sense. So maybe it might make sense to continue the analysis, when a non-core check result was suppressed, and turn off that check for the rest of the path?<br></div><div>* In the former case maybe the better alternative to make the check not to generate a sink.<br></div><div><br><div><div><div><div><div>Some of us thinks that it might make sense to implement both hash and pragma or comment based suppression.<br></div><div><br></div><div>Regards,<br></div><div>Gábor<br></div></div></div></div></div></div></div></div></div></div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 25 August 2016 at 19:31, Devin Coughlin <span dir="ltr"><<a href="mailto:dcoughlin@apple.com" target="_blank">dcoughlin@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Aug 24, 2016, at 4:56 PM, p23 power <<a href="mailto:p23power@gmail.com" target="_blank">p23power@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>Hi Anna,</div><br><div class="gmail_extra"><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 style="word-wrap:break-word"><div>The main in-source suppression mechanism that the clang static analyzer supports is the ‘<span style="background-color:rgb(238,238,238)">__clang_analyzer__</span>’ macro. I am not sure if you tried using it or not...  so I am curious what are the main limitations of it that you are seeing.<br></div></div></blockquote><div><br></div>The doc page you refer to says: "use this macro to selectively exclude code the analyzer examines" ... so wouldn't this potentially create false positives later in the code path? For example, by excluding code that initializes variables that is required later in the path, the analyzer may then warn about a garbage values.</div><div class="gmail_quote"><br></div><div class="gmail_quote">Additionally, our customers do not want to, and some times are not allow to, add such annotations into the code, especially when it's maintained by another team or company.<br><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div></div><div> </div><div>The static analyzer already has support for suppression hashes in tree. The CodeChecker tool (<a href="https://github.com/Ericsson/codechecker" target="_blank">https://github.com/Ericsson/c<wbr>odechecker</a>) is using them to provide user workflows such as baselining and issue suppression. </div></div></blockquote><div><br></div><div>Where I like this tool, it imposes on the developer that all warnings need to viewed through that interface and it also requires the company to setup the tool in the first place.  Many developers just want to run the analyzer locally and either view the stdout or html reports.  </div></div></div></div></div></blockquote><div><br></div></span><div>I have found the text output to be not very helpful in understanding typical static analyzer reports — and in some cases actively harmful since an unhelpful report is more likely to be interpreted as a false positive. I view the text output as primarily a mechanism for testing the analyzer.</div><div><br></div><div>I also think a good goal is to eventually get the static analyzer itself out of the business of generating HTML. It seems to me that decisions about the user workflow of how to present/filter/navigate issues are much better implemented outside of clang itself. I agree that it is important to be able to run the analyzer and view reports without setup — but I think we can achieve this without sticking the UI logic in the compiler.</div><span class=""><div><br></div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Lastly, you didn't comment on the idea of feeding a yaml/json file containing the issue hashes back into the analyzer for suppression the output.  What are your thoughts on this solution?</div></div></div></div></div></blockquote><div><br></div></span><div>I think the biggest benefit of using a yaml/json file is for issue baselining rather than suppression and I would support incorporating this functionality in clang for that purpose. It is important that the format be easily diffable and stored in version control — i.e. not sqlite database.</div><div><br></div><div>In my view, the key different between baselining and suppression is that suppressions typically have lifecycle comments associated when them. These comments often include:</div><div>- A justification for the suppression. This is typically an explanation of why the diagnostic is a false positive.</div><div>- A criterion for when the suppression can be removed. This is often in the form of a bug filed against the static analyzer to fix the false positive or a bug filed against the codebase under analysis to follow proper coding conventions (e.g., for memory management and ownership).</div><div><br></div><div>Suppression lifecycle comments are important so that when the code in question changes someone can evaluate whether the suppression is still needed and make sure it is not hiding a true positive in the changed codebase. These comments can either be represented in the code itself (in which case the external yaml/json suppressions file would presumably not be needed) or viewed and stored by some external tool like CodeChecker (in which case this suppression/filtering logic is probably better implemented there).</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Devin</div><div><br></div><div><br></div><div><br></div></font></span></div></div></blockquote></div><br></div>