martong added a comment. In D72705#2238487 <https://reviews.llvm.org/D72705#2238487>, @Szelethus wrote:
> I debated this > <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=emacs_errorreturn&review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&is-unique=on&diff-type=New&checker-name=alpha.unix.ErrorReturn&report-hash=24c61769a1ba68dddd98b4611c1132bd&report-id=7060> > report with @bruntib, depending from where you look at it, its either a > false positive or not -- in short, this is what happens: > > char *p = /* some assumed to be valid c string */ > end = strchr(p, '\n'); > if (end - p > 4) { // <-- warning here on end's invalid use > } > > the guarded code wouldn't be executed, should `end` be zero. Now, if we > interpret the rule in a very literal sense, that > >> The successful completion or failure of each of the standard library >> functions listed in the following table shall be determined either by >> comparing the function’s return value with the value listed in the column >> labeled “Error Return” or by calling one of the library functions mentioned >> in the footnotes. > > then indeed, it doesn't compare the return value with the item in the table, > but it does compare it with superset of it. Now, we can either say that > > 1. A superset isn't good enough, because its a sign of code smell, or > 2. Judge that the user probably did handle the error case, albeit in an > unconventional way, and report like these don't provide any value. > > I don't think we can realistically pursue point (2.). We would need to widen > the context (by going further up the AST with `Stmt::getParent()`) we're > considering for a potential check. However, forming a non-checking expression > that we don't immediately report can quickly get out of control: > > char *p = /* some assumed to be valid c string */; > end = strchr(p, '\n'); > int result = end - p; > if (result > 4) { > } > > You would be right to believe, without context from the code that is yet to > be executed, that the computation of `result` is invalid, because `end` is > unchecked. However, the code complies with the rule interpreted by (2.). This > seems to be a problem that can have infinite complexity without very severe > restriction on the analysis. Indeed, if we wander too far from the original > function call (by creating values that descend from the return value to be > checked), being sure that no checks happened (by point 2.) quickly becomes > unmanageable. > > So, for all practical purposes, we're locked to option (1.). However, the > warning message this way is imprecise -- we're **not** enforcing whether the > error value was checked for error, we're enforcing whether the return value > was checked against its respective failure value, and the message should > reflect that. In fact, the checker description, and some comments should be > clarified in this regard as well. > > I would imagine the pool of functions that deserve such incredibly strict > enforcement is rather small. Time management, maybe, but certainly not string > modeling functions, because problems related to them are solved by a > completely different checker infrastructure (`CStringChecker`), just as > stream modeling functions and memory management. Maybe we should only look > for statement-local bugs (basically a discarded return value) for functions > that don't demand this rigidness, but I suspect there is a clang-tidy check > already out there doing it. Seems like I got what @NoQ meant here. > > In D72705#2088319 <https://reviews.llvm.org/D72705#2088319>, @NoQ wrote: > >> I still don't understand how do we enable this checker, i.e. for whom and >> how often. Is there anything we'll be able to turn on by default, like maybe >> warn on all functions that wear `__attribute__((warn_unused_result))`? > > > > --- > > I'm very happy we got another round of evaluations, we've gotten some > invaluable information out of it. For future patches, it would be great to > have your input on at least some of the reports as you're publishing them. > For the latest project, I think there is clear intent from the developer to > avoid null pointer misuses -- you should justify why a report is still > appropriate there. > > You mentioned in D72705#2187556 <https://reviews.llvm.org/D72705#2187556> > that on `duckdb` you stumbled across 1-2 false positives, but also that you > found 23 reports, and that you ran the checker on `vim`. This may be okay for > an alpha checker, or maybe not, if it points to a fundamental problem in your > approach. Please publish them before updating the patch. If all goes well and > those cases could be patched up later, here are the next important steps: > > - Have a warning message that precisely states what, and how the rule was > violated, even if its done with placeholders, even if you only wanna place a > FIXME and address this later. Either is fine. By the time we ship this > checker, a message that delivers the following information, even if phrased > differently, would be great: "According to the CERT rule ERR33-C, the <return > value/output parameter> must be explicitly checked whether its > <equal/greater/less than> <error value>." > - Expand on the checker description similarly. Mention that checking against > the superset of the error value isn't sufficient. Took one beer to read through this :D Besides the joke, this is a really insightful comment and thanks for sharing your thoughts. I absolutely agree that the checker should state in the warning messages that the value was not checked against its respective failure-value. Also thank you Balazs for your hard work! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72705/new/ https://reviews.llvm.org/D72705 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
