nickdesaulniers accepted this revision. nickdesaulniers added a comment. This revision is now accepted and ready to land.
In D59963#1555595 <https://reviews.llvm.org/D59963#1555595>, @dvyukov wrote: > Re more checks. I guess we can borrow some from the existing checkers: > https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html > https://bottest.wiki.kernel.org/coccicheck > https://lwn.net/Articles/752408/ > https://lwn.net/Articles/691882/ > > They do some checks very well. But if we could do something better (more true > positives, less false positives), that may be useful. > Also figuring out which of the existing checks in clang-tidy/analyzer are > relevant/useful for kernel is another direction. I agree, but we need the core module added first so we can start adding more. > Also making the thread-safety analysis work for kernel may be a big deal. Yes; but there's many many issues there; a GSoC project is being done on this. https://github.com/himanshujha199640/linux-kernel-analysis/blob/report/gsoc19/reports/clang-thread-safety-analysis.md I don't expect that to be solved here in this code review. Since we have additional checks waiting on this to land, LGTM. ================ Comment at: clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c:6 +// Prototypes of the error functions. +void * __must_check ERR_PTR(long error); +long __must_check PTR_ERR(const void *ptr); ---------------- tmroeder wrote: > nickdesaulniers wrote: > > Let's come up with another check; `__must_check` has a bug upstream in the > > kernel sources (I looked into this maybe a month ago). The kernel disables > > a warning group that this would be under, if I re-enable the lone warning, > > then this works properly at compile time. (I forget the warning, and should > > have filed a bug). Point being, fixing this upstream in kernel sources is > > preferable to me to a clang tidy check, but it's a good start. > Good point. How about the related smatch checks in > https://repo.or.cz/smatch.git/blob_plain/HEAD:/check_err_ptr_deref.c > > It looks for cases where possible ERR_PTR values are dereferenced (wrong > because it's not a pointer), and passing non-negative values to ERR_PTR. > > What do you think? Thinking more about it; while we've now cleaned this up in upstream mainline Linux, it still will be a fair amount of work to backport all of the fixes to the LTS branches from which most distros base their releases on. So this check still will have value in that it can currently detect things that Clang won't report for LTS kernels which are very very relevant to at least Android and CrOS. Further, @Nathan-Huckleberry has an additional check that needs the module added here, so let's land this patch so we can start adding more checks to the module. Worst case we remove this check, but for now I do think it will give us more coverage of LTS Linux kernels than we have today with Clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59963/new/ https://reviews.llvm.org/D59963 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
