[PATCH] D73898: [analyzer] StdLibraryFunctionsChecker: Add argument constraints

Fri Mar 6 09:54:20 PST 2020

martong marked 9 inline comments as done.
martong added a comment.

In D73898#1894923 <https://reviews.llvm.org/D73898#1894923>, @balazske wrote:

> It may be useful to make a "macro value map" kind of object. Some macros can be added to it as a string, and it is possible to lookup for an `Expr` if one of the added macros is used there. This can be done by checking the concrete (numeric) value of the `Expr` and compare to the value of the macro, or by checking if the expression comes from a macro and take this macro name (use string comparison). Such an object can be useful because the functionality is needed at more checkers, for example the ones I am working on (StreamChecker and ErrorReturnChecker too).

Please see my previous answer to @gamesh411

================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:296
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions">,
+  Dependencies<[StdCLibraryFunctionsChecker]>,
----------------
Szelethus wrote:
> How about we add an example as well?
You mean like NonNull or other constraints?

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697-699
+      // The behavior is undefined if the value of the argument is not
+      // representable as unsigned char or is not equal to EOF. See e.g. C99
+      // 7.4.1.2 The isalpha function (p: 181-182).
----------------
gamesh411 wrote:
> martong wrote:
> > Szelethus wrote:
> > > This is true for the rest of the summaries as well, but shouldn't we retrieve the `unsigned char` size from `ASTContext`?
> > Yes this is a good idea. I will do this.
> > 
> > What bothers me really much, however, is that we should handle EOF in a platform dependent way as well ... and I have absolutely no idea how to do that given that is defined by a macro in a platform specific header file. I am desperately in need for help and ideas about how could we get the value of EOF for the analysed platform.
> If the EOF is not used in the TU analyzed, then there would be no way to find the specific `#define`.
> Another approach would be to check if the value is defined by an expression that is the EOF define (maybe transitively?).
I believe that the given standard C lib implementation (e.g. glibc) must provide a header for the prototypes of these functions where EOF is also defined transitively in any of the dependent system headers. Otherwise user code could misuse the value of EOF and thus the program would behave in an undefined manner.

C99 clearly states that you should #include <ctype.h> to use isalhpa.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:161
+
+    ValueRange negate() const {
+      ValueRange tmp(*this);
----------------
Szelethus wrote:
> Maybe `complement` would be a better name? That sounds a lot more like a set operation. Also, this function highlights well that inheritance might not be the best solution here.
Well, we check the argument constraint validity by trying to apply it's logical negation. In case of a range inclusion this is being out of that range. In case of non-null this is being null. And so on. The logic how we try to check an argument constraint is the same in all cases of the different constraints. And that is the point: in order to support a new kind of constraint we just have to figure out how to "apply" and "negate" one constraint. In my opinion this is a perfect case for polimorphism.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:191
+  ///   * a list of branches - a list of list of ranges -
+  ///     i.e. a list of lists of lists of segments,
+  ///   * a list of argument constraints, that must be true on every branch.
----------------
Szelethus wrote:
> I think that is a rather poor example to help understand what `list of list of ranges` means :) -- Could you try to find something better?
Yeah, that part definitely should be reworded.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:10
 // This checker improves modeling of a few simple library functions.
 // It does not generate warnings.
 //
----------------
Szelethus wrote:
> I suspect this comment is no longer relevant.
Uh, yes.

================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:1-7
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
----------------
Szelethus wrote:
> Hmm, why do we have 2 different test files that essentially do the same? Shouldn't we only have a single one with `analyzer-output=text`?
No, I wanted to have two different test files to test two different things: (1) We do have the constraints applied (here we don't care about the warnings and the path)
(2) Check that we have a warning with the proper tracking and notes.

================
Comment at: clang/test/Analysis/std-c-library-functions.c:1-31
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
----------------
Szelethus wrote:
> What a beautiful sight. Thanks.
Anytime :D

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898