[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-08-07 Thread Nick Desaulniers via Phabricator via cfe-commits 
nickdesaulniers added inline comments.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:12
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include 
+

tmroeder wrote:
> tmroeder wrote:
> > nickdesaulniers wrote:
> > > tmroeder wrote:
> > > > lebedev.ri wrote:
> > > > > This looks wrong
> > > > Yeah, I'm not sure where that came from. I'll remove it.
> > > Hard to tell, but I don't think this `using` statement was ever removed 
> > > as requested?
> > It's not the "using" statement that was requested to be removed in the 
> > initial diff, but rather "#include ", which I did 
> > remove.
> > 
> > I'm not sure why the "This looks wrong" gets attached to the "using" 
> > statement in later diffs.
> Nick, can you confirm that this addresses your comment?
LGTM


Repository:
  rL LLVM

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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-26 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked an inline comment as done.
tmroeder added inline comments.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:12
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include 
+

tmroeder wrote:
> nickdesaulniers wrote:
> > tmroeder wrote:
> > > lebedev.ri wrote:
> > > > This looks wrong
> > > Yeah, I'm not sure where that came from. I'll remove it.
> > Hard to tell, but I don't think this `using` statement was ever removed as 
> > requested?
> It's not the "using" statement that was requested to be removed in the 
> initial diff, but rather "#include ", which I did 
> remove.
> 
> I'm not sure why the "This looks wrong" gets attached to the "using" 
> statement in later diffs.
Nick, can you confirm that this addresses your comment?


Repository:
  rL LLVM

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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-26 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked an inline comment as done.
tmroeder added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:77
+
+  This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
+  ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return

tmroeder wrote:
> Eugene.Zelenko wrote:
> > tmroeder wrote:
> > > Eugene.Zelenko wrote:
> > > > Release Notes should include short description. One sentence is enough, 
> > > > but it'll good idea to keep it same as first statement of documentation.
> > > Would you like me to submit a patch that removes the second paragraph of 
> > > this description, then?
> > Yes. Documentation is created for details. But you still need to make its 
> > first sentence same as in Release Notes. See other checks and their Release 
> > Notes as example (in previous release branches).
> I'm sorry, I don't understand. What's the referent of "it" in "you still need 
> to make its first sentence same as in Release Notes"? What first sentence 
> needs to match the first sentence of the Release Notes?
> 
> If it's the header file documentation (MustCheckErrs.h), then as far as I can 
> tell, the first paragraph of Release Notes is identical to the first 
> paragraph of the .h file documentation, other than the double backquotes, 
> which I didn't think belonged in a .h file comment.
> 
> What am I missing?
I have looked back, and I think that https://reviews.llvm.org/D65343 should 
address the comment here. Please take a look at let me know.


Repository:
  rL LLVM

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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-26 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked an inline comment as done.
tmroeder added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:77
+
+  This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
+  ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return

Eugene.Zelenko wrote:
> tmroeder wrote:
> > Eugene.Zelenko wrote:
> > > Release Notes should include short description. One sentence is enough, 
> > > but it'll good idea to keep it same as first statement of documentation.
> > Would you like me to submit a patch that removes the second paragraph of 
> > this description, then?
> Yes. Documentation is created for details. But you still need to make its 
> first sentence same as in Release Notes. See other checks and their Release 
> Notes as example (in previous release branches).
I'm sorry, I don't understand. What's the referent of "it" in "you still need 
to make its first sentence same as in Release Notes"? What first sentence needs 
to match the first sentence of the Release Notes?

If it's the header file documentation (MustCheckErrs.h), then as far as I can 
tell, the first paragraph of Release Notes is identical to the first paragraph 
of the .h file documentation, other than the double backquotes, which I didn't 
think belonged in a .h file comment.

What am I missing?


Repository:
  rL LLVM

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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-26 Thread Eugene Zelenko via Phabricator via cfe-commits 
Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:77
+
+  This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
+  ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return

tmroeder wrote:
> Eugene.Zelenko wrote:
> > Release Notes should include short description. One sentence is enough, but 
> > it'll good idea to keep it same as first statement of documentation.
> Would you like me to submit a patch that removes the second paragraph of this 
> description, then?
Yes. Documentation is created for details. But you still need to make its first 
sentence same as in Release Notes. See other checks and their Release Notes as 
example (in previous release branches).


Repository:
  rL LLVM

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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-26 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked an inline comment as done.
tmroeder added inline comments.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:12
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include 
+

nickdesaulniers wrote:
> tmroeder wrote:
> > lebedev.ri wrote:
> > > This looks wrong
> > Yeah, I'm not sure where that came from. I'll remove it.
> Hard to tell, but I don't think this `using` statement was ever removed as 
> requested?
It's not the "using" statement that was requested to be removed in the initial 
diff, but rather "#include ", which I did remove.

I'm not sure why the "This looks wrong" gets attached to the "using" statement 
in later diffs.


Repository:
  rL LLVM

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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-26 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked an inline comment as done.
tmroeder added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:77
+
+  This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
+  ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return

Eugene.Zelenko wrote:
> Release Notes should include short description. One sentence is enough, but 
> it'll good idea to keep it same as first statement of documentation.
Would you like me to submit a patch that removes the second paragraph of this 
description, then?


Repository:
  rL LLVM

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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-25 Thread Tom Roeder via Phabricator via cfe-commits 
This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
tmroeder marked an inline comment as done.
Closed by commit rL367071: [clang-tidy] Add a module for the Linux kernel. 
(authored by tmroeder, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59963?vs=211836&id=211840#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59963

Files:
  clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/trunk/clang-tidy/linuxkernel/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
  clang-tools-extra/trunk/clang-tidy/linuxkernel/MustCheckErrsCheck.h
  clang-tools-extra/trunk/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/test/clang-tidy/linuxkernel-must-check-errs.c

Index: clang-tools-extra/trunk/clang-tidy/linuxkernel/MustCheckErrsCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/linuxkernel/MustCheckErrsCheck.h
+++ clang-tools-extra/trunk/clang-tidy/linuxkernel/MustCheckErrsCheck.h
@@ -0,0 +1,43 @@
+//===--- MustCheckErrsCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_MUSTCHECKERRSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_MUSTCHECKERRSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace linuxkernel {
+
+/// Checks Linux kernel code to see if it uses the results from the functions in
+/// linux/err.h. Also checks to see if code uses the results from functions that
+/// directly return a value from one of these error functions.
+///
+/// This is important in the Linux kernel because ERR_PTR, PTR_ERR, IS_ERR,
+/// IS_ERR_OR_NULL, ERR_CAST, and PTR_ERR_OR_ZERO return values must be checked,
+/// since positive pointers and negative error codes are being used in the same
+/// context. These functions are marked with
+/// __attribute__((warn_unused_result)), but some kernel versions do not have
+/// this warning enabled for clang.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/linuxkernel-must-use-errs.html
+class MustCheckErrsCheck : public ClangTidyCheck {
+public:
+  MustCheckErrsCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace linuxkernel
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_MUSTCHECKERRSCHECK_H
Index: clang-tools-extra/trunk/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
@@ -0,0 +1,53 @@
+//===--- MustCheckErrsCheck.cpp - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "MustCheckErrsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace linuxkernel {
+
+void MustCheckErrsCheck::registerMatchers(MatchFinder *Finder) {
+  auto ErrFn =
+  functionDecl(hasAnyName("ERR_PTR", "PTR_ERR", "IS_ERR", "IS_ERR_OR_NULL",
+  "ERR_CAST", "PTR_ERR_OR_ZERO"));
+  auto NonCheckingStmts = stmt(anyOf(compoundStmt(), labelStmt()));
+  Finder->addMatcher(
+  callExpr(callee(ErrFn), hasParent(NonCheckingStmts)).bind("call"),
+  this);
+
+  auto ReturnToCheck = returnStmt(hasReturnValue(callExpr(callee(ErrFn;
+  auto ReturnsErrFn = functionDecl(hasDescendant(ReturnToChec

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-25 Thread Eugene Zelenko via Phabricator via cfe-commits 
Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:77
+
+  This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
+  ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return

Release Notes should include short description. One sentence is enough, but 
it'll good idea to keep it same as first statement of documentation.


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-25 Thread Nick Desaulniers via Phabricator via cfe-commits 
nickdesaulniers requested changes to this revision.
nickdesaulniers added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:12
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include 
+

tmroeder wrote:
> lebedev.ri wrote:
> > This looks wrong
> Yeah, I'm not sure where that came from. I'll remove it.
Hard to tell, but I don't think this `using` statement was ever removed as 
requested?


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-25 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder updated this revision to Diff 211836.
tmroeder added a comment.

Synchronize the documentation, as requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
  clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.h
  clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c

Index: clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s linuxkernel-must-check-errs %t
+
+#define __must_check __attribute__((warn_unused_result))
+
+// Prototypes of the error functions.
+void * __must_check ERR_PTR(long error);
+long  __must_check PTR_ERR(const void *ptr);
+int  __must_check IS_ERR(const void *ptr);
+int  __must_check IS_ERR_OR_NULL(const void *ptr);
+void * __must_check ERR_CAST(const void *ptr);
+int  __must_check PTR_ERR_OR_ZERO(const void *ptr);
+
+void f() {
+  ERR_PTR(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'ERR_PTR' is unused
+  PTR_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'PTR_ERR' is unused
+  IS_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'IS_ERR' is unused
+  ERR_CAST((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'ERR_CAST' is unused
+out:
+  PTR_ERR_OR_ZERO((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'PTR_ERR_OR_ZERO' is unused
+}
+
+void *f1() {
+  return ERR_PTR(0);
+}
+
+long f2() {
+  if (IS_ERR((void *)0)) {
+return PTR_ERR((void *)0);
+  }
+  return -1;
+}
+
+void f3() {
+  f1();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'f1' is unused but represents an error value
+  f2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'f2' is unused but represents an error value
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -183,6 +183,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   linuxkernel-must-use-errs
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - linuxkernel-must-use-errs
+
+linuxkernel-must-use-errs
+=
+
+Checks for cases where the kernel error functions ``ERR_PTR``,
+``PTR_ERR``, ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and
+``PTR_ERR_OR_ZERO`` are called but the results are not used. These
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+
+This also checks for unused values returned by functions that return
+``ERR_PTR``.
+
+Examples:
+
+.. code-block:: c
+
+  /* Trivial unused call to an ERR function */
+  PTR_ERR_OR_ZERO(some_function_call());
+
+  /* A function that returns ERR_PTR. */
+  void *fn() { ERR_PTR(-EINVAL); }
+
+  /* An invalid use of fn. */
+  fn();
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,7 +67,19 @@
 Improvements to clang-tidy
 --
 
-The improvements are...
+- New :doc:`linuxkernel-must-use-errs
+  ` check.
+
+  Checks Linux kernel code to see if it uses the results from the functions in
+  ``linux/err.h``. Also checks to see if code uses the results from functions that
+  directly return a value from one of these error functions.
+
+  This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
+  ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return
+  values must be checked, since positive pointers and

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-25 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked 2 inline comments as done.
tmroeder added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:75
+
+  Checks Linux Kernel code to see if it uses the results from the functions in
+  linux/err.h. Also checks to see if code uses the results from functions that

Eugene.Zelenko wrote:
> Please highlight linux/err.h with double back-ticks. I think will be good 
> idea to synchronize documentation text.
Thanks, I've synchronized the documentation now between this file and the 
header.


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-25 Thread Eugene Zelenko via Phabricator via cfe-commits 
Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:75
+
+  Checks Linux Kernel code to see if it uses the results from the functions in
+  linux/err.h. Also checks to see if code uses the results from functions that

Please highlight linux/err.h with double back-ticks. I think will be good idea 
to synchronize documentation text.


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-25 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder updated this revision to Diff 211783.
tmroeder added a comment.

Sync'ing to the latest HEAD commit on master.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
  clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.h
  clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c

Index: clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s linuxkernel-must-check-errs %t
+
+#define __must_check __attribute__((warn_unused_result))
+
+// Prototypes of the error functions.
+void * __must_check ERR_PTR(long error);
+long  __must_check PTR_ERR(const void *ptr);
+int  __must_check IS_ERR(const void *ptr);
+int  __must_check IS_ERR_OR_NULL(const void *ptr);
+void * __must_check ERR_CAST(const void *ptr);
+int  __must_check PTR_ERR_OR_ZERO(const void *ptr);
+
+void f() {
+  ERR_PTR(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'ERR_PTR' is unused
+  PTR_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'PTR_ERR' is unused
+  IS_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'IS_ERR' is unused
+  ERR_CAST((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'ERR_CAST' is unused
+out:
+  PTR_ERR_OR_ZERO((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'PTR_ERR_OR_ZERO' is unused
+}
+
+void *f1() {
+  return ERR_PTR(0);
+}
+
+long f2() {
+  if (IS_ERR((void *)0)) {
+return PTR_ERR((void *)0);
+  }
+  return -1;
+}
+
+void f3() {
+  f1();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'f1' is unused but represents an error value
+  f2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'f2' is unused but represents an error value
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -183,6 +183,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   linuxkernel-must-use-errs
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - linuxkernel-must-use-errs
+
+linuxkernel-must-use-errs
+=
+
+Checks for cases where the kernel error functions ``ERR_PTR``,
+``PTR_ERR``, ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and
+``PTR_ERR_OR_ZERO`` are called but the results are not used. These
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+
+This also checks for unused values returned by functions that return
+``ERR_PTR``.
+
+Examples:
+
+.. code-block:: c
+
+  /* Trivial unused call to an ERR function */
+  PTR_ERR_OR_ZERO(some_function_call());
+
+  /* A function that returns ERR_PTR. */
+  void *fn() { ERR_PTR(-EINVAL); }
+
+  /* An invalid use of fn. */
+  fn();
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -69,6 +69,13 @@
 
 The improvements are...
 
+- New :doc:`linuxkernel-must-use-errs
+  ` check.
+
+  Checks Linux Kernel code to see if it uses the results from the functions in
+  linux/err.h. Also checks to see if code uses the results from functions that
+  directly return a value from one of these error functions.
+
 Improvements to include-fixer
 -
 
Index: clang-tools-extra/clang-tidy/tool/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/tool/CMakeLists.txt
+++ clang-tools-extra/clan

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-07-24 Thread Nick Desaulniers via Phabricator via cfe-commits 
nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

In D59963#195 , @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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-06-24 Thread Dmitry Vyukov via Phabricator via cfe-commits 
dvyukov added a comment.

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.
Also making the thread-safety analysis work for kernel may be a big deal.


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-06-24 Thread Dmitry Vyukov via Phabricator via cfe-commits 
dvyukov added a comment.

I assume you tried to run it on the kernel. Please post the current output 
somewhere.


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

Re: [PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-06-20 Thread Nick Desaulniers via cfe-commits 
Sounds like a good start.

On Thu, Jun 20, 2019 at 11:55 AM Tom Roeder via Phabricator
 wrote:
>
> tmroeder marked an inline comment as done.
> tmroeder added inline comments.
>
>
> 
> 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);
> 
> 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?
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D59963/new/
>
> https://reviews.llvm.org/D59963
>
>
>


-- 
Thanks,
~Nick Desaulniers
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-06-20 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked an inline comment as done.
tmroeder added inline comments.



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);

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?


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-06-11 Thread Nick Desaulniers via Phabricator via cfe-commits 
nickdesaulniers added a comment.

See also: https://github.com/ClangBuiltLinux/linux/issues/520


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-06-07 Thread Nick Desaulniers via Phabricator via cfe-commits 
nickdesaulniers added inline comments.



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);

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.


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-23 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:24
+  "ERR_CAST", "PTR_ERR_OR_ZERO"));
+  auto NonCheckingStmts = stmt(anyOf(compoundStmt(), labelStmt()));
+  Finder->addMatcher(

tmroeder wrote:
> lebedev.ri wrote:
> > Are there any rules what kernel considers to be checking and not checking?
> > This i think e.g. will accept cast-to-void, assignment to a variable and 
> > then ignoring it, etc.
> > 
> Yes, this check is extremely simplistic. My understanding of clang-tidy 
> checks was that the most value comes from them catching obviously wrong 
> behavior and not having any false positives. I didn't think they were 
> supposed to catch all the ways in which something could be wrong.
> 
> But I've never written a clang-tidy check before. What is their expected 
> purpose?
We are more forgiving of false positives in clang-tidy than we are in the 
compiler proper, so we're okay with checks being more chatty, within reason. 
Obviously, the less false positives (and false negatives), the better because 
it's more useful for the user. 



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:39
+  if (MatchedCallExpr &&
+  MatchedCallExpr->hasUnusedResultAttr(*Result.Context)) {
+diag(MatchedCallExpr->getExprLoc(), "Unused result from error function %0")

tmroeder wrote:
> aaron.ballman wrote:
> > Is the presence of the attribute actually important, or is it simply 
> > expected that the declarations will have the attribute and so this check is 
> > benign?
> The latter. But I think I could remove it; it seems unlikely that the 
> attribute will be removed from these functions any time, though it could be 
> disabled. It gets set in include/linux/compiler-gcc.h because clang sets the 
> macros __GNUC__ and __GNUC_MINOR__ and __GNUC_PATCHLEVEL__ greater than 3, 4, 
> and 0, respectively.
I'd probably remove it -- the check is useful even if the function is not 
marked with the attribute, but if the function is expected to be marked with 
the attribute in all circumstances anyway, then this is just doing needless 
work.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:51
+  }
+}
+

tmroeder wrote:
> aaron.ballman wrote:
> > Is there a way for the user to explicitly disable this warning, or is there 
> > no such thing as a false positive as far as the kernel is concerned?
> > 
> > Should the diagnostic also produce a fix-it, or is there not a sufficiently 
> > common replacement pattern to be used?
> The basic case (are these error functions being used in any way at all?) is 
> an error and should be fixed. As noted in response to a comment above, the 
> check in that case is so naive that anything it catches is wrong.
> 
> With respect to not using the result from functions that return this value, I 
> think the same argument applies: if code calls a function that returns an 
> error like this and literally ignores the result, then clang-tidy should flag 
> it. However, I don't know of a tool that currently checks the kernel for this 
> kind of transitive property with respect to these functions, so it's possible 
> that there are errors like that in the kernel (or idioms that I don't know 
> about).
> 
> I thought that the way that you turn off clang-tidy checks is by specifying 
> them at the command line with a minus sign in front of them: 
> -checks=-linuxkernel-must-check-errs.
> 
> Or do you mean locally turning it off? In that case, you can just use the 
> result of the function in any trivial way, like casting it to void.
> 
> With respect to the fixit; I thought about that, but I'm not sure I know what 
> the right fixit should be. I'd like to leave it without a fixit for now and 
> come back to it later if it becomes clear what to do here.
> The basic case (are these error functions being used in any way at all?) is 
> an error and should be fixed. As noted in response to a comment above, the 
> check in that case is so naive that anything it catches is wrong.

That sounds great to me.

> Or do you mean locally turning it off? In that case, you can just use the 
> result of the function in any trivial way, like casting it to void.

That's mostly what I was trying to understand. We usually want checks to have 
some way to be silenced locally (cast to void, NOLINT comments, whatever makes 
sense for the construct), though it sounds like your situation probably doesn't 
need it because every instance caught is truly a bug.

> With respect to the fixit; I thought about that, but I'm not sure I know what 
> the right fixit should be. I'd like to leave it without a fixit for now and 
> come back to it later if it becomes clear what to do here.

Sounds fine to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-23 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst:10
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+

tmroeder wrote:
> gribozavr wrote:
> > IIRC it is possible to pass through compiler warnings through ClangTidy... 
> > WDYT about that instead of reimplementing the warning?
> > 
> > However we would lose the ability to "infer" `warn_unused_result` on 
> > functions that return `ERR_PTR`.  However, since the analysis is not 
> > cross-translation-unit, IDK how much value there is.
> I don't know exactly how to pass the warnings through, but I'd be interested 
> in learning how to do that. I agree that that would be cleaner than this 
> (partial) reimplementation.
> 
> Note that my code above does do something like that: I currently check that 
> the unused-result attribute is set on the return from the call.
> I don't know exactly how to pass the warnings through, but I'd be interested 
> in learning how to do that. I agree that that would be cleaner than this 
> (partial) reimplementation.

It seems like it is done by default, and `-Wunused-result` is enabled by 
default:

```
$ cat /tmp/example.cpp
int __attribute__((warn_unused_result)) foo();

void bar() {
  foo();
}
$ ./bin/clang-tidy /tmp/example.cpp
1 warning generated.
/tmp/example.cpp:4:3: warning: ignoring return value of function declared with 
'warn_unused_result' attribute [clang-diagnostic-unused-result]
  foo();
  ^
```

If it does not work for you, you can enable it on the command line:

```
clang-tidy -extra-arg=-Wunused-result -checks=clang-diagnostic-unused-result 
/tmp/example.cpp
```

> Note that my code above does do something like that: I currently check that 
> the unused-result attribute is set on the return from the call.

Yes, I was saying that we would lose the ability to do that.  However, is it 
that valuable?  The analysis in ClangTidy does not cross translation unit 
boundaries, so unless the caller and the callee are in the same file, this 
inference does not buy us much.

You could also use an existing check, `bugprone-unused-return-value`, without 
even requiring the function to be annotated with the attribute:

```
$ cat /tmp/example.cpp
int foo();

void bar() {
  foo();
}
$ ./bin/clang-tidy /tmp/example.cpp -config="{Checks: 
'bugprone-unused-return-value', CheckOptions: [{key: 
'bugprone-unused-return-value.CheckedFunctions', value: 'foo'}]}"
1 warning generated.
/tmp/example.cpp:4:3: warning: the value returned by this function should be 
used [bugprone-unused-return-value]
  foo();
  ^
/tmp/example.cpp:4:3: note: cast the expression to void to silence this warning
```


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-23 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder updated this revision to Diff 196273.
tmroeder added a comment.

Remove an unnecessary header and fix the error text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
  clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.h
  clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c

Index: clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s linuxkernel-must-check-errs %t
+
+#define __must_check __attribute__((warn_unused_result))
+
+// Prototypes of the error functions.
+void * __must_check ERR_PTR(long error);
+long  __must_check PTR_ERR(const void *ptr);
+int  __must_check IS_ERR(const void *ptr);
+int  __must_check IS_ERR_OR_NULL(const void *ptr);
+void * __must_check ERR_CAST(const void *ptr);
+int  __must_check PTR_ERR_OR_ZERO(const void *ptr);
+
+void f() {
+  ERR_PTR(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'ERR_PTR' is unused
+  PTR_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'PTR_ERR' is unused
+  IS_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'IS_ERR' is unused
+  ERR_CAST((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'ERR_CAST' is unused
+out:
+  PTR_ERR_OR_ZERO((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'PTR_ERR_OR_ZERO' is unused
+}
+
+void *f1() {
+  return ERR_PTR(0);
+}
+
+long f2() {
+  if (IS_ERR((void *)0)) {
+return PTR_ERR((void *)0);
+  }
+  return -1;
+}
+
+void f3() {
+  f1();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'f1' is unused but represents an error value
+  f2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result from function 'f2' is unused but represents an error value
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   linuxkernel-must-use-errs
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - linuxkernel-must-use-errs
+
+linuxkernel-must-use-errs
+=
+
+Checks for cases where the kernel error functions ``ERR_PTR``,
+``PTR_ERR``, ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and
+``PTR_ERR_OR_ZERO`` are called but the results are not used. These
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+
+This also checks for unused values returned by functions that return
+``ERR_PTR``.
+
+Examples:
+
+.. code-block:: c
+
+  /* Trivial unused call to an ERR function */
+  PTR_ERR_OR_ZERO(some_function_call());
+
+  /* A function that returns ERR_PTR. */
+  void *fn() { ERR_PTR(-EINVAL); }
+
+  /* An invalid use of fn. */
+  fn();
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -142,6 +142,13 @@
   ` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`linuxkernel-must-use-errs
+  ` check.
+
+  Checks Linux Kernel code to see if it uses the results from the functions in
+  linux/err.h. Also checks to see if code uses the results from functions that
+  directly return a value from one of these error functions.
+
 - New :doc:`openmp-exception-escape
   ` check.
 
Index: clang-tools-extra/clang-tidy/tool/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/tool/CMa

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-23 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked 5 inline comments as done.
tmroeder added a comment.

Thanks to everyone for the comments. I've answered them as best I can, and I'm 
definitely open to changes or to scrapping this entirely.

I should have prefixed this patch with a discussion on the main list, perhaps. 
My main use case for this clang-tidy module is twofold: find problems in 
existing kernel code and checking incoming patches to (some of the) kernel 
mailing lists. I don't expect all (or even most) kernel developers to use this, 
just like I don't think most kernel developers use existing checkers (sparse 
and smatch) that have to be explicitly enabled by build-time options in Kbuild.

You might ask why I would want to implement these checks in clang-tidy at all 
if there are already static-analysis tools like sparse and smatch, though I 
expect that this list is generally in favor of expanding the scope of 
clang-tidy. The answer is that I think that having these checks directly in the 
compiler is the right way to go; clang-tidy (and clang-check, where I also 
would like to add some static analysis for the kernel) provide a principled 
basis for checking C code rather than using custom C parsers for the kernel. 
And I really like the AST matcher language and think it provides a powerful 
tool for writing these checks.




Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:12
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include 
+

lebedev.ri wrote:
> This looks wrong
Yeah, I'm not sure where that came from. I'll remove it.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:24
+  "ERR_CAST", "PTR_ERR_OR_ZERO"));
+  auto NonCheckingStmts = stmt(anyOf(compoundStmt(), labelStmt()));
+  Finder->addMatcher(

lebedev.ri wrote:
> Are there any rules what kernel considers to be checking and not checking?
> This i think e.g. will accept cast-to-void, assignment to a variable and then 
> ignoring it, etc.
> 
Yes, this check is extremely simplistic. My understanding of clang-tidy checks 
was that the most value comes from them catching obviously wrong behavior and 
not having any false positives. I didn't think they were supposed to catch all 
the ways in which something could be wrong.

But I've never written a clang-tidy check before. What is their expected 
purpose?



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:39
+  if (MatchedCallExpr &&
+  MatchedCallExpr->hasUnusedResultAttr(*Result.Context)) {
+diag(MatchedCallExpr->getExprLoc(), "Unused result from error function %0")

aaron.ballman wrote:
> Is the presence of the attribute actually important, or is it simply expected 
> that the declarations will have the attribute and so this check is benign?
The latter. But I think I could remove it; it seems unlikely that the attribute 
will be removed from these functions any time, though it could be disabled. It 
gets set in include/linux/compiler-gcc.h because clang sets the macros __GNUC__ 
and __GNUC_MINOR__ and __GNUC_PATCHLEVEL__ greater than 3, 4, and 0, 
respectively.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:51
+  }
+}
+

aaron.ballman wrote:
> Is there a way for the user to explicitly disable this warning, or is there 
> no such thing as a false positive as far as the kernel is concerned?
> 
> Should the diagnostic also produce a fix-it, or is there not a sufficiently 
> common replacement pattern to be used?
The basic case (are these error functions being used in any way at all?) is an 
error and should be fixed. As noted in response to a comment above, the check 
in that case is so naive that anything it catches is wrong.

With respect to not using the result from functions that return this value, I 
think the same argument applies: if code calls a function that returns an error 
like this and literally ignores the result, then clang-tidy should flag it. 
However, I don't know of a tool that currently checks the kernel for this kind 
of transitive property with respect to these functions, so it's possible that 
there are errors like that in the kernel (or idioms that I don't know about).

I thought that the way that you turn off clang-tidy checks is by specifying 
them at the command line with a minus sign in front of them: 
-checks=-linuxkernel-must-check-errs.

Or do you mean locally turning it off? In that case, you can just use the 
result of the function in any trivial way, like casting it to void.

With respect to the fixit; I thought about that, but I'm not sure I know what 
the right fixit should be. I'd like to leave it without a fixit for now and 
come back to it later if it becomes clear what to do here.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst:10
+functions are mar

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-23 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst:10
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+

IIRC it is possible to pass through compiler warnings through ClangTidy... WDYT 
about that instead of reimplementing the warning?

However we would lose the ability to "infer" `warn_unused_result` on functions 
that return `ERR_PTR`.  However, since the analysis is not 
cross-translation-unit, IDK how much value there is.


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-23 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:39
+  if (MatchedCallExpr &&
+  MatchedCallExpr->hasUnusedResultAttr(*Result.Context)) {
+diag(MatchedCallExpr->getExprLoc(), "Unused result from error function %0")

Is the presence of the attribute actually important, or is it simply expected 
that the declarations will have the attribute and so this check is benign?



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:40
+  MatchedCallExpr->hasUnusedResultAttr(*Result.Context)) {
+diag(MatchedCallExpr->getExprLoc(), "Unused result from error function %0")
+<< MatchedCallExpr->getDirectCallee()->getNameAsString();

clang-tidy diagnostics are nongrammatical in the same way the compiler error 
messages are: don't capitalize the start of the sentence, no terminating 
punctuation, etc. I'd probably reword to `result from error function %0 is 
unused`



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:41
+diag(MatchedCallExpr->getExprLoc(), "Unused result from error function %0")
+<< MatchedCallExpr->getDirectCallee()->getNameAsString();
+  }

You should pass the result from `getDirectCallee()` rather than converting to a 
string. The diagnostics engine can handle any `NamedDecl *` automatically, 
including proper quoting, etc.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:48
+diag(MatchedTransitiveCallExpr->getExprLoc(),
+ "Unused result from function %0, which returns an error value")
+<< MatchedTransitiveCallExpr->getDirectCallee()->getNameAsString();

Similar comments here. How about `result from function %0 is unused but 
represents an error value`?



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:49
+ "Unused result from function %0, which returns an error value")
+<< MatchedTransitiveCallExpr->getDirectCallee()->getNameAsString();
+  }

Similar issue here as above.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:51
+  }
+}
+

Is there a way for the user to explicitly disable this warning, or is there no 
such thing as a false positive as far as the kernel is concerned?

Should the diagnostic also produce a fix-it, or is there not a sufficiently 
common replacement pattern to be used?


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-23 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added inline comments.



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:12
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include 
+

This looks wrong



Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:24
+  "ERR_CAST", "PTR_ERR_OR_ZERO"));
+  auto NonCheckingStmts = stmt(anyOf(compoundStmt(), labelStmt()));
+  Finder->addMatcher(

Are there any rules what kernel considers to be checking and not checking?
This i think e.g. will accept cast-to-void, assignment to a variable and then 
ignoring it, etc.



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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-22 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked an inline comment as done.
tmroeder added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:148
+
+  FIXME: add release notes.
+

Eugene.Zelenko wrote:
> Please add short description. Should be same as first statements in 
> documentation.
Please take a look. I've rewritten the initial documentation lines in the check 
header and used that text here, as suggested.


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-22 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder updated this revision to Diff 196151.
tmroeder added a comment.

Fix a line-length issue in the check code and rewrite the doc text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
  clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.h
  clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c

Index: clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s linuxkernel-must-check-errs %t
+
+#define __must_check __attribute__((warn_unused_result))
+
+// Prototypes of the error functions.
+void * __must_check ERR_PTR(long error);
+long  __must_check PTR_ERR(const void *ptr);
+int  __must_check IS_ERR(const void *ptr);
+int  __must_check IS_ERR_OR_NULL(const void *ptr);
+void * __must_check ERR_CAST(const void *ptr);
+int  __must_check PTR_ERR_OR_ZERO(const void *ptr);
+
+void f() {
+  ERR_PTR(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function ERR_PTR
+  PTR_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function PTR_ERR
+  IS_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function IS_ERR
+  ERR_CAST((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function ERR_CAST
+out:
+  PTR_ERR_OR_ZERO((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function PTR_ERR_OR_ZERO
+}
+
+void *f1() {
+  return ERR_PTR(0);
+}
+
+long f2() {
+  if (IS_ERR((void *)0)) {
+return PTR_ERR((void *)0);
+  }
+  return -1;
+}
+
+void f3() {
+  f1();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from function f1, which returns an error value
+  f2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from function f2, which returns an error value
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   linuxkernel-must-use-errs
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - linuxkernel-must-use-errs
+
+linuxkernel-must-use-errs
+=
+
+Checks for cases where the kernel error functions ``ERR_PTR``,
+``PTR_ERR``, ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and
+``PTR_ERR_OR_ZERO`` are called but the results are not used. These
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+
+This also checks for unused values returned by functions that return
+``ERR_PTR``.
+
+Examples:
+
+.. code-block:: c
+
+  /* Trivial unused call to an ERR function */
+  PTR_ERR_OR_ZERO(some_function_call());
+
+  /* A function that returns ERR_PTR. */
+  void *fn() { ERR_PTR(-EINVAL); }
+
+  /* An invalid use of fn. */
+  fn();
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -142,6 +142,13 @@
   ` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`linuxkernel-must-use-errs
+  ` check.
+
+  Checks Linux Kernel code to see if it uses the results from the functions in
+  linux/err.h. Also checks to see if code uses the results from functions that
+  directly return a value from one of these error functions.
+
 - New :doc:`openmp-exception-escape
   ` check.
 
Index: clang-tools-extra/clang-tidy/tool/CMakeLists.txt
===
--- clang-tools-extra/clang-tid

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-22 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder updated this revision to Diff 196150.
tmroeder added a comment.

Actually add the documentation in the release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
  clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.h
  clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c

Index: clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s linuxkernel-must-check-errs %t
+
+#define __must_check __attribute__((warn_unused_result))
+
+// Prototypes of the error functions.
+void * __must_check ERR_PTR(long error);
+long  __must_check PTR_ERR(const void *ptr);
+int  __must_check IS_ERR(const void *ptr);
+int  __must_check IS_ERR_OR_NULL(const void *ptr);
+void * __must_check ERR_CAST(const void *ptr);
+int  __must_check PTR_ERR_OR_ZERO(const void *ptr);
+
+void f() {
+  ERR_PTR(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function ERR_PTR
+  PTR_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function PTR_ERR
+  IS_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function IS_ERR
+  ERR_CAST((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function ERR_CAST
+out:
+  PTR_ERR_OR_ZERO((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function PTR_ERR_OR_ZERO
+}
+
+void *f1() {
+  return ERR_PTR(0);
+}
+
+long f2() {
+  if (IS_ERR((void *)0)) {
+return PTR_ERR((void *)0);
+  }
+  return -1;
+}
+
+void f3() {
+  f1();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from function f1, which returns an error value
+  f2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from function f2, which returns an error value
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   linuxkernel-must-use-errs
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - linuxkernel-must-use-errs
+
+linuxkernel-must-use-errs
+=
+
+Checks for cases where the kernel error functions ``ERR_PTR``,
+``PTR_ERR``, ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and
+``PTR_ERR_OR_ZERO`` are called but the results are not used. These
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+
+This also checks for unused values returned by functions that return
+``ERR_PTR``.
+
+Examples:
+
+.. code-block:: c
+
+  /* Trivial unused call to an ERR function */
+  PTR_ERR_OR_ZERO(some_function_call());
+
+  /* A function that returns ERR_PTR. */
+  void *fn() { ERR_PTR(-EINVAL); }
+
+  /* An invalid use of fn. */
+  fn();
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -142,6 +142,11 @@
   ` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`linuxkernel-must-use-errs
+  ` check.
+
+  Checks Linux Kernel code for misuse of important error functions.
+
 - New :doc:`openmp-exception-escape
   ` check.
 
Index: clang-tools-extra/clang-tidy/tool/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/tool/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/tool/CMakeLists.txt
@@ -26,6 +26,7 @@
   clangTidyFuchsiaModule
   clangTidyGoogleModule
   clangTidyHICPPModule

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-22 Thread Eugene Zelenko via Phabricator via cfe-commits 
Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:148
+
+  FIXME: add release notes.
+

Please add short description. Should be same as first statements in 
documentation.


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-22 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder updated this revision to Diff 196148.
tmroeder marked an inline comment as done.
tmroeder added a comment.

Updated with an initial check.

Verified that add_new_check.py works (that's how I added the check). Also added 
some basic docs and a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
  clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
  clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.h
  clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c

Index: clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s linuxkernel-must-check-errs %t
+
+#define __must_check __attribute__((warn_unused_result))
+
+// Prototypes of the error functions.
+void * __must_check ERR_PTR(long error);
+long  __must_check PTR_ERR(const void *ptr);
+int  __must_check IS_ERR(const void *ptr);
+int  __must_check IS_ERR_OR_NULL(const void *ptr);
+void * __must_check ERR_CAST(const void *ptr);
+int  __must_check PTR_ERR_OR_ZERO(const void *ptr);
+
+void f() {
+  ERR_PTR(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function ERR_PTR
+  PTR_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function PTR_ERR
+  IS_ERR((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function IS_ERR
+  ERR_CAST((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function ERR_CAST
+out:
+  PTR_ERR_OR_ZERO((void *)0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from error function PTR_ERR_OR_ZERO
+}
+
+void *f1() {
+  return ERR_PTR(0);
+}
+
+long f2() {
+  if (IS_ERR((void *)0)) {
+return PTR_ERR((void *)0);
+  }
+  return -1;
+}
+
+void f3() {
+  f1();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from function f1, which returns an error value
+  f2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unused result from function f2, which returns an error value
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   linuxkernel-must-use-errs
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - linuxkernel-must-use-errs
+
+linuxkernel-must-use-errs
+=
+
+Checks for cases where the kernel error functions ``ERR_PTR``,
+``PTR_ERR``, ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and
+``PTR_ERR_OR_ZERO`` are called but the results are not used. These
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+
+This also checks for unused values returned by functions that return
+``ERR_PTR``.
+
+Examples:
+
+.. code-block:: c
+
+  /* Trivial unused call to an ERR function */
+  PTR_ERR_OR_ZERO(some_function_call());
+
+  /* A function that returns ERR_PTR. */
+  void *fn() { ERR_PTR(-EINVAL); }
+
+  /* An invalid use of fn. */
+  fn();
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -142,6 +142,11 @@
   ` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`linuxkernel-must-use-errs
+  ` check.
+
+  FIXME: add release notes.
+
 - New :doc:`openmp-exception-escape
   ` check.
 
Index: clang-tools-extra/clang-tidy/tool/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/tool/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/tool/CMakeLists.txt
@@

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-04-03 Thread Alexander Kornienko via Phabricator via cfe-commits 
alexfh added a comment.

Can you verify that the add_new_check.py script works fine with this new module?
Thanks!


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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-03-29 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder updated this revision to Diff 192860.
tmroeder added a comment.

Changed the module name to linuxkernel as suggested and updated the files to 
match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
  clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
  clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt

Index: clang-tools-extra/clang-tidy/tool/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/tool/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/tool/CMakeLists.txt
@@ -26,6 +26,7 @@
   clangTidyFuchsiaModule
   clangTidyGoogleModule
   clangTidyHICPPModule
+  clangTidyLinuxKernelModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
Index: clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
@@ -17,6 +17,7 @@
   clangTidyFuchsiaModule
   clangTidyGoogleModule
   clangTidyHICPPModule
+  clangTidyLinuxKernelModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
Index: clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
@@ -0,0 +1,34 @@
+//===--- LinuxKernelTidyModule.cpp - clang-tidy===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+
+namespace clang {
+namespace tidy {
+namespace linux {
+
+/// This module is for checks specific to the Linux kernel.
+class LinuxKernelModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+  }
+};
+// Register the LinuxKernelTidyModule using this statically initialized
+// variable.
+static ClangTidyModuleRegistry::Add
+X("linux-module", "Adds checks specific to the Linux kernel.");
+} // namespace linux
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the LinuxKernelModule.
+volatile int LinuxKernelModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
@@ -0,0 +1,13 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyLinuxKernelModule
+  LinuxKernelTidyModule.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )
Index: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
===
--- clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
+++ clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
@@ -35,6 +35,11 @@
 static int LLVM_ATTRIBUTE_UNUSED BugproneModuleAnchorDestination =
 BugproneModuleAnchorSource;
 
+// This anchor is used to force the linker to link the LinuxKernelModule.
+extern volatile int LinuxKernelModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED LinuxKernelModuleAnchorDestination =
+LinuxKernelModuleAnchorSource;
+
 // This anchor is used to force the linker to link the LLVMModule.
 extern volatile int LLVMModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED LLVMModuleAnchorDestination =
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -44,6 +44,7 @@
 add_subdirectory(fuchsia)
 add_subdirectory(google)
 add_subdirectory(hicpp)
+add_subdirectory(linuxkernel)
 add_subdirectory(llvm)
 add_subdirectory(misc)
 add_subdirectory(modernize)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

2019-03-29 Thread Tom Roeder via Phabricator via cfe-commits 
tmroeder marked 2 inline comments as done.
tmroeder added a comment.

In D59963#1447985 , @alexfh wrote:

> Looks reasonable in general, but we usually add modules with at least one 
> check. Let's do the same here.


OK, will do.




Comment at: clang-tools-extra/clang-tidy/linux/LinuxTidyModule.cpp:13
+
+using namespace clang::ast_matchers;
+

lebedev.ri wrote:
> You don't need this here.
Thanks. I've removed it now.


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