aaron.ballman created this revision.
aaron.ballman added reviewers: alexfh, klimek.
aaron.ballman added a subscriber: cfe-commits.
Some object types are not meant to be used as a value type in all
circumstances, but the language (mainly C) does not provide facilities for
marking these types as noncopyable. For instance, you should never pass a FILE
object by value, but only by pointer. This seems to fall into two category of
types:
A FILE type should never be declared by value (whether as a local, a field, or
a parameter) and such a pointer should never be dereferenced.
Some POSIX types, like pthread_mutex_t can be declared by value as a variable
or field, but should not be declared by value as a parameter, and such a
pointer should never be dereferenced.
This patch adds a checker (misc-non-copyable-objects, but a better moniker
would not make me sad) to catch code that does this accidentally. This
corresponds (at least for treatment of FILE) to:
https://www.securecoding.cert.org/confluence/display/c/FIO38-C.+Do+not+copy+a+FILE+object
~Aaron
http://reviews.llvm.org/D12945
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/NonCopyableObjects.cpp
clang-tidy/misc/NonCopyableObjects.h
test/clang-tidy/misc-non-copyable-objects.c
test/clang-tidy/misc-non-copyable-objects.cpp
Index: test/clang-tidy/misc-non-copyable-objects.cpp
===================================================================
--- test/clang-tidy/misc-non-copyable-objects.cpp
+++ test/clang-tidy/misc-non-copyable-objects.cpp
@@ -0,0 +1,26 @@
+// RUN: %python %S/check_clang_tidy.py %s misc-non-copyable-objects %t
+
+namespace std {
+typedef struct FILE {} FILE;
+}
+using namespace std;
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: 'f' declared as type 'FILE'; did you mean 'FILE *'?
+void g(std::FILE f);
+
+struct S {
+ // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: 'f' declared as type 'FILE'; did you mean 'FILE *'?
+ ::FILE f;
+};
+
+void func(FILE *f) {
+ // CHECK-MESSAGES: :[[@LINE+1]]:13: warning: 'f1' declared as type 'FILE'; did you mean 'FILE *'?
+ std::FILE f1; // match
+ // CHECK-MESSAGES: :[[@LINE+2]]:10: warning: 'f2' declared as type 'FILE'; did you mean 'FILE *'?
+ // CHECK-MESSAGES: :[[@LINE+1]]:15: warning: expression has suspicious type 'FILE'
+ ::FILE f2 = *f; // match, match
+ // CHECK-MESSAGES: :[[@LINE+1]]:15: warning: 'f3' declared as type 'FILE'; did you mean 'FILE *'?
+ struct FILE f3; // match
+ // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: expression has suspicious type 'FILE'
+ (void)sizeof(*f); // match
+}
Index: test/clang-tidy/misc-non-copyable-objects.c
===================================================================
--- test/clang-tidy/misc-non-copyable-objects.c
+++ test/clang-tidy/misc-non-copyable-objects.c
@@ -0,0 +1,43 @@
+// RUN: %python %S/check_clang_tidy.py %s misc-non-copyable-objects %t
+
+typedef struct FILE {} FILE;
+typedef struct pthread_cond_t {} pthread_cond_t;
+typedef int pthread_mutex_t;
+
+// CHECK-MESSAGES: :[[@LINE+1]]:13: warning: 'f' declared as type 'FILE'; did you mean 'FILE *'?
+void g(FILE f);
+// CHECK-MESSAGES: :[[@LINE+1]]:24: warning: 'm' declared as type 'pthread_mutex_t'; did you mean 'pthread_mutex_t *'?
+void h(pthread_mutex_t m);
+// CHECK-MESSAGES: :[[@LINE+1]]:23: warning: 'c' declared as type 'pthread_cond_t'; did you mean 'pthread_cond_t *'?
+void i(pthread_cond_t c);
+
+struct S {
+ pthread_cond_t c; // ok
+ // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: 'f' declared as type 'FILE'; did you mean 'FILE *'?
+ FILE f;
+};
+
+void func(FILE *f) {
+ // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: 'f1' declared as type 'FILE'; did you mean 'FILE *'?
+ FILE f1; // match
+ // CHECK-MESSAGES: :[[@LINE+2]]:8: warning: 'f2' declared as type 'FILE'; did you mean 'FILE *'?
+ // CHECK-MESSAGES: :[[@LINE+1]]:13: warning: expression has suspicious type 'FILE'
+ FILE f2 = *f;
+ // CHECK-MESSAGES: :[[@LINE+1]]:15: warning: 'f3' declared as type 'FILE'; did you mean 'FILE *'?
+ struct FILE f3;
+ // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: expression has suspicious type 'FILE'
+ (void)sizeof(*f);
+ (void)sizeof(FILE);
+ // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: expression has suspicious type 'FILE'
+ g(*f);
+
+ pthread_mutex_t m; // ok
+ h(m); // ok
+
+ pthread_cond_t c; // ok
+ i(c); // ok
+
+ pthread_mutex_t *m1 = &m; // ok
+ // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: expression has suspicious type 'pthread_mutex_t'
+ h(*m1);
+}
Index: clang-tidy/misc/NonCopyableObjects.h
===================================================================
--- clang-tidy/misc/NonCopyableObjects.h
+++ clang-tidy/misc/NonCopyableObjects.h
@@ -0,0 +1,31 @@
+//===--- NonCopyableObjects.h - clang-tidy-----------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONCOPYABLEOBJECTS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONCOPYABLEOBJECTS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// The check flags dereferences and non-pointer declarations of objects that
+/// are not meant to be passed by value, such as C FILE objects.
+class NonCopyableObjectsCheck : public ClangTidyCheck {
+public:
+ NonCopyableObjectsCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONCOPYABLEOBJECTS_H
Index: clang-tidy/misc/NonCopyableObjects.cpp
===================================================================
--- clang-tidy/misc/NonCopyableObjects.cpp
+++ clang-tidy/misc/NonCopyableObjects.cpp
@@ -0,0 +1,90 @@
+//===--- NonCopyableObjects.cpp - clang-tidy-------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NonCopyableObjects.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace {
+bool isPOSIXTypeName(StringRef ClassName) {
+ static const char *TypeNames[] = {
+ "::pthread_cond_t",
+ "::pthread_mutex_t",
+ "pthread_cond_t",
+ "pthread_mutex_t"
+ };
+ return std::binary_search(std::begin(TypeNames), std::end(TypeNames),
+ ClassName);
+}
+
+bool isFILETypeName(StringRef ClassName) {
+ static const char *TypeNames[] = {
+ "::FILE",
+ "FILE",
+ "std::FILE"
+ };
+ return std::binary_search(std::begin(TypeNames), std::end(TypeNames),
+ ClassName);
+}
+
+AST_MATCHER(NamedDecl, isFILEType) {
+ return isFILETypeName(Node.getQualifiedNameAsString());
+}
+
+AST_MATCHER(NamedDecl, isPOSIXType) {
+ return isPOSIXTypeName(Node.getQualifiedNameAsString());
+}
+} // namespace
+
+namespace tidy {
+void NonCopyableObjectsCheck::registerMatchers(MatchFinder *Finder) {
+ // There are two ways to get into trouble with objects like FILE *:
+ // dereferencing the pointer type to be a non-pointer type, and declaring
+ // the type as a non-pointer type in the first place. While the declaration
+ // itself could technically be well-formed in the case where the type is not
+ // an opaque type, it's highly suspicious behavior.
+ //
+ // POSIX types are a bit different in that it's reasonable to declare a
+ // non-pointer variable or data member of the type, but it is not reasonable
+ // to dereference a pointer to the type, or declare a parameter of non-pointer
+ // type.
+ auto BadFILEType = hasType(namedDecl(isFILEType()).bind("type_decl"));
+ auto BadPOSIXType = hasType(namedDecl(isPOSIXType()).bind("type_decl"));
+ auto BadEitherType = anyOf(BadFILEType, BadPOSIXType);
+
+ Finder->addMatcher(
+ namedDecl(anyOf(varDecl(BadFILEType), fieldDecl(BadFILEType)))
+ .bind("decl"),
+ this);
+ Finder->addMatcher(parmVarDecl(BadPOSIXType).bind("decl"), this);
+ Finder->addMatcher(
+ expr(unaryOperator(hasOperatorName("*"), BadEitherType)).bind("expr"),
+ this);
+}
+
+void NonCopyableObjectsCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *D = Result.Nodes.getNodeAs<NamedDecl>("decl");
+ const auto *BD = Result.Nodes.getNodeAs<NamedDecl>("type_decl");
+ const auto *E = Result.Nodes.getNodeAs<Expr>("expr");
+
+ if (D && BD)
+ diag(D->getLocation(), "'%0' declared as type '%1'; did you mean '%1 *'?")
+ << D->getName() << BD->getName();
+ else if (E)
+ diag(E->getExprLoc(), "expression has suspicious type '%0'")
+ << BD->getName();
+}
+
+} // namespace tidy
+} // namespace clang
+
Index: clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -20,6 +20,7 @@
#include "MacroRepeatedSideEffectsCheck.h"
#include "MoveConstructorInitCheck.h"
#include "NoexceptMoveConstructorCheck.h"
+#include "NonCopyableObjects.h"
#include "SizeofContainerCheck.h"
#include "StaticAssertCheck.h"
#include "SwappedArgumentsCheck.h"
@@ -55,8 +56,9 @@
"misc-move-constructor-init");
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
"misc-noexcept-move-constructor");
- CheckFactories.registerCheck<SizeofContainerCheck>(
- "misc-sizeof-container");
+ CheckFactories.registerCheck<NonCopyableObjectsCheck>(
+ "misc-non-copyable-objects");
+ CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
CheckFactories.registerCheck<StaticAssertCheck>(
"misc-static-assert");
CheckFactories.registerCheck<SwappedArgumentsCheck>(
Index: clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tidy/misc/CMakeLists.txt
+++ clang-tidy/misc/CMakeLists.txt
@@ -12,6 +12,7 @@
MiscTidyModule.cpp
MoveConstructorInitCheck.cpp
NoexceptMoveConstructorCheck.cpp
+ NonCopyableObjects.cpp
SizeofContainerCheck.cpp
StaticAssertCheck.cpp
SwappedArgumentsCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
