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

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

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

Apologies for inserting myself in here :)

Please use the "[analyzer]" tag instead of "[clang][checkers]" in future changes, because we've used that traditionally for years, and most of us are automatically subscribed to it. The term "checker" is mostly known for these modules in the analyzer, but I'm pretty sure its used in many other places, considering that the main selling point of clang is "checking" the source code better then most other compilers.

---

You stated that

> First simple implementation, infrastructure is ready.

Yet the checker code is still about as long as in D71510 <https://reviews.llvm.org/D71510>. I share the sentiment of @NoQ in D71510#1818438 <https://reviews.llvm.org/D71510#1818438>, this is far too big to review thoroughly, even if we're putting it in alpha first and leave the gritty bits for later. Despite the patch's aim is to introduce an infrastructure, I'm reading code about function filters, special cases for variadic functions, explicit casts to void, 3 different classes for 3 different kinds of return values, sometimes we "save" arguments, but not always, and many other things I failed to understand.

I like to pinpoint to this comment that explains well why we historically preferred smaller patches in the analyzer: D45532#1083670 <https://reviews.llvm.org/D45532#1083670> (which is on, ironically, my patch).

I see that you're using a statement visitor, which always raises the question as to why wasn't a matcher sufficient. Although, I'm not exactly sure why we're using a syntactic check for a problem so inherently pathsensitive at all, and I couldn't really find an explanation in the code.

---

I'm not sure whether we want to hunt cases like this down, or at least not with this checker:

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

Isn't this a dead store or unused variable issue? Or, in this specific case, also a leak.

---

There were valid questions raised by D71510#1818438 <https://reviews.llvm.org/D71510#1818438>, and very few of those were addressed. I tried looking up discussions on discord, cfe-dev, internal mailing lists, the previous patch and I didn't find much. I would prefer to first agree on what we're shooting for and how, and have a battle plan drawn and explained in the code, such as

1. We're sifting out the functions whose return value should always be checked.
2. Should the function be of interest, we're adding its return value to the GDM.
3. Do some analysis on whether the return value was validated.
  1. For this, we're using syntactic analysis (?), which starts with ...
  2. If ... then ...
  3. etc
4. Report errors if so.

The test cases convince me that there is real value to your work, but I'd urge you to take a few steps back and leave room for discussion, and reduce the scope of this patch to only establish the infrastructure, and maybe find the simplest case. I can't stress enough that the idea is great, and even if this ends up as an optin checker we should have it, so please don't take my stance as anything else but caution :)



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:44
+/// as error check.
+class CheckForErrorResultChecker {
+public:
----------------
Please avoid calling any class a checker if they are not inheriting from `Checker<>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72705