[PATCH] D72705: [analyzer] Added new checker 'alpha.unix.ErrorReturn'.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D72705#1861750 <https://reviews.llvm.org/D72705#1861750>, @balazske wrote:

> Uploading new diff with added comments and renaming.


Great, very much appreciated! It took a loong time, but I think I got whats going on. So, step by step, this is what we're going if a function of interest is called:

1. Put the return value in a map because we need to keep an eye out for it.
2. If that value is ever read (which we'll know about in `checkLocation`), we will thoroughly check that statement whether it
  1. Qualifies as a check for an error, in which case we take the return value out of the map
  2. Is a read, in which case we emit a warning
  3. Is an assignment (like `for (P = aligned_alloc(4, 8); P;)` or `  void *P = aligned_alloc(4, 8);`), in which case we jump to step 2 with the current statement's parent in the AST.
  4. Does nothing, in which case we leave it in the map for now.

That is the essence of what's going on, is that correct?

> The code is at least saved here, can be used as starting point for smaller changes.

Thanks! For now, lets discuss how the current solution works, let other reviewers take a look at the high level idea, and after reaching some consensus, try to see how we can split this patch up into logical chunks.

In D72705#1859711 <https://reviews.llvm.org/D72705#1859711>, @balazske wrote:

> Generally, is it a problem if there are multiple checkers that may detect same defects but with different approach?


If avoidable, yes. If not, I think its an acceptable sacrifice.

> Or should the code of this checker (for example) made more complicated only to filter out cases that may be detected by other checkers too?

My point was different. Are we sure that it is an error not to check the error value here, before going out of scope?:

  void test_Null_UnusedVar() {
    void *P = aligned_alloc(4, 8);
  } // expected-warning{{Return value was not checked}}

I'm not debating whether this core is incorrect, only the fact that not checking the return value //isn't// the issue here. In any case, let's drop this topic for now, because it adds very little to the real point of this patch, seeing how the problem should be solved as a whole. We should revisit this specific case in a small, followup patch.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:220-223
+/// Information about a specific function call that has an error return code to
+/// check. This data is stored in a map and indexed by the SymbolRef that stands
+/// for the result of the function call.
+struct CalledFunctionData {
----------------
The comments leave me to believe that the correct class name would be `FunctionCallData`, right? Because `FnInfo` does the job of describing the called function, and this one deals with the actual call.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72705/new/

https://reviews.llvm.org/D72705