[PATCH] D20811: [analyzer] Model some library functions

[Artem Dergachev via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D20811#544250, @dcoughlin wrote:

> I think a good rule of thumb for readability is: suppose you are a maintainer and need to add a summary for a new function. Can you copy the the summary for an existing function and figure out what each component means so you can change it for the new function?


Seems i've written too many summaries to reliably use this rule :)

Could you have a look at another attempt?:

  SUMMARY(isalnum, ARGUMENT_TYPES { IntTy }, RETURN_TYPE(IntTy),
          INVALIDATION_APPROACH(EvalCallAsPure))
    CASE // Boils down to isupper() or islower() or isdigit()
      PRE_CONDITION(ARG_NO(0), CONDITION_KIND(WithinRange))
        RANGE('0', '9')
        RANGE('A', 'Z')
        RANGE('a', 'z')
      END_PRE_CONDITION
      POST_CONDITION(OutOfRange)
        VALUE(0)
      END_POST_CONDITION
    END_CASE
    CASE // The locale-specific range.
      PRE_CONDITION(ARG_NO(0), CONDITION_KIND(WithinRange))
        RANGE(128, 255)
      END_PRE_CONDITION
      // No post-condition. We are completely unaware of
      // locale-specific return values.
    END_CASE
    CASE
      PRE_CONDITION(ARG_NO(0), CONDITION_KIND(OutOfRange))
        RANGE('0', '9')
        RANGE('A', 'Z')
        RANGE('a', 'z')
        RANGE(128, 255)
      END_PRE_CONDITION
      POST_CONDITION(WithinRange)
        VALUE(0)
      END_POST_CONDITION
    END_CASE
  END_SUMMARY


================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:419
@@ -418,1 +418,3 @@
 
+def StdLibraryFunctionsChecker : Checker<"StdLibraryFunctions">,
+  HelpText<"Improve modeling of standard library functions">,
----------------
dcoughlin wrote:
> I know you and Gábor already discussed this -- but shouldn't this be CStdLibraryFunctionsChecker or 'StdCLibraryFunctionsChecker'? Or is is your intent that both C and C++ standard libraries would be modeled by this checker?
Hmm, i just realized what you guys were talking about :) The same checker cpp file and even the same checker object should probably produce different checker list entries here which would go into separate packages (cplusplus for C++ library functions, etc.). We could even split the specifications into different files, but the checker object would still be the same, defined in the same file. Will do.

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:537
@@ +536,3 @@
+      SPEC_DATA {
+        ARGUMENT_TYPES { IntTy },
+        RETURN_TYPE(IntTy),
----------------
dcoughlin wrote:
> The argument and return types seem like more a property of the function than than the summary. Why are they here and not with the function name?
Because this is where C++ initializer list syntax forces them to be. Hiding this detail is, as far as i see, only possible with the means of BEGIN_.../END_... macros (which isn't a big deal i think).

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:547
@@ +546,3 @@
+            RANGE {
+              RET_VAL, RANGE_KIND(OutOfRange),
+              SET { POINT(0) }
----------------
dcoughlin wrote:
> Is it ever the case that this final 'RANGE" constrains anything other than the return value? If not, can 'RET_VAL' be elided?
Some summaries only have pre-conditions: "for this argument constraint, any return value is possible". We should also be able to support void functions, which have no return values.


https://reviews.llvm.org/D20811