[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:279-282
+  llvm::DenseSet result;
+  result.insert(std::begin(posixFunctions), std::end(posixFunctions));
+  result.insert(std::begin(glibcFunctions), std::end(glibcFunctions));
+  return {result.begin(), result.end()};

njames93 wrote:
> Would it not be faster to get rid of the set. Instead insert all the items in 
> to a vector, then sort and unique it
I can't imagine this will have measurable performance difference.
The main problem is algorithmic, https://reviews.llvm.org/D90944#inline-853246

But my main point is 
https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors


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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Also can this be ran through clang-tidy, feeling a few naming violations are in 
here. If you use arc to upload your patches you'll get lint warnings about 
clang-tidy and clang-format.




Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:279-282
+  llvm::DenseSet result;
+  result.insert(std::begin(posixFunctions), std::end(posixFunctions));
+  result.insert(std::begin(glibcFunctions), std::end(glibcFunctions));
+  return {result.begin(), result.end()};

Would it not be faster to get rid of the set. Instead insert all the items in 
to a vector, then sort and unique it


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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:295
+  case MtUnsafeCheck::LibcType::Any:
+return hasAnyName(anyFunctions);
+  }

lebedev.ri wrote:
> return anyOf(hasAnyName(posixFunctions), hasAnyName(glibcFunctions));
It would be a bit slower than now - the current code removes duplicates. 
If/when other FunctionSets are added, duplicate list will be much bigger.



Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:309-310
+void MtUnsafeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(callExpr(callee(functionDecl(hasAnyMtUnsafeNames(Libc
+ .bind("mt-unsafe"),
+ this);

lebedev.ri wrote:
> Is there any way to invert the direction of this matcher,
> instead of checking each call that it's callee isn't one of the bad ones,
> look through all function decls, and for all the bad ones, diagnose all calls 
> to them?
I'm not sure it is possible without additional costs - it would require somehow 
marking bad FunctionDecl, it requires an addition of state.

I'm not an expert in LLVM AST, maybe it is very cheap and easy to do, but I 
don't see how, do you?


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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

`Libc` option name doesn't really make sense to me, maybe `FunctionSet` would 
fit better.




Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:32-33
+
+// Initial list was extracted from gcc documentation
+static const StringRef glibcFunctions[] = {
+"::argp_error",

I would expect this can be outside of all the namespaces right after `using 
namespace clang::ast_matchers;`



Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:287-288
+
+static ast_matchers::internal::Matcher
+hasAnyMtUnsafeNames(MtUnsafeCheck::LibcType libc) {
+  switch (libc) {

Likewise, i would expect this can be outside of namespaces



Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:295
+  case MtUnsafeCheck::LibcType::Any:
+return hasAnyName(anyFunctions);
+  }

return anyOf(hasAnyName(posixFunctions), hasAnyName(glibcFunctions));



Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:309-310
+void MtUnsafeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(callExpr(callee(functionDecl(hasAnyMtUnsafeNames(Libc
+ .bind("mt-unsafe"),
+ this);

Is there any way to invert the direction of this matcher,
instead of checking each call that it's callee isn't one of the bad ones,
look through all function decls, and for all the bad ones, diagnose all calls 
to them?



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:111
+
+  Finds thread-unsafe functions usage.
+

<...>, Currently knows about POSIX and GLIBC.

or something along those lines


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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Just a general drive by point, is it wise to add a new tidy module to 
clang-tidy called `threads` (or similar). 
We have got a few other checks related to multi-threaded code in the pipeline 
(D77493 , D75229 
) that would fit perfectly for the same module.
Not to mention it would make it a lot easier to find checks specifically 
designed for multi-threaded workloads.


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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon added a comment.

In D90944#2380910 , @lebedev.ri wrote:

> What i would however like to be improved, is better docs.

I hope I'll addressed your questions in documentation. Please tell me whether 
you still have any unanswered questions.


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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon updated this revision to Diff 305460.
segoon added a comment.

- minor changes to docs


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

https://reviews.llvm.org/D90944

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t -- -config='{CheckOptions: [{key: "misc-mt-unsafe.Libc", value: "posix"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  ::sleep(2);
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [misc-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t -- -config='{CheckOptions: [{key: "misc-mt-unsafe.Libc", value: "glibc"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  dirname(nullptr);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [misc-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - misc-mt-unsafe
+
+misc-mt-unsafe
+==
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe functions. Usually they access static variables without
+synchronization (e.g. gmtime(3)) or utilize signals in a racy way.
+The set of functions to check is specified with the `Libc` option.
+
+Examples:
+
+.. code-block:: c++
+
+tm = gmtime(timep); // uses a global buffer
+
+sleep(1); // implementation may use SIGALRM
+
+.. option:: Libc
+
+  Specifies which functions in libc should be considered thread-safe,
+  possible values are `posix`, `glibc`, or `any`.
+
+  `posix` means POSIX defined thread-unsafe functions. POSIX.1-2001
+  in "2.9.1 Thread-Safety" defines that all functions specified in the
+  standard are thread-safe except a predefined list of thread-unsafe
+  functions.
+
+  Glibc defines some of them as thread-safe (e.g. dirname(3)), but adds
+  non-POSIX thread-unsafe ones (e.g. getopt_long(3)). Glibc's list is
+  compiled from GNU web documentation wi

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon updated this revision to Diff 304189.
segoon edited the summary of this revision.
segoon added a comment.

- add `Libc` option
- improve docs


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

https://reviews.llvm.org/D90944

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t -- -config='{CheckOptions: [{key: "misc-mt-unsafe.Libc", value: "posix"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  ::sleep(2);
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [misc-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t -- -config='{CheckOptions: [{key: "misc-mt-unsafe.Libc", value: "glibc"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  dirname(nullptr);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [misc-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - misc-mt-unsafe
+
+misc-mt-unsafe
+==
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe functions. Usually they access static variables without
+synchronization (e.g. gmtime(3)) or utilize signals in a racy way.
+The set of functions to check is specified with the `Libc` option.
+
+Examples:
+
+.. code-block:: c++
+
+tm = gmtime(timep); // uses a global buffer
+
+sleep(1); // implementation may use SIGALRM
+
+.. option:: Libc
+
+  Specifies which functions in libc should be considered thread-safe,
+  possible values are `posix`, `glibc`, or `any`.
+  POSIX.1-2001 in "2.9.1 Thread-Safety" defines that all functions
+  specified in the standard are thread-safe except a predefined list of
+  thread-unsafe functions.
+  Glibc defines some of them as thread-safe (e.g. dirname(3)), but adds
+  non-POSIX thread-unsafe ones (e.g. getopt_long(3)).
+  If you want to identify thread-unsafe API for at least one 

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

While i share concern about false-positives, it is literally impossible to 
avoid them here,
and this should be viewed more as an enforcement tool (don't use thread-unsafe 
fns),
not bug-detection check.

What i would however like to be improved, is better docs.
I'm guessing `mt-unsafe` is MT-Unsafe from 
https://www.gnu.org/software/libc/manual/html_node/POSIX-Safety-Concepts.html ?
Does this fully cover all glibc functions? POSIX? Etc?


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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon updated this revision to Diff 303532.
segoon added a comment.

- use static instead of namespace {}
- don't use SourceRange()
- revert unrelated changes to .rst


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

https://reviews.llvm.org/D90944

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - misc-mt-unsafe
+
+misc-mt-unsafe
+==
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe functions. Usually they access static variables without
+synchronization (e.g. gmtime(3)) or utilize signals in a racy way.
+
+Examples:
+
+.. code-block:: c++
+
+tm = gmtime(timep); // uses a global buffer
+
+sleep(1); // implementation may use SIGALRM
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
@@ -198,6 +198,7 @@
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"
`misc-misplaced-const `_,
+   `misc-mt-unsafe `_,
`misc-new-delete-overloads `_,
`misc-no-recursion `_,
`misc-non-copyable-objects `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,10 @@
   Finds condition variables in nested ``if`` statements that were also checked
   in the outer ``if`` statement and were not changed.
 
+- New :doc:`misc-mt-unsafe ` check.
+
+  Finds thread-unsafe functions usage.
+
 - New :doc:`readability-function-cognitive-complexity
   ` check.
 
Index: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
@@ -0,0 +1,34 @@
+//===--- MtUnsafeCheck.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_MISC_MTUNSAFECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MTUNSAFECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Checks that non-thread-safe functions are not used.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-mt-unsafe.html
+class MtUnsafeCheck : public ClangTidyCheck {
+public:
+  MtUnsafeCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MTUNSAFECHECK_H
Index: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp
@@ -0,0 +1,199 @@
+//===--- MtUnsafeCheck.cpp - clang-tidy ---===//
+//
+// Part of the LLVM Project, under the Apache Lice

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon added a comment.

In D90944#2379688 , @njames93 wrote:

> It appears to not check for signs that the code is running in a multi 
> threaded manner, This will result in many false positives in code that is 
> known to be single threaded.

I'm not sure there is a trustworthy check whether a source is going to be used 
in MT environment. A program can be linked with threads libraries but still use 
a single thread and use MT-unsafe API with no problem. I'd rather disable the 
check by default and let the user explicitly enable the check. I'm not sure 
what's clang-tidy policy for such case - should I create a new module instead 
of "misc"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:33
`abseil-upgrade-duration-conversions 
`_, "Yes"
-   `altera-struct-pack-align `_,
+   `altera-struct-pack-align `_, "Yes"
`android-cloexec-accept `_, "Yes"

segoon wrote:
> njames93 wrote:
> > Can you undo this change and all other unrelated changes to this file.
> These changes are added by ./add_new_check.py. I guess the file was altered 
> by hands w/o using the script the last time(s).
I'm aware of that. but these changes are unrelated and shouldn't be a part of 
the PR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon marked an inline comment as done.
segoon added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:33
`abseil-upgrade-duration-conversions 
`_, "Yes"
-   `altera-struct-pack-align `_,
+   `altera-struct-pack-align `_, "Yes"
`android-cloexec-accept `_, "Yes"

njames93 wrote:
> Can you undo this change and all other unrelated changes to this file.
These changes are added by ./add_new_check.py. I guess the file was altered by 
hands w/o using the script the last time(s).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

It appears to not check for signs that the code is running in a multi threaded 
manner, This will result in many false positives in code that is known to be 
single threaded.




Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:19
+
+namespace {
+

Don't use an anonymous namespace, just make the decls contained static.



Comment at: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp:199
+  diag(Call->getBeginLoc(), "function is not thread safe")
+  << SourceRange(Call->getBeginLoc(), Call->getEndLoc());
+}

I don't think this range needs appending onto the diagnostic.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:33
`abseil-upgrade-duration-conversions 
`_, "Yes"
-   `altera-struct-pack-align `_,
+   `altera-struct-pack-align `_, "Yes"
`android-cloexec-accept `_, "Yes"

Can you undo this change and all other unrelated changes to this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon marked an inline comment as done.
segoon added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp:3-4
+
+#include 
+#include 
+

lebedev.ri wrote:
> Tests should be hermetic, they can not use headers from system
fixed


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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon updated this revision to Diff 303499.
segoon added a comment.

don't include any system headers in tests


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

https://reviews.llvm.org/D90944

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - misc-mt-unsafe
+
+misc-mt-unsafe
+==
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe functions. Usually they access static variables without
+synchronization (e.g. gmtime(3)) or utilize signals in a racy way.
+
+Examples:
+
+.. code-block:: c++
+
+tm = gmtime(timep); // uses a global buffer
+
+sleep(1); // implementation may use SIGALRM
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
@@ -30,7 +30,7 @@
`abseil-time-comparison `_, "Yes"
`abseil-time-subtraction `_, "Yes"
`abseil-upgrade-duration-conversions `_, "Yes"
-   `altera-struct-pack-align `_,
+   `altera-struct-pack-align `_, "Yes"
`android-cloexec-accept `_, "Yes"
`android-cloexec-accept4 `_,
`android-cloexec-creat `_, "Yes"
@@ -143,6 +143,7 @@
`cppcoreguidelines-narrowing-conversions `_,
`cppcoreguidelines-no-malloc `_,
`cppcoreguidelines-owning-memory `_,
+   `cppcoreguidelines-prefer-member-initializer `_, "Yes"
`cppcoreguidelines-pro-bounds-array-to-pointer-decay `_,
`cppcoreguidelines-pro-bounds-constant-array-index `_, "Yes"
`cppcoreguidelines-pro-bounds-pointer-arithmetic `_,
@@ -179,7 +180,6 @@
`google-readability-todo `_,
`google-runtime-int `_,
`google-runtime-operator `_,
-   `google-runtime-references `_,
`google-upgrade-googletest-case `_, "Yes"
`hicpp-avoid-goto `_,
`hicpp-exception-baseclass `_,
@@ -198,6 +198,7 @@
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"
`misc-misplaced-const `_,
+   `misc-mt-unsafe `_, "Yes"
`misc-new-delete-overloads `_,
`misc-no-recursion `_,
`misc-non-copyable-objects `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,10 @@
   Finds condition variables in nested ``if`` statements that were also checked
   in the outer ``if`` statement and were not changed.
 
+- New :doc:`misc-mt-unsafe ` check.
+
+  Finds thread-unsafe functions usage.
+
 - New :doc:`readability-function-cognitive-complexity
   ` check.
 
Index: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
@@ -0,0 +1,34 @@
+//===--- MtUnsafeCheck.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_MISC_MTUNSAFECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MTUNSAFECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Checks that non-thread-safe funct

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp:3-4
+
+#include 
+#include 
+

Tests should be hermetic, they can not use headers from system


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon created this revision.
segoon added a reviewer: alexfh.
Herald added subscribers: cfe-commits, lxfind, modocache, xazax.hun, mgorny.
Herald added a project: clang.
segoon requested review of this revision.

Checks for some thread-unsafe functions against a black list of 
known-to-be-unsafe functions. Usually they access static variables without 
synchronization (e.g. gmtime(3)) or utilize signals in a racy way (e.g. 
sleep(3)).

The patch adds a check instead of auto-fix as thread-safe alternatives usually 
have API with an additional argument (e.g. gmtime(3) v.s. gmtime_r(3)) or have 
a different semantics (e.g. exit(3) v.s. __exit(3)), so it is a rather tricky 
or non-expected fix.

The check is used in Yandex Taxi backend and has caught many unpleasant bugs. A 
similar patch for coroutine-unsafe API is coming next.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90944

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t
+
+#include 
+#include 
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  time_t tt{};
+  auto tm = gmtime(&tt);
+  // CHECK-MESSAGES: :[[@LINE-3]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(&tt);
+  // CHECK-MESSAGES: :[[@LINE-4]]:8: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - misc-mt-unsafe
+
+misc-mt-unsafe
+==
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe functions. Usually they access static variables without
+synchronization (e.g. gmtime(3)) or utilize signals in a racy way.
+
+Examples:
+
+.. code-block:: c++
+
+tm = gmtime(timep); // uses a global buffer
+
+sleep(1); // implementation may use SIGALRM
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
@@ -30,7 +30,7 @@
`abseil-time-comparison `_, "Yes"
`abseil-time-subtraction `_, "Yes"
`abseil-upgrade-duration-conversions `_, "Yes"
-   `altera-struct-pack-align `_,
+   `altera-struct-pack-align `_, "Yes"
`android-cloexec-accept `_, "Yes"
`android-cloexec-accept4 `_,
`android-cloexec-creat `_, "Yes"
@@ -143,6 +143,7 @@
`cppcoreguidelines-narrowing-conversions `_,
`cppcoreguidelines-no-malloc `_,
`cppcoreguidelines-owning-memory `_,
+   `cppcoreguidelines-prefer-member-initializer `_, "Yes"
`cppcoreguidelines-pro-bounds-array-to-pointer-decay `_,
`cppcoreguidelines-pro-bounds-constant-array-index `_, "Yes"
`cppcoreguidelines-pro-bounds-pointer-arithmetic `_,
@@ -179,7 +180,6 @@
`google-readability-todo `_,
`google-runtime-int `_,
`google-runtime-operator `_,
-   `google-runtime-references `_,
`google-upgrade-googletest-case `_, "Yes"
`hicpp-avoid-goto `_,
`hicpp-exception-baseclass `_,
@@ -198,6 +198,7 @@
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"
`misc-misplaced-const `_,
+   `misc-mt-unsafe `_, "Yes"
`misc-new-delete-overloads `_,
`misc-no-recursion `_,
`misc-non-copyable-objects `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,10 @@
   Finds condition variables in nested ``if`` statements that were also checked
   in the outer ``if`` statement and were not changed.
 
+- New :doc:`misc-mt-unsafe ` check.
+
+  Finds thread-unsafe functions usage.
+
 - New :doc:`readability-function-cognitive-complexity
   ` check.
 
Index: clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
@@ -0,0 +1,34 @@
+//=