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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
