[PATCH] D143194: [clang][analyzer] Make messages of StdCLibraryFunctionsChecker user-friendly

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

Szelethus added a reviewer: gamesh411.
Szelethus added a comment.

Awesome, been a long time coming!!

Other than the minor observation of changing "of function" to "to", I'm inclined to accept this patch. We definitely should describe what the value IS, not just what is should be (aside from the cases concerning nullness, I think they are fine as-is), but seems to be another can of worms deserving of its own patch.

The reason why I'd solve the description of the argument separately is that other checkers also suffer from this problem (`alpha.security.ArrayBoundV2`, hello), and it opens conversations such as "What if we can't sensibly describe that value?", and on occasions, we have felt that the go-to solution should be just suppressing the warning. But then again, its an intentional false negative.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:915
   Result += getArgDesc(ArgN);
-  Result += DK == Violation ? " should not be NULL" : " is not NULL";
+  Result += " of function '";
+  Result += getFunctionName(Call);
----------------
Looking at the tests, the word 'function' may be redundant and unnecessary.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:932
   Result += getArgDesc(ArgN);
-  Result += DK == Violation ? " should be " : " is ";
-
-  // Range kind as a string.
-  Kind == OutOfRange ? Result += "out of" : Result += "within";
-
-  // Get the range values as a string.
-  Result += " the range ";
-  if (Ranges.size() > 1)
-    Result += "[";
-  unsigned I = Ranges.size();
-  for (const std::pair<RangeInt, RangeInt> &R : Ranges) {
-    Result += "[";
-    const llvm::APSInt &Min = BVF.getValue(R.first, T);
-    const llvm::APSInt &Max = BVF.getValue(R.second, T);
-    Min.toString(Result);
-    Result += ", ";
-    Max.toString(Result);
-    Result += "]";
-    if (--I > 0)
-      Result += ", ";
+  Result += " of function '";
+  Result += getFunctionName(Call);
----------------
Here too.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:966
   Result += getArgDesc(ArgN);
-  Result += DK == Violation ? " should be " : " is ";
-  Result += "equal to or greater than the value of ";
+  Result += " of function '";
+  Result += getFunctionName(Call);
----------------
Here too.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:980
   }
   return Result.c_str();
 }
----------------
We are saying what the value should be, but neglect to say what it is instead. Something like this would be good:

"The value of the 1st argument to _nonnull should be equal to or less then 10, but is 11".

This is what I (and many of our users IIRC) miss in checker messages, like in those where we are supposed to describe what the value of the index is (besides it causing a buffer overflow).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143194