steakhal added a comment.
Looks pretty good.
================
Comment at: clang/docs/analyzer/checkers.rst:704
+optin.portabilityMinor.UnixAPI
+"""""""""""""""""""""""""
+Finds non-severe implementation-defined behavior in UNIX/Posix functions.
----------------
This line should be just as long, as the line above.
================
Comment at: clang/docs/analyzer/checkers.rst:705
+"""""""""""""""""""""""""
+Finds non-severe implementation-defined behavior in UNIX/Posix functions.
+
----------------
Our docs aren't great, but we should have a brief description what the checker
detects, basically here it would be "Find null pointers being passed to
printf-like functions. Usually, passing null pointers to such functions is
implementation defined, thus non-portable. Printf-like functions are: x, y, z."
Illustrate one example for diagnosing a case. Also illustrate one case similar
to the bad one, that is fixed (basically ensure that the pointer is not null
there).
And that's it.
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:45-50
+// The PortabilityMinor package is for checkers that find non-severe
portability
+// issues (see also the Portability package). Such checks may be unwanted for
+// developers who want to ignore minor portability issues, hence they are put
in
+// a separate package.
+def PortabilityMinorOptIn : Package<"portabilityMinor">, ParentPackage<OptIn>;
+
----------------
I don't think we should create a separate package. A separate checker should be
enough in `alpha.unix`, or `alpha.optin`, or just `optin`.
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:1671
+
+def UnixAPIPortabilityMinorChecker : Checker<"UnixAPI">,
+ HelpText<"Finds non-severe implementation-defined behavior in UNIX/Posix
functions">,
----------------
Usually, the user-facing checker name which is the `<"...">` part there,
matches the name of the checker's class name. It helps for maintenance.
I would strongly suggest keeping this naming convention.
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:1673
+ HelpText<"Finds non-severe implementation-defined behavior in UNIX/Posix
functions">,
+ Documentation<NotDocumented>;
+
----------------
Docs are not really optional these days, but we can come back later once the
checker is ironed out.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:88
+class UnixAPIPortabilityMinorChecker
+ : public Checker<check::PreStmt<CallExpr>> {
----------------
Usually, we put separate checkers into their own file.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:89
+class UnixAPIPortabilityMinorChecker
+ : public Checker<check::PreStmt<CallExpr>> {
+public:
----------------
Prefer using the `check::PreCall` callback over the `PreStmt`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:94
+private:
+ mutable std::unique_ptr<BugType> BT_printfPointerConversionSpecifierNULL;
+
----------------
Nowdays, if I'm not mistaken, we just inline initialize these eagerly.
That way you could also avoid marking it mutable.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:102
+ void
+ ReportPrintfPointerConversionSpecifierNULL(clang::ento::CheckerContext &C,
+ ProgramStateRef nullState,
----------------
Feel free to just call it `reportBug`.
Also, in the cpp file, usually we use the namespaces `clang,llvm,ento` already.
You don't need to fully-qualify these names.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:593-594
+
+ if (FName == "printf")
+ CheckPrintfPointerConversionSpecifierNULL(C, CE, 1);
+
----------------
For matching names, we usually use `CallDescriptions` to do it for us.
If think you could use them while also checking if the Call is a
`CallEvent::isGlobalCFunction()`. I suspect, you only wanna check those.
I think you should define a `CallDescriptionMap`, because you will also need
your specific `data_arg_index`...
Look for uses of such maps and you will get inspired.
================
Comment at: clang/test/Analysis/unix-fns.c:81-92
+#ifndef NULL
+#define NULL ((void*) 0)
+#endif
+
+struct FILE_t;
+typedef struct FILE_t FILE;
+
----------------
I'd highly recommend not touching this file to avoid the bulk change in the
expected plist output. Please just introduce a new test file.
================
Comment at: clang/test/Analysis/unix-fns.c:280-289
+// Test pointer constraint.
+void printf_pointer_conversion_specifier_null_pointer_constraint(char *format,
void *pointer1) {
+ void *pointer2 = NULL;
+ printf(format, pointer2); // expected-warning{{The result of passing a null
pointer to the pointer conversion specifier of the printf family of functions
is implementation defined}}
+ if (pointer1 != NULL) {
+ printf(format, pointer1); // no-warning
+ return;
----------------
I would appreciate a test demonstrating that the pointer argument won't get
constrained after the `printf` call. I think such case was already discussed.
We should also demonstrate that unknown pointers (which doesn't known to be
neither null, nor non-null) won't raise issues.
In the checker-logic, we have the `data_args_index` for each `printf`-like
function.
Can you demonstrate that each is handled as expected?
Also have a test when the formatstring itself is null.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154838/new/
https://reviews.llvm.org/D154838
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
