[PATCH] D52984: [analyzer] Checker reviewer's checklist
This revision was automatically updated to reflect the committed changes. Closed by commit rL352470: [analyzer] Added a checklist to help checker authors and reviewers (authored by xazax, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D52984?vs=176403=184045#toc Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52984/new/ https://reviews.llvm.org/D52984 Files: cfe/trunk/www/analyzer/checker_dev_manual.html Index: cfe/trunk/www/analyzer/checker_dev_manual.html === --- cfe/trunk/www/analyzer/checker_dev_manual.html +++ cfe/trunk/www/analyzer/checker_dev_manual.html @@ -675,6 +675,111 @@ (gdb) p C.getPredecessor()->getCodeDecl().getBody()->dump() +Making Your Checker Better + +User facing documentation is important for adoption! Make sure the checker list is updated +at the homepage of the analyzer. Also ensure the description is clear to +non-analyzer-developers in Checkers.td. +Warning and note messages should be clear and easy to understand, even if a bit long. + + Messages should start with a capital letter (unlike Clang warnings!) and should not + end with .. + Articles are usually omitted, eg. Dereference of a null pointer -> + Dereference of null pointer. + Introduce BugReporterVisitors to emit additional notes that explain the warning + to the user better. There are some existing visitors that might be useful for your check, + e.g. trackNullOrUndefValue. For example, SimpleStreamChecker should highlight + the event of opening the file when reporting a file descriptor leak. + +If the check tracks anything in the program state, it needs to implement the +checkDeadSymbolscallback to clean the state up. +The check should conservatively assume that the program is correct when a tracked symbol +is passed to a function that is unknown to the analyzer. +checkPointerEscape callback could help you handle that case. +Use safe and convenient APIs! + + Always use CheckerContext::generateErrorNode and +CheckerContext::generateNonFatalErrorNode for emitting bug reports. +Most importantly, never emit report against CheckerContext::getPredecessor. + Prefer checkPreCall and checkPostCall to +checkPreStmtCallExpr and checkPostStmtCallExpr. + Use CallDescription to detect hardcoded API calls in the program. + Simplify C.getState()->getSVal(E, C.getLocationContext()) to C.getSVal(E). + +Common sources of crashes: + + CallEvent::getOriginExpr is nullable - for example, it returns null for an +automatic destructor of a variable. The same applies to some values generated while the +call was modeled, eg. SymbolConjured::getStmt is nullable. + CallEvent::getDecl is nullable - for example, it returns null for a + call of symbolic function pointer. + addTransition, generateSink, generateNonFatalErrorNode, +generateErrorNode are nullable because you can transition to a node that you have already visited. + Methods of CallExpr/FunctionDecl/CallEvent that +return arguments crash when the argument is out-of-bounds. If you checked the function name, +it doesn't mean that the function has the expected number of arguments! +Which is why you should use CallDescription. + Nullability of different entities within different kinds of symbols and regions is usually + documented via assertions in their constructors. + NamedDecl::getName will fail if the name of the declaration is not a single token, +e.g. for destructors. You could use NamedDecl::getNameAsString for those cases. +Note that this method is much slower and should be used sparringly, e.g. only when generating reports +but not during analysis. + Is -analyzer-checker=core included in all test RUN: lines? It was never supported +to run the analyzer with the core checks disabled. It might cause unexpected behavior and +crashes. You should do all your testing with the core checks enabled. + + +Patterns that you should most likely avoid even if they're not technically wrong: + + BugReporterVisitor should most likely not match the AST of the current program point + to decide when to emit a note. It is much easier to determine that by observing changes in + the program state. + In State->getSVal(Region), if Region is not known to be a TypedValueRegion + and the optional type argument is not specified, the checker may accidentally try to dereference a + void pointer. + Checker logic should not depend on whether a certain value is a Loc or NonLoc. +It should be immediately obvious whether the SVal is a Loc or a +NonLoc depending on the AST that is being checked. Checking whether a value +is Loc or Unknown/Undefined or whether the value is +NonLoc or Unknown/Undefined is totally fine. + New symbols should not be constructed in the checker via direct calls to
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ accepted this revision. NoQ added a comment. Thanks!!~ I'm slow, but at least i admit it... Just made myself a fancier phabricator query so that not to forget about patches, so hopefully i won't forget about patches that often anymore :/ I mean, seriously, i'm very happy that we're all doing this sort of stuff together. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52984/new/ https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun updated this revision to Diff 176403. xazax.hun marked 4 inline comments as done. xazax.hun added a comment. - Addressed further comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52984/new/ https://reviews.llvm.org/D52984 Files: www/analyzer/checker_dev_manual.html Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -675,6 +675,111 @@ (gdb) p C.getPredecessor()->getCodeDecl().getBody()->dump() +Making Your Checker Better + +User facing documentation is important for adoption! Make sure the checker list is updated +at the homepage of the analyzer. Also ensure the description is clear to +non-analyzer-developers in Checkers.td. +Warning and note messages should be clear and easy to understand, even if a bit long. + + Messages should start with a capital letter (unlike Clang warnings!) and should not + end with .. + Articles are usually omitted, eg. Dereference of a null pointer -> + Dereference of null pointer. + Introduce BugReporterVisitors to emit additional notes that explain the warning + to the user better. There are some existing visitors that might be useful for your check, + e.g. trackNullOrUndefValue. For example, SimpleStreamChecker should highlight + the event of opening the file when reporting a file descriptor leak. + +If the check tracks anything in the program state, it needs to implement the +checkDeadSymbolscallback to clean the state up. +The check should conservatively assume that the program is correct when a tracked symbol +is passed to a function that is unknown to the analyzer. +checkPointerEscape callback could help you handle that case. +Use safe and convenient APIs! + + Always use CheckerContext::generateErrorNode and +CheckerContext::generateNonFatalErrorNode for emitting bug reports. +Most importantly, never emit report against CheckerContext::getPredecessor. + Prefer checkPreCall and checkPostCall to +checkPreStmtCallExpr and checkPostStmtCallExpr. + Use CallDescription to detect hardcoded API calls in the program. + Simplify C.getState()->getSVal(E, C.getLocationContext()) to C.getSVal(E). + +Common sources of crashes: + + CallEvent::getOriginExpr is nullable - for example, it returns null for an +automatic destructor of a variable. The same applies to some values generated while the +call was modeled, eg. SymbolConjured::getStmt is nullable. + CallEvent::getDecl is nullable - for example, it returns null for a + call of symbolic function pointer. + addTransition, generateSink, generateNonFatalErrorNode, +generateErrorNode are nullable because you can transition to a node that you have already visited. + Methods of CallExpr/FunctionDecl/CallEvent that +return arguments crash when the argument is out-of-bounds. If you checked the function name, +it doesn't mean that the function has the expected number of arguments! +Which is why you should use CallDescription. + Nullability of different entities within different kinds of symbols and regions is usually + documented via assertions in their constructors. + NamedDecl::getName will fail if the name of the declaration is not a single token, +e.g. for destructors. You could use NamedDecl::getNameAsString for those cases. +Note that this method is much slower and should be used sparringly, e.g. only when generating reports +but not during analysis. + Is -analyzer-checker=core included in all test RUN: lines? It was never supported +to run the analyzer with the core checks disabled. It might cause unexpected behavior and +crashes. You should do all your testing with the core checks enabled. + + +Patterns that you should most likely avoid even if they're not technically wrong: + + BugReporterVisitor should most likely not match the AST of the current program point + to decide when to emit a note. It is much easier to determine that by observing changes in + the program state. + In State->getSVal(Region), if Region is not known to be a TypedValueRegion + and the optional type argument is not specified, the checker may accidentally try to dereference a + void pointer. + Checker logic should not depend on whether a certain value is a Loc or NonLoc. +It should be immediately obvious whether the SVal is a Loc or a +NonLoc depending on the AST that is being checked. Checking whether a value +is Loc or Unknown/Undefined or whether the value is +NonLoc or Unknown/Undefined is totally fine. + New symbols should not be constructed in the checker via direct calls to SymbolManager, +unless they are of SymbolMetadata class tagged by the checker, +or they represent newly created values such as the return value in evalCall. +For modeling arithmetic/bitwise/comparison operations, SValBuilder
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added inline comments. Herald added a subscriber: gamesh411. Comment at: www/analyzer/checker_dev_manual.html:678 +Making Your Check Better + Probably `Checker` here as well. Comment at: www/analyzer/checker_dev_manual.html:681 +User facing documentation is important for adoption! Make sure the checker list is updated +at the homepage of the analyzer. Also ensure the description is clear to +non-analyzer-developers in Checkers.td. I think a link to the checker list would be great here. Comment at: www/analyzer/checker_dev_manual.html:738-740 + In State->getSVal(Region), Region is not necessarily a TypedValueRegion + and the optional type argument is not specified. It is likely that the checker + may accidentally try to dereference a void pointer. I think [[ https://reviews.llvm.org/D52984#inline-479362 | this problem ]] isn't fully fixed. Eg., "In `State->getSVal(Region)`, if `Region` is not known to be a `TypedValueRegion` and the optional type argument is not specified, the checker may accidentally try to dereference a void pointer", and i think most other bullets here should be re-phrased a bit. Comment at: www/analyzer/checker_dev_manual.html:772 +generateNonFatalErrorNode (or sequence of such if the split is intended) is +immediately followed by return from the checker callback + Multiple implementations of evalCall in different checkers should not conflict. A `.` is missing here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52984/new/ https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Let's continue the conversation about coding standards somewhere else. Can you please mark inlines as done? https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun updated this revision to Diff 173654. xazax.hun added a comment. - Use the term `checker` instead of `check`. https://reviews.llvm.org/D52984 Files: www/analyzer/checker_dev_manual.html Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -675,6 +675,111 @@ (gdb) p C.getPredecessor()->getCodeDecl().getBody()->dump() +Making Your Check Better + +User facing documentation is important for adoption! Make sure the checker list is updated +at the homepage of the analyzer. Also ensure the description is clear to +non-analyzer-developers in Checkers.td. +Warning and note messages should be clear and easy to understand, even if a bit long. + + Messages should start with a capital letter (unlike Clang warnings!) and should not + end with .. + Articles are usually omitted, eg. Dereference of a null pointer -> + Dereference of null pointer. + Introduce BugReporterVisitors to emit additional notes that explain the warning + to the user better. There are some existing visitors that might be useful for your check, + e.g. trackNullOrUndefValue. For example, SimpleStreamChecker should highlight + the event of opening the file when reporting a file descriptor leak. + +If the check tracks anything in the program state, it needs to implement the +checkDeadSymbolscallback to clean the state up. +The check should conservatively assume that the program is correct when a tracked symbol +is passed to a function that is unknown to the analyzer. +checkPointerEscape callback could help you handle that case. +Use safe and convenient APIs! + + Always use CheckerContext::generateErrorNode and +CheckerContext::generateNonFatalErrorNode for emitting bug reports. +Most importantly, never emit report against CheckerContext::getPredecessor. + Prefer checkPreCall and checkPostCall to +checkPreStmtCallExpr and checkPostStmtCallExpr. + Use CallDescription to detect hardcoded API calls in the program. + Simplify C.getState()->getSVal(E, C.getLocationContext()) to C.getSVal(E). + +Common sources of crashes: + + CallEvent::getOriginExpr is nullable - for example, it returns null for an +automatic destructor of a variable. The same applies to some values generated while the +call was modeled, eg. SymbolConjured::getStmt is nullable. + CallEvent::getDecl is nullable - for example, it returns null for a + call of symbolic function pointer. + addTransition, generateSink, generateNonFatalErrorNode, +generateErrorNode are nullable because you can transition to a node that you have already visited. + Methods of CallExpr/FunctionDecl/CallEvent that +return arguments crash when the argument is out-of-bounds. If you checked the function name, +it doesn't mean that the function has the expected number of arguments! +Which is why you should use CallDescription. + Nullability of different entities within different kinds of symbols and regions is usually + documented via assertions in their constructors. + NamedDecl::getName will fail if the name of the declaration is not a single token, +e.g. for destructors. You could use NamedDecl::getNameAsString for those cases. +Note that this method is much slower and should be used sparringly, e.g. only when generating reports +but not during analysis. + Is -analyzer-checker=core included in all test RUN: lines? It was never supported +to run the analyzer with the core checks disabled. It might cause unexpected behavior and +crashes. You should do all your testing with the core checks enabled. + + +Patterns that you should most likely avoid even if they're not technically wrong: + + BugReporterVisitor should most likely not match the AST of the current program point + to decide when to emit a note. It is much easier to determine that by observing changes in + the program state. + In State->getSVal(Region), Region is not necessarily a TypedValueRegion + and the optional type argument is not specified. It is likely that the checker + may accidentally try to dereference a void pointer. + Checker logic depends on whether a certain value is a Loc or NonLoc. +It should be immediately obvious whether the SVal is a Loc or a +NonLoc depending on the AST that is being checked. Checking whether a value +is Loc or Unknown/Undefined or whether the value is +NonLoc or Unknown/Undefined is totally fine. + New symbols are constructed in the checker via direct calls to SymbolManager, +unless they are of SymbolMetadata class tagged by the checker, +or they represent newly created values such as the return value in evalCall. +For modeling arithmetic/bitwise/comparison operations, SValBuilder should be used. + Custom ProgramPointTags are created within the checker. There is usually +no good
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. Mainly my problem is that checker files can be large and hard to maneuver, some forward declare and document everything, some are a big bowl of spaghetti, and I think having a checklist of how it should be organized to keep uniformity and readability would be great. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. In https://reviews.llvm.org/D52984#1294258, @NoQ wrote: > In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote: > > > Personally, I think it's detrimental to the community for subprojects to > > come up with their own coding guidelines. My preference is for the static > > analyzer to come more in line with the rest of the project (over time, > > organically) in terms of style, terminology, diagnostic wording, etc. > > However, if the consensus is that we want a separate coding standard, I > > think it should be explicitly documented somewhere public and then > > maintained as part of the project. > > > I tihnk it's mostly conventions of using Analyzer-specific APIs, eg. avoid > `addTransition()` hell - i guess we already have that, or how to register > custom immutable maps, or how to implement checker dependencies or > inter-checker APIs, or how much do we want to split modeling and checking > into separate checkers, stuff like that. Indeed, I don't mean to change anything about the LLVM coding guideline, just add a couple Static Analyzer specific additional restrictions. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote: > Personally, I think it's detrimental to the community for subprojects to come > up with their own coding guidelines. My preference is for the static analyzer > to come more in line with the rest of the project (over time, organically) in > terms of style, terminology, diagnostic wording, etc. However, if the > consensus is that we want a separate coding standard, I think it should be > explicitly documented somewhere public and then maintained as part of the > project. I tihnk it's mostly conventions of using Analyzer-specific APIs, eg. avoid `addTransition()` hell - i guess we already have that, or how to register custom immutable maps, or how to implement checker dependencies or inter-checker APIs, or how much do we want to split modeling and checking into separate checkers, stuff like that. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in aaron.ballman wrote: > xazax.hun wrote: > > Szelethus wrote: > > > xazax.hun wrote: > > > > Szelethus wrote: > > > > > Make sure the **checker** list **is** updated > > > > I think at some point we should decide if we prefer the term check or > > > > checker to refer to these things :) Clang Tidy clearly prefers check. > > > That is the distinction I'm aware of too: checkers in the Static > > > Analyzer, checks in clang-tidy. > > My understanding is the following: we want users to use the term `check`, > > since that is more widespread and used by other (non-clang) tools as well. > > The term `checker` is something like a historical artifact in the developer > > community of the static analyzer. But if this is not the case, I am happy > > to change the terminology. But I do want to have some input from rest of > > the community too :) > I grew up with the term "checker", but I feel like "check" may have won the > war. I don't have a strong opinion here though. We have the word "checker" all over the website, in option names, and, most importantly, in the "How to write a //checker// in 24 hours" video. I don't think we have much choice (: https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
aaron.ballman added a comment. In https://reviews.llvm.org/D52984#1294223, @xazax.hun wrote: > - Move the checklist up before additional info in the HTML file. > - Fix minor nits. > - Add missing bullet points (thanks @Szelethus for noticing) > > I did not add any coding convention related item yet. I wonder if it is a > good idea to have our own coding guidelines even if it is derived from the > LLVM one. Personally, I think it's detrimental to the community for subprojects to come up with their own coding guidelines. My preference is for the static analyzer to come more in line with the rest of the project (over time, organically) in terms of style, terminology, diagnostic wording, etc. However, if the consensus is that we want a separate coding standard, I think it should be explicitly documented somewhere public and then maintained as part of the project. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in xazax.hun wrote: > Szelethus wrote: > > xazax.hun wrote: > > > Szelethus wrote: > > > > Make sure the **checker** list **is** updated > > > I think at some point we should decide if we prefer the term check or > > > checker to refer to these things :) Clang Tidy clearly prefers check. > > That is the distinction I'm aware of too: checkers in the Static Analyzer, > > checks in clang-tidy. > My understanding is the following: we want users to use the term `check`, > since that is more widespread and used by other (non-clang) tools as well. > The term `checker` is something like a historical artifact in the developer > community of the static analyzer. But if this is not the case, I am happy to > change the terminology. But I do want to have some input from rest of the > community too :) I grew up with the term "checker", but I feel like "check" may have won the war. I don't have a strong opinion here though. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun added inline comments. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in Szelethus wrote: > xazax.hun wrote: > > Szelethus wrote: > > > Make sure the **checker** list **is** updated > > I think at some point we should decide if we prefer the term check or > > checker to refer to these things :) Clang Tidy clearly prefers check. > That is the distinction I'm aware of too: checkers in the Static Analyzer, > checks in clang-tidy. My understanding is the following: we want users to use the term `check`, since that is more widespread and used by other (non-clang) tools as well. The term `checker` is something like a historical artifact in the developer community of the static analyzer. But if this is not the case, I am happy to change the terminology. But I do want to have some input from rest of the community too :) https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun updated this revision to Diff 173513. xazax.hun marked 11 inline comments as done. xazax.hun added a comment. - Move the checklist up before additional info in the HTML file. - Fix minor nits. - Add missing bullet points (thanks @Szelethus for noticing) I did not add any coding convention related item yet. I wonder if it is a good idea to have our own coding guidelines even if it is derived from the LLVM one. https://reviews.llvm.org/D52984 Files: www/analyzer/checker_dev_manual.html Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -675,6 +675,111 @@ (gdb) p C.getPredecessor()->getCodeDecl().getBody()->dump() +Making Your Check Better + +User facing documentation is important for adoption! Make sure the check list is updated +at the homepage of the analyzer. Also ensure the description is clear to +non-analyzer-developers in Checkers.td. +Warning and note messages should be clear and easy to understand, even if a bit long. + + Messages should start with a capital letter (unlike Clang warnings!) and should not + end with .. + Articles are usually omitted, eg. Dereference of a null pointer -> + Dereference of null pointer. + Introduce BugReporterVisitors to emit additional notes that explain the warning + to the user better. There are some existing visitors that might be useful for your check, + e.g. trackNullOrUndefValue. For example, SimpleStreamChecker should highlight + the event of opening the file when reporting a file descriptor leak. + +If the check tracks anything in the program state, it needs to implement the +checkDeadSymbolscallback to clean the state up. +The check should conservatively assume that the program is correct when a tracked symbol +is passed to a function that is unknown to the analyzer. +checkPointerEscape callback could help you handle that case. +Use safe and convenient APIs! + + Always use CheckerContext::generateErrorNode and +CheckerContext::generateNonFatalErrorNode for emitting bug reports. +Most importantly, never emit report against CheckerContext::getPredecessor. + Prefer checkPreCall and checkPostCall to +checkPreStmtCallExpr and checkPostStmtCallExpr. + Use CallDescription to detect hardcoded API calls in the program. + Simplify C.getState()->getSVal(E, C.getLocationContext()) to C.getSVal(E). + +Common sources of crashes: + + CallEvent::getOriginExpr is nullable - for example, it returns null for an +automatic destructor of a variable. The same applies to some values generated while the +call was modeled, eg. SymbolConjured::getStmt is nullable. + CallEvent::getDecl is nullable - for example, it returns null for a + call of symbolic function pointer. + addTransition, generateSink, generateNonFatalErrorNode, +generateErrorNode are nullable because you can transition to a node that you have already visited. + Methods of CallExpr/FunctionDecl/CallEvent that +return arguments crash when the argument is out-of-bounds. If you checked the function name, +it doesn't mean that the function has the expected number of arguments! +Which is why you should use CallDescription. + Nullability of different entities within different kinds of symbols and regions is usually + documented via assertions in their constructors. + NamedDecl::getName will fail if the name of the declaration is not a single token, +e.g. for destructors. You could use NamedDecl::getNameAsString for those cases. +Note that this method is much slower and should be used sparringly, e.g. only when generating reports +but not during analysis. + Is -analyzer-checker=core included in all test RUN: lines? It was never supported +to run the analyzer with the core checks disabled. It might cause unexpected behavior and +crashes. You should do all your testing with the core checks enabled. + + +Patterns that you should most likely avoid even if they're not technically wrong: + + BugReporterVisitor should most likely not match the AST of the current program point + to decide when to emit a note. It is much easier to determine that by observing changes in + the program state. + In State->getSVal(Region), Region is not necessarily a TypedValueRegion + and the optional type argument is not specified. It is likely that the checker + may accidentally try to dereference a void pointer. + Checker logic depends on whether a certain value is a Loc or NonLoc. +It should be immediately obvious whether the SVal is a Loc or a +NonLoc depending on the AST that is being checked. Checking whether a value +is Loc or Unknown/Undefined or whether the value is +NonLoc or Unknown/Undefined is totally fine. + New symbols are constructed in the checker via direct calls to SymbolManager, +unless they are of
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added inline comments. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in xazax.hun wrote: > Szelethus wrote: > > Make sure the **checker** list **is** updated > I think at some point we should decide if we prefer the term check or checker > to refer to these things :) Clang Tidy clearly prefers check. That is the distinction I'm aware of too: checkers in the Static Analyzer, checks in clang-tidy. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun added inline comments. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in Szelethus wrote: > Make sure the **checker** list **is** updated I think at some point we should decide if we prefer the term check or checker to refer to these things :) Clang Tidy clearly prefers check. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. Thx!! Tiny nits. Comment at: www/analyzer/checker_dev_manual.html:720 +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in +Checkers.td. Szelethus wrote: > ensure the description is clear even to non-developers > to non-developers Good luck explaining use-after-move to my grandma :) Like, i mean, probably to non-analyzer-developers? Comment at: www/analyzer/checker_dev_manual.html:722-723 +Checkers.td. +Introduce BugReporterVisitors to emit additional notes that better explain the found +bug to the user. There are some existing visitors that might be useful for your check, +e.g. trackNullOrUndefValue. For example, SimpleStreamChecker should highlight ...that explain the warning to the user better. Comment at: www/analyzer/checker_dev_manual.html:728 +checkDeadSymbolscallback to clean the state up. +The check should handle correctly if a tracked symbol is passed to a function that is +unknown to the analyzer. checkPointerEscape callback could help you handle ...should correctly handle... Or: ...should conservatively assume that the program is correct when... Comment at: www/analyzer/checker_dev_manual.html:744 + CallEvent::getOriginExpr is nullable - for example, it returns null for an +automatic destructor of a variable. The same applies to all values generated while the +call was modeled, eg. SymbolConjured::getStmt is nullable. Not necessarily all values (if the call is inlined, it may actually happen that all values are concrete), but definitely some values. Comment at: www/analyzer/checker_dev_manual.html:758 +e.g. for destructors. You could use NamedDecl::getNameAsString for those cases. +Note that, this method is much slower and should be used sparringly, e.g. only when generating reports +but not during analysis. I suspect that the comma after 'that' is unnecessary. Comment at: www/analyzer/checker_dev_manual.html:763-764 + + A BugReporterVisitor that matches the AST to decide when to emit note instead of + examining/diffing the states. + In State->getSVal(Region), Region is not necessarily a TypedValueRegion Hmm. Because we're actively bashing the developer in this section, i think we should be careful about being clear what's wrong and what's right and why, while keeping every bullet self-contained (so that it wasn't taken out of the context, i.e. "Bad things: 1. X, 2. ..." shouldn't be accidentally understood as "X"). Eg., ``` Patterns that you should most likely avoid even if they're not technically wrong: * `BugReporterVisitor` should most likely not match the AST of the current program point to decide when to emit a note. It is much easier to determine that by observing changes in the program state. ``` Comment at: www/analyzer/checker_dev_manual.html:777 +For modeling arithmetic/bitwise/comparison operations, SValBuilder should be used. + Custom ProgramPointTags are created within the checker. There is usually +no good reason for a checker to chain multiple nodes together, because checkers aren't worklists. `ProgramPointTags` (?) Comment at: www/analyzer/checker_dev_manual.html:781 +Checkers are encouraged to actively participate in the analysis by sharing + its knowledge about the program state with the rest of the analyzer, + but they should not be disrupting the analysis unnecessarily: their knowledge Comment at: www/analyzer/checker_dev_manual.html:789 +paths needs to be dropped entirely. For example, it is fine to eagerly split +paths while modeling isalpha(x) as long as xi is constrained accordingly on +each path. At the same time, it is not a good idea to split paths over the `x` Comment at: www/analyzer/checker_dev_manual.html:809-812 +Is -analyzer-checker=core included in all test RUN: lines? It was never supported + to run the analyzer with the core checks disabled. It might cause unexpected behavior and + crashes. You should do all your testing with the core checks enabled. + Let's move this bullet to the top? It's kinda lonely here after all those huge sections with tons of sub-bullets. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. In https://reviews.llvm.org/D52984#1269850, @NoQ wrote: > - Warning and note messages should be clear and easy to understand, even if a > bit long. > - Messages should start with a capital letter (unlike Clang warnings!) and > should not end with `.`. > - Articles are usually omitted, eg. `Dereference of a null pointer` -> > `Dereference of null pointer`. I don't see this point in the list. Coding standard related items should probably in a different list, alongside the out-of-alpha items. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in Make sure the **checker** list **is** updated Comment at: www/analyzer/checker_dev_manual.html:720 +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in +Checkers.td. ensure the description is clear even to non-developers https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. Also, context is missing :) https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment.
We probably should also add an entry about some code conventions we use here,
for example, the use of `auto` was debated recently when used with
`SVal::getAs`, maybe something like this:
- As an LLVM subproject, the code in the Static Analyzer should too follow the
https://llvm.org/docs/CodingStandards.html, but we do have some further
restrictions/additions to that:
- Prefer the usage of `auto` when using `SVal::getAs`, and `const auto *`
when using `MemRegion::getAs`. The coding standard states that "//Use auto if
and only if it makes the code more readable or easier to maintain.//", and even
though `SVal::getAs` returns with `llvm::Optional`, which is not
completely trivial, `SVal` is one of the most heavily used types in the Static
Analyzer, and this convention makes the code significantly more readable.
- Use `llvm::SmallString` and `llvm::raw_svector_ostream` to construct report
messages. The coding standard explicitly **doesn't** forbid the use of the
`` library, but we do.
- Do not forward declare or define static functions inside an anonymous
namespace in checker files.
- Document checkers on top of the file, rather then with comments before the
checker class.
- Checker files usually contain the checker class, the registration
functions, and of course the actual implementation (usually several more
functions and data structures) of the checker logic. To make the code more
readable, keep the checker class on top of the file, forward declare all data
structures and implementation functions below that, and put the registration
function on the very bottom. The actual logic should be in between. For example:
//===- MyChecker.cpp -*- C++
-*-==//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===--===//
//
// MyChecker is a tool to show an example how a checker file should be
// organized.
//
//===--===//
#include "ClangSACheckers.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
namespace {
// Any documentation you'd like to put here should instead be put on top of
the
// file, where the checker is described.
class MyChecker : public Checker {
public:
MyChecker() = default;
void checkEndFunction(const ReturnStmt *RS, CheckerContext ) const;
};
/// Helper class that is only here for this example.
class Helper {
public:
/// Helps.
void help();
};
} // end of anonymous namespace
/// Determines whether the problem is solvable.
static bool isProblemSolvable();
//===--===//
// Methods for MyChecker.
//===--===//
void MyChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext ) const {
// TODO: Implement the logic.
}
//===--===//
// Methods for Helper.
//===--===//
void Helper::help() {
// TODO: Actually help.
}
//===--===//
// Utility functions.
//===--===//
static bool isProblemSolvable() {
return false;
}
void ento::registerMyChecker(CheckerManager ) {
auto Chk = Mgr.registerChecker();
}
https://reviews.llvm.org/D52984
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun updated this revision to Diff 172354. xazax.hun added a comment. This new version based on the bullets by NoQ. I also included some additional ones from other lists and added some new ones, e.g. the NamedDecl::getName will fail if the name of the decl is not a single token. I also reordered a bit. Advice that is more advanced and guidelines that are less likely to be violated should be closer to the bottom of the list. Please let me know if I forgot something or the order of the list should be changed. https://reviews.llvm.org/D52984 Files: www/analyzer/checker_dev_manual.html Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -714,6 +714,103 @@ a lot of information. +Making Your Check Better + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in +Checkers.td. +Introduce BugReporterVisitors to emit additional notes that better explain the found +bug to the user. There are some existing visitors that might be useful for your check, +e.g. trackNullOrUndefValue. For example, SimpleStreamChecker should highlight +the event of opening the file when reporting a file descriptor leak. +If the check tracks anything in the program state, it needs to implement the +checkDeadSymbolscallback to clean the state up. +The check should handle correctly if a tracked symbol is passed to a function that is +unknown to the analyzer. checkPointerEscape callback could help you handle +that case. +Use safe and convenient APIs! + + Always use CheckerContext::generateErrorNode and +CheckerContext::generateNonFatalErrorNode for emitting bug reports. +Most importantly, never emit report against CheckerContext::getPredecessor. + Prefer checkPreCall and checkPostCall to +checkPreStmtCallExpr and checkPostStmtCallExpr. + Use CallDescription to detect hardcoded API calls in the program. + Simplify C.getState()->getSVal(E, C.getLocationContext()) to C.getSVal(E). + +Common sources of crashes: + + CallEvent::getOriginExpr is nullable - for example, it returns null for an +automatic destructor of a variable. The same applies to all values generated while the +call was modeled, eg. SymbolConjured::getStmt is nullable. + CallEvent::getDecl is nullable - for example, it returns null for a + call of symbolic function pointer. + addTransition, generateSink, generateNonFatalErrorNode, +generateErrorNode are nullable because you can transition to a node that you have already visited. + Methods of CallExpr/FunctionDecl/CallEvent that +return arguments crash when the argument is out-of-bounds. If you checked the function name, +it doesn't mean that the function has the expected number of arguments! +Which is why you should use CallDescription. + Nullability of different entities within different kinds of symbols and regions is usually + documented via assertions in their constructors. + NamedDecl::getName will fail if the name of the declaration is not a single token, +e.g. for destructors. You could use NamedDecl::getNameAsString for those cases. +Note that, this method is much slower and should be used sparringly, e.g. only when generating reports +but not during analysis. + +The following checker code patterns are not wrong but suspicious: + + A BugReporterVisitor that matches the AST to decide when to emit note instead of + examining/diffing the states. + In State->getSVal(Region), Region is not necessarily a TypedValueRegion + and the optional type argument is not specified. It is likely that the checker + may accidentally try to dereference a void pointer. + Checker logic depends on whether a certain value is a Loc or NonLoc. +It should be immediately obvious whether the SVal is a Loc or a +NonLoc depending on the AST that is being checked. Checking whether a value +is Loc or Unknown/Undefined or whether the value is +NonLoc or Unknown/Undefined is totally fine. + New symbols are constructed in the checker via direct calls to SymbolManager, +unless they are of SymbolMetadata class tagged by the checker, +or they represent newly created values such as the return value in evalCall. +For modeling arithmetic/bitwise/comparison operations, SValBuilder should be used. + Custom ProgramPointTags are created within the checker. There is usually +no good reason for a checker to chain multiple nodes together, because checkers aren't worklists. + +Checkers are encouraged to actively participate in the analysis by sharing + its knowledge about the program state with the rest of the analyzer, + but they should not be disrupting the analysis unnecessarily: + + If a checker splits program state, this must be based on
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. Ok, so where do we put it? :) I think we should put the everyday checklist into the checker developer manual under a title like "Making Your Checker Better" and put the out-of-alpha checklist somewhere into docs/analyzer as plain text (because that's not a sort of info you expect to find in the manual), though we could duplicate some points from there on the website (eg., "write visitors!"). Repository: rC Clang https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:720
+Are Checkers.td and the CMakeLists entry alphabetically ordered?
+
+
aaron.ballman wrote:
> Do we want to add an item for "Is the help text for the checker a useful
> description of what the check does?" or something along those lines? I
> happened to be looking at the help text for checkers and some of it is...
> less than ideal ("Checks MPI code", etc).
Strongly agreed.
Repository:
rC Clang
https://reviews.llvm.org/D52984
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
aaron.ballman added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:720
+Are Checkers.td and the CMakeLists entry alphabetically ordered?
+
+
Do we want to add an item for "Is the help text for the checker a useful
description of what the check does?" or something along those lines? I happened
to be looking at the help text for checkers and some of it is... less than
ideal ("Checks MPI code", etc).
Repository:
rC Clang
https://reviews.llvm.org/D52984
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. I think @NoQ's list would be great to add! @dkrupp, as far as I'm aware, has some works in progress to avoid the usage of HTML, so let's put this on hold for a bit. Repository: rC Clang https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. Yes. Not just for checkers, not just for beginners :) https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. One more thing. From what I've seen, new checkers are in an overwhelming majority (again, from what **I've** seen) made by beginners, so I'd propose to include comments in checkers that are at least in part understandable by a beginner. I don't mean to make make 60% of the code grey, but there are a lot of unexplained non-trivial hackery in some. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. Hmm, i guess i sent this out too early. As per our long-lasting tradition, i forgot the point about the web page :\ Bullets about RUN-lines, Checkers.td descriptions and CMakeLists are also great for everyday use :) https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. My take on the out-of-alpha checklist: - The checker should be evaluated on a large codebase in order to discover crashes and false positives specific to the checker. It should demonstrate the expected reasonably good results on it, with the stress on having as little false positives as possible. - The codebase should contain patterns that the checker checks. It should ideally also contain actual bugs, but it is fine if the checker was inspired by a one-of-a-kind devastating bug while there aren't many other instances of this bug around. - Warning and note messages should be clear and easy to understand, even if a bit long. - Messages should start with a capital letter (unlike Clang warnings!) and should not end with `.`. - Articles are usually omitted, eg. `Dereference of a null pointer` -> `Dereference of null pointer`. - Static Analyzer's checker API saves you from a lot of work, but it still forces you into a certain amount of boilerplate code when it comes to managing entities specific to your checker: - Almost all path-sensitive checkers need their own `BugReporterVisitor` in order to highlight interesting events along the path - or at least a call to `trackNullOrUndefValue`. //(need to wait for https://reviews.llvm.org/D52758)// - For example, SimpleStreamChecker should highlight the event of opening the file when reporting a file descriptor leak. - If the checker tracks anything in the program state, it needs to implement the `checkDeadSymbols` callback to clean the state up. - Unless there's a strong indication that the user is willing to opt-in to a stricter checking, obvious false positives that could be caused by the checker itself should be fixed. For example: - If a checker tracks state of mutable objects enumerated by symbols, escaping symbols into unfamiliar functions must invalidate the state - for example, through `checkPointerEscape`. - For example, SimpleStreamChecker should drop its knowledge about an open file descriptor when it is passed into a function that may potentially close the descriptor. At the same time, it is fine to not escape closed descriptors. - If the checker is intentionally leaving a room for false positives, i.e., it checks for a coding standard that isn't critical to follow when it comes to formal correctness of the program, then there should be a cheap explicit way to suppress the warnings when the violation of the coding standard is intentional. - Fuzzy matching of API function or type names is generally dangerous when it may lead to false positives, fine otherwise. - For example, a checker for LLVM `isa<>` API shouldn't match it as `isa*`, so that it didn't accidentally make assumptions on how `isalpha` works. - It is perfectly fine to have some fuzziness for initial experiments in alpha - that's a great way to figure out which APIs you want to cover. My take on the everyday checklist: - Checkers are encouraged to actively participate in the analysis by sharing its knowledge about the program state with the rest of the analyzer, but they should not be disrupting the analysis unnecessarily: - If a checker splits program state, this must be based on knowledge that the newly appearing branches are definitely possible and worth exploring from the user's perspective. Otherwise the state split should be delayed until there's an indication that one of the paths is taken, or one of the paths needs to be dropped entirely. - For example, it is fine to eagerly split paths while modeling `isalpha(x)` as long as `x` is constrained accordingly on each path. At the same time, it is not a good idea to split paths over the return value of `printf()` while modeling the call because nobody ever checks for errors in `printf`; at best, it'd just double the remaining analysis time. - Caution is advised when using `CheckerContext::generateNonFatalErrorNode` because it generates an independent transition, much like `addTransition`. It is easy to accidentally split paths while using it. - Ideally, try to structure the code so that it was obvious that every `addTransition` or `generateNonFatalErrorNode` (or sequence of such if the split is intended) is immediately followed by return from the checker callback. - Multiple implementations of `evalCall` in different checkers should not conflict. - When implementing `evalAssume`, the checker should always return a non-null state for either the true assumption or the false assumption (or both). - Checkers shall not mutate values of expressions, i.e. use the `ProgramState::BindExpr` API, unless they are fully responsible for computing the value. Under no circumstances should they change non-`Unknown` values of expressions. - Currently the only valid use case for this API in checkers is to model the return value in the `evalCall` callback. - If expression values are incorrect, `ExprEngine` needs to be fixed
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. > is it though? If we simply add a new menu point, it'll require us to modify a huge number of files. We might get away with good comments that explain the situation and making sure that all changes are done with sed, but i guess we'll still be stuck forever. > HTML files are in the repo, there is a script generating them from doxygen, > and the script has to be run manually. Yeah, i guess we could do that. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
george.karpenkov added a comment. > This too will most likely require server maintainers to intervene, so i think > fixing SSI is more realistic. Not really. The way it is done e.g. for http://clang.llvm.org/docs/LibASTMatchersReference.html is the HTML files are in the repo, there is a script generating them from doxygen, and the script has to be run manually. Still seems a lesser evil. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. > we probably also should be moving towards an auto-generated model anyway, if > we want to e.g. automatically build sections of a website from the code. This too will most likely require server maintainers to intervene, so i think fixing SSI is more realistic. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
george.karpenkov added a comment. > inline the header into every page, which is horrible is it though? we probably also should be moving towards an auto-generated model anyway, if we want to e.g. automatically build sections of a website from the code. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. In https://reviews.llvm.org/D52984#1260761, @george.karpenkov wrote: > @Szelethus @xazax.hun Since you guys are looking into the website, do you > know why the top bar has disappeared from all pages? (E.g. > http://clang-analyzer.llvm.org/available_checks.html ) The relevant reading here is https://bugs.llvm.org/show_bug.cgi?id=33811 I believe it's a server configuration error (good old "server-side includes" are disabled), and there's not much we can do other than inline the header into every page, which is horrible. Maybe some sort of javascript hack to load the headers from another URL and then inject them into the page might work. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. I noticed it too, it's already on my TODO list. Didn't look into the causes just yet though. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
george.karpenkov added a comment. Herald added a subscriber: donat.nagy. @Szelethus @xazax.hun Since you guys are looking into the website, do you know why the top bar has disappeared from all pages? (E.g. http://clang-analyzer.llvm.org/available_checks.html ) https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
MTC added a comment. Greate idea! If we can enrich this list, it will not only help the reviewer, but also help beginners, like me, avoid some pitfalls when developing a new checker. I agree with NoQ to classify the lists. In addition to the two categories proposed by NoQ, there is another classof issues, see below, we don't have to take them into account. - Develop a checker(proposed by NoQ). Like: 1. Does it have to be in the path-sensitive manner? 2. Should we move it into `clang-tidy`? 3. Prefer `ASTMatcher` to `StmtVisitor`? 4. Use `CallDescription` to get a stricter filter. 5. Could we use `checkPreCall()` instead of `checkPreStmt(const CallExpr *, )`? 6. Could we put this function model into `StdLibraryFunctionsChecker.cpp`? - Make a good checke(proposed by NoQ). - More general coding standards. Like: 1. Use `isa<>` instead of `dyn_cast<>` if we don't need the casted result. 2. Use `dyn_cast_or_null` instead of `dyn_cast` to enable the null-check. 3. Use `llvm::make_unique` instead of `std::make_unique` because `std::make_unique` is not included in C++11. 4. Avoid unnecessary null-check 5. ... and so on. The third list can be long, and maybe we don't need to take it into account, because developers will follow this standards whether they are developing checkers or contribute to other projects, like LLDB. We need to dig deeper into the analyzer-specific coding standards. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun added inline comments. Comment at: www/analyzer/checker_dev_manual.html:708 +Checker Reviewer Checklist + NoQ wrote: > I think we actually need two separate checklists: > * for common bugs (careful use of non-fatal error nodes, etc. - stuff we > check for on every review) - "Common pitfalls when developing a checker" (?), > also "Analyzer-specific coding standards" (?) > * for out-of-alpha requirements (all boilerplate present, warnings are clear > to all users, evaluated on large codebases) - "What makes a good checker?" (?) Do you think these two lists should be here, or would you prefer two move them to different parts of the document? Comment at: www/analyzer/checker_dev_manual.html:710 + +Is there unintended branching in the exploded graph? +Are there unintended sinks? NoQ wrote: > Most importantly, "If addTransition() with unspecified predecessor is ever > executed after generateNonFatalErrorNode(), is the state split intended?" Is generateNonFatalErrorNode special? Two addTransition calls could have similar unintended effect, though I can imagine that being more rare. Comment at: www/analyzer/checker_dev_manual.html:711 +Is there unintended branching in the exploded graph? +Are there unintended sinks? +Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives? NoQ wrote: > You mean like forgetting to add a transition to the second possible state? My intention here is to check if the sinks are justified. Each sink result in coverage loss, so we do not really want to generate them for minor warnings, when we could continue the analysis in a meaningful manner. Comment at: www/analyzer/checker_dev_manual.html:712 +Are there unintended sinks? +Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives? +Have the checker been run on large projects? NoQ wrote: > You mean suppress false positives with a visitor? Interestingly, right now > there are almost no checkers that do this. But i suspect that there's no good > reason for that, just `trackNullOrUndefValue()` covers most of the needs. > > I believe that this is also the right way to suppress false positives that > come from inlined functions in headers. Such as STL, which we currently > prevent from being inlined entirely, which not only fixes many false > positives, but also introduces some due to information loss. If we instead > suppressed positives when we notice that the bug itself or certain > interesting path events (potentially checker-specific) occur in headers, we > could have had even less false positives and probably some new true positives. I think this guideline is not followed at the moment, but I have the following vision: For each false positive report, there should be a way to rewrite the source code in a natural way to suppress that report. E.g.: let's look at malloc checker: If we take an infeasible path, we could add an assert to help suppress the report (though it is not always trivial, and this is not specific to the checker). If we have a custom free function, we should be able to mark that function, so no memory leak is reported. If we have a memory pool like thing, we should be able to communicate this to the checker somehow. Or in case of conversion checker, we could check for implicit casts, and we could handle explicit casts as intended and do not report them. So false positives could be suppressed by making some implicit casts explicit. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
NoQ added inline comments. Comment at: www/analyzer/checker_dev_manual.html:708 +Checker Reviewer Checklist + I think we actually need two separate checklists: * for common bugs (careful use of non-fatal error nodes, etc. - stuff we check for on every review) - "Common pitfalls when developing a checker" (?), also "Analyzer-specific coding standards" (?) * for out-of-alpha requirements (all boilerplate present, warnings are clear to all users, evaluated on large codebases) - "What makes a good checker?" (?) Comment at: www/analyzer/checker_dev_manual.html:710 + +Is there unintended branching in the exploded graph? +Are there unintended sinks? Most importantly, "If addTransition() with unspecified predecessor is ever executed after generateNonFatalErrorNode(), is the state split intended?" Comment at: www/analyzer/checker_dev_manual.html:711 +Is there unintended branching in the exploded graph? +Are there unintended sinks? +Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives? You mean like forgetting to add a transition to the second possible state? Comment at: www/analyzer/checker_dev_manual.html:712 +Are there unintended sinks? +Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives? +Have the checker been run on large projects? You mean suppress false positives with a visitor? Interestingly, right now there are almost no checkers that do this. But i suspect that there's no good reason for that, just `trackNullOrUndefValue()` covers most of the needs. I believe that this is also the right way to suppress false positives that come from inlined functions in headers. Such as STL, which we currently prevent from being inlined entirely, which not only fixes many false positives, but also introduces some due to information loss. If we instead suppressed positives when we notice that the bug itself or certain interesting path events (potentially checker-specific) occur in headers, we could have had even less false positives and probably some new true positives. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. LGTM! I can think of a couple more, like - making sure that `clang-format` was run on the new files, - not forgetting header guards, - making sure that variables and functions only intended for use within an `assert` call are handled in a way that release build bots won't break on them (`(void)Var`), - every virtual overridden method is marked as `override` (bots love breaking on that), but these are general rules, not specific to the analyzer, even if they are easily forgotten and aren't emphasized enough elsewhere. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun updated this revision to Diff 168664. xazax.hun added a comment. - Added the ideas from Kristof. https://reviews.llvm.org/D52984 Files: www/analyzer/checker_dev_manual.html Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -705,6 +705,20 @@ The list of Implicit Checkers +Checker Reviewer Checklist + +Is there unintended branching in the exploded graph? +Are there unintended sinks? +Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives? +Have the checker been run on large projects? +Did we update the list of available checks on the homepage? +Are GDM structures properly cleaned up? +Escaping into unkown functions is properly handled (if applicable)? +Is -analyzer-checker=core included in all RUN: lines? +Is the description in Checkers.td clear to the users? +Are Checkers.td and the CMakeLists entry alphabetically ordered? + + Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -705,6 +705,20 @@ The list of Implicit Checkers +Checker Reviewer Checklist + +Is there unintended branching in the exploded graph? +Are there unintended sinks? +Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives? +Have the checker been run on large projects? +Did we update the list of available checks on the homepage? +Are GDM structures properly cleaned up? +Escaping into unkown functions is properly handled (if applicable)? +Is -analyzer-checker=core included in all RUN: lines? +Is the description in Checkers.td clear to the users? +Are Checkers.td and the CMakeLists entry alphabetically ordered? + + ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. I love the idea. I got a couple more: Is `-analyzer-checker=core` included in all `RUN:` lines? Is the description in `Checkers.td` clear to a regular user? Is `Checkers.td` and the CMakeLists file alphabetically sorted? Repository: rC Clang https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun created this revision. xazax.hun added reviewers: NoQ, george.karpenkov, Szelethus, rnkovacs, szepet, dcoughlin, a.sidorin, MTC. xazax.hun added a project: clang. Herald added subscribers: mikhail.ramalho, dkrupp, baloghadamsoftware, whisperity. This is a first proposal to have a checklist for reviewing checkers. Feel free to propose additional points or moving it around in the HTML document (or putting it somewhere else?) I think this would come handy to catch some of the most common errors, omissions when reviewing checks. For example, we tend to not update the list of checks at the homepage. + Added Devin for potential word smithing. Repository: rC Clang https://reviews.llvm.org/D52984 Files: www/analyzer/checker_dev_manual.html Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -705,6 +705,17 @@ The list of Implicit Checkers +Checker Reviewer Checklist + +Is there unintended branching in the exploded graph? +Are there unintended sinks? +Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives? +Have the checker been run on large projects? +Did we update the list of available checks on the homepage? +Are GDM structures properly cleaned up? +Escaping into unkown functions is properly handled (if applicable)? + + Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -705,6 +705,17 @@ The list of Implicit Checkers +Checker Reviewer Checklist + +Is there unintended branching in the exploded graph? +Are there unintended sinks? +Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives? +Have the checker been run on large projects? +Did we update the list of available checks on the homepage? +Are GDM structures properly cleaned up? +Escaping into unkown functions is properly handled (if applicable)? + + ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
