aaron.ballman created this revision.
aaron.ballman added reviewers: klimek, alexfh.
aaron.ballman added a subscriber: cfe-commits.
When defining a free store function, it is almost invariably a bug to fail to
declare the corresponding free store function. For instance, implementing
operator new() but failing to implement operator delete(). This new checker
catches instances where the free store functions are mismatched, while still
allowing for common code patterns involving deleted functions or placement
forms of functions. Specifically, this will not warn on implicit free store
functions, placement forms, deleted functions (such as defining a placement new
and a deleted operator delete(void*)), and private functions (for code that
predates C++11).
This patch corresponds to:
https://www.securecoding.cert.org/confluence/display/cplusplus/DCL54-CPP.+Overload+allocation+and+deallocation+functions+as+a+pair+in+the+same+scope
I ran this checker over Clang and LLVM and it did find two issues, one of which
I have fixed. YAMLParser.h had a protected operator delete() with a trivial
definition that I converted to be a deleted function in r248320. This was a
borderline false positive -- the code was correct because the body of operator
delete() was trivial. However, defining the function as deleted is cleaner
since accidental deletion from within Node or one of its subclasses will now be
diagnosed instead of silently accepted and doing nothing. The other is a true
positive as best I can tell. The MDNode class in Metadata.h defines a placement
new operator, a free store operator delete(), and two placement delete
functions. The free store operator delete() has no matching free store operator
new() (including when looking in super classes), but the implementation does
not appear to trigger undefined behavior -- it just seems like a really
confused design (which is mitigated by the fact that the free store delete is
protected). Also, one of the placement delete functions is not required by std,
as the comments suggest, and should be removed.
~Aaron
http://reviews.llvm.org/D13071
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/NewDeleteOverloadsCheck.cpp
clang-tidy/misc/NewDeleteOverloadsCheck.h
test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp
test/clang-tidy/misc-new-delete-overloads.cpp
Index: test/clang-tidy/misc-new-delete-overloads.cpp
===================================================================
--- test/clang-tidy/misc-new-delete-overloads.cpp
+++ test/clang-tidy/misc-new-delete-overloads.cpp
@@ -0,0 +1,68 @@
+// RUN: %python %S/check_clang_tidy.py %s misc-new-delete-overloads %t -- -std=c++14
+
+typedef unsigned int size_t;
+
+struct S {
+ // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope
+ void *operator new(size_t size) noexcept;
+ // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new[]' has no matching declaration of 'operator delete[]' at the same scope
+ void *operator new[](size_t size) noexcept;
+};
+
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope
+void *operator new(size_t size) noexcept;
+
+struct T {
+ // Sized deallocations are not enabled by default, and so this new/delete pair
+ // does not match. However, we expect only one warning, for the new, because
+ // the operator delete is a placement delete and we do not warn on mismatching
+ // placement operations.
+ // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope
+ void *operator new(size_t size) noexcept;
+ void operator delete(void *ptr, size_t) noexcept; // ok only if sized deallocation is enabled
+};
+
+struct U {
+ void *operator new(size_t size) noexcept;
+ void operator delete(void *ptr) noexcept;
+
+ void *operator new[](size_t) noexcept;
+ void operator delete[](void *) noexcept;
+};
+
+struct Z {
+ // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: declaration of 'operator delete' has no matching declaration of 'operator new' at the same scope
+ void operator delete(void *ptr) noexcept;
+ // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: declaration of 'operator delete[]' has no matching declaration of 'operator new[]' at the same scope
+ void operator delete[](void *ptr) noexcept;
+};
+
+struct A {
+ void *operator new(size_t size, Z) noexcept; // ok, placement new
+};
+
+struct B {
+ void operator delete(void *ptr, A) noexcept; // ok, placement delete
+};
+
+// It is okay to have a class with an inaccessible free store operator.
+struct C {
+ void *operator new(size_t, A) noexcept; // ok, placement new
+private:
+ void operator delete(void *) noexcept;
+};
+
+// It is also okay to have a class with a delete free store operator.
+struct D {
+ void *operator new(size_t, A) noexcept; // ok, placement new
+ void operator delete(void *) noexcept = delete;
+};
+
+struct E : U {
+ void *operator new(size_t) noexcept; // okay, we inherit operator delete from U
+};
+
+struct F : S {
+ // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope
+ void *operator new(size_t) noexcept;
+};
Index: test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp
===================================================================
--- test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp
+++ test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp
@@ -0,0 +1,20 @@
+// RUN: %python %S/check_clang_tidy.py %s misc-new-delete-overloads %t -- -std=c++14 -fsized-deallocation
+
+typedef unsigned int size_t;
+
+struct S {
+ // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: declaration of 'operator delete' has no matching declaration of 'operator new' at the same scope
+ void operator delete(void *ptr, size_t) noexcept; // not a placement delete
+};
+
+struct T {
+ // Because we have enabled sized deallocations explicitly, this new/delete
+ // pair matches.
+ void *operator new(size_t size) noexcept;
+ void operator delete(void *ptr, size_t) noexcept; // ok because sized deallocation is enabled
+};
+
+// While we're here, check that global operator delete with no operator new
+// is also matched.
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: declaration of 'operator delete' has no matching declaration of 'operator new' at the same scope
+void operator delete(void *ptr) noexcept;
Index: clang-tidy/misc/NewDeleteOverloadsCheck.h
===================================================================
--- clang-tidy/misc/NewDeleteOverloadsCheck.h
+++ clang-tidy/misc/NewDeleteOverloadsCheck.h
@@ -0,0 +1,35 @@
+//===--- NewDeleteOverloadsCheck.h - clang-tidy----------------------------===//
+//
+// 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_NEWDELETEOVERLOADS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NEWDELETEOVERLOADS_H
+
+#include "../ClangTidy.h"
+#include "llvm/ADT/SmallVector.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+class NewDeleteOverloadsCheck : public ClangTidyCheck {
+ llvm::SmallVector<const class clang::FunctionDecl *, 2> Overloads;
+
+public:
+ NewDeleteOverloadsCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onEndOfTranslationUnit() override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NEWDELETEOVERLOADS_H
Index: clang-tidy/misc/NewDeleteOverloadsCheck.cpp
===================================================================
--- clang-tidy/misc/NewDeleteOverloadsCheck.cpp
+++ clang-tidy/misc/NewDeleteOverloadsCheck.cpp
@@ -0,0 +1,198 @@
+//===--- NewDeleteOverloadsCheck.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 "NewDeleteOverloadsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace {
+AST_MATCHER(FunctionDecl, isPlacementOverload) {
+ bool New;
+ switch (Node.getOverloadedOperator()) {
+ default:
+ return false;
+ case OO_New:
+ case OO_Array_New:
+ New = true;
+ break;
+ case OO_Delete:
+ case OO_Array_Delete:
+ New = false;
+ break;
+ }
+
+ // Variadic functions are always placement functions.
+ if (Node.isVariadic())
+ return true;
+
+ // Placement new is easy: it always has more than one parameter (the first
+ // parameter is always the size). If it's an overload of delete or delete[]
+ // that has only one parameter, it's never a placement delete.
+ if (New)
+ return Node.getNumParams() > 1;
+ else if (Node.getNumParams() == 1)
+ return false;
+
+ // Placement delete is a little more challenging. They always have more than
+ // one parameter with the first parameter being a pointer. However, the
+ // second parameter can be a size_t for sized deallocation, and that is never
+ // a placement delete operator.
+ if (Node.getNumParams() <= 1 || Node.getNumParams() > 2)
+ return true;
+
+ const FunctionProtoType *FPT = Node.getType()->castAs<FunctionProtoType>();
+ ASTContext &Ctx = Node.getASTContext();
+ if (Ctx.getLangOpts().SizedDeallocation &&
+ Ctx.hasSameType(FPT->getParamType(1), Ctx.getSizeType()))
+ return false;
+
+ return true;
+}
+} // namespace
+
+namespace tidy {
+namespace misc {
+
+namespace {
+OverloadedOperatorKind GetCorrespondingOverload(const FunctionDecl *FD) {
+ switch (FD->getOverloadedOperator()) {
+ default: break;
+ case OO_New:
+ return OO_Delete;
+ case OO_Delete:
+ return OO_New;
+ case OO_Array_New:
+ return OO_Array_Delete;
+ case OO_Array_Delete:
+ return OO_Array_New;
+ }
+ llvm_unreachable("Not an overloaded allocation operator");
+}
+
+const char *GetOperatorName(OverloadedOperatorKind K) {
+ switch (K) {
+ default: break;
+ case OO_New:
+ return "operator new";
+ case OO_Delete:
+ return "operator delete";
+ case OO_Array_New:
+ return "operator new[]";
+ case OO_Array_Delete:
+ return "operator delete[]";
+ }
+ llvm_unreachable("Not an overloaded allocation operator");
+}
+
+bool AreCorrespondingOverloads(const FunctionDecl *LHS,
+ const FunctionDecl *RHS) {
+ return RHS->getOverloadedOperator() == GetCorrespondingOverload(LHS);
+}
+
+bool HasCorrespondingOverloadInOneClass(const CXXRecordDecl *RD,
+ const CXXMethodDecl *MD) {
+ // Check the methods in the given class.
+ for (const auto *BMD : RD->methods())
+ if (BMD->isOverloadedOperator() && AreCorrespondingOverloads(MD, BMD))
+ return true;
+
+ // Check base classes.
+ for (const auto &BS : RD->bases())
+ if (HasCorrespondingOverloadInOneClass(BS.getType()->getAsCXXRecordDecl(),
+ MD))
+ return true;
+
+ return false;
+}
+bool HasCorrespondingOverloadInBaseClass(const CXXMethodDecl *MD) {
+ // Get the parent class of the method; we do not need to care about checking
+ // the methods in this class as the caller has already done that by looking
+ // at the declaration contexts.
+ const CXXRecordDecl *RD = MD->getParent();
+
+ for (const auto &BS : RD->bases())
+ if (HasCorrespondingOverloadInOneClass(BS.getType()->getAsCXXRecordDecl(),
+ MD))
+ return true;
+
+ return false;
+}
+} // anonymous namespace
+
+void NewDeleteOverloadsCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus)
+ return;
+
+ // Match all operator new and operator delete overloads (including the array
+ // forms). Do not match implicit operators, placement operators, or deleted
+ // or nonpublic operators.
+ Finder->addMatcher(
+ functionDecl(
+ unless(anyOf(isImplicit(), isPlacementOverload(), isDeleted(),
+ cxxMethodDecl(unless(isPublic())))),
+ anyOf(hasOverloadedOperatorName("new"),
+ hasOverloadedOperatorName("new[]"),
+ hasOverloadedOperatorName("delete"),
+ hasOverloadedOperatorName("delete[]")))
+ .bind("func"),
+ this);
+}
+
+void NewDeleteOverloadsCheck::check(const MatchFinder::MatchResult &Result) {
+ // Add any matches we locate to the list of things to be checked at the
+ // end of the translation unit.
+ Overloads.push_back(Result.Nodes.getNodeAs<FunctionDecl>("func"));
+}
+
+void NewDeleteOverloadsCheck::onEndOfTranslationUnit() {
+ // Walk over the list of declarations we've found to see if there is a
+ // corresponding overload at the same declaration context or within a base
+ // class. If there is not, add the element to the list of declarations to
+ // diagnose.
+ SmallVector<const FunctionDecl *, 4> Diagnose;
+ for (const auto *O : Overloads) {
+ const auto &OI = std::find_if(
+ Overloads.begin(), Overloads.end(), [&](const FunctionDecl *FD) {
+ if (FD == O)
+ return false;
+ // If the declaration contexts don't match, we don't need to check
+ // any further.
+ if (FD->getDeclContext() != O->getDeclContext())
+ return false;
+
+ // Since the declaration contexts match, see whether the current
+ // element is the corresponding operator.
+ if (!AreCorrespondingOverloads(O, FD))
+ return false;
+
+ return true;
+ });
+
+ if (OI == Overloads.end()) {
+ // Check to see if there is a corresponding overload in a base class
+ // context. If there isn't, or if the overload is not a class member
+ // function, then we should diagnose.
+ const auto *MD = dyn_cast<CXXMethodDecl>(O);
+ if (!MD || !HasCorrespondingOverloadInBaseClass(MD))
+ Diagnose.push_back(O);
+ }
+ }
+
+ for (const auto *FD : Diagnose)
+ diag(FD->getLocation(), "declaration of %0 has no matching declaration "
+ "of '%1' at the same scope")
+ << FD << GetOperatorName(GetCorrespondingOverload(FD));
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -19,6 +19,7 @@
#include "MacroParenthesesCheck.h"
#include "MacroRepeatedSideEffectsCheck.h"
#include "MoveConstructorInitCheck.h"
+#include "NewDeleteOverloadsCheck.h"
#include "NoexceptMoveConstructorCheck.h"
#include "SizeofContainerCheck.h"
#include "StaticAssertCheck.h"
@@ -53,6 +54,8 @@
"misc-macro-repeated-side-effects");
CheckFactories.registerCheck<MoveConstructorInitCheck>(
"misc-move-constructor-init");
+ CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
+ "misc-new-delete-overloads");
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
"misc-noexcept-move-constructor");
CheckFactories.registerCheck<SizeofContainerCheck>(
Index: clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tidy/misc/CMakeLists.txt
+++ clang-tidy/misc/CMakeLists.txt
@@ -11,6 +11,7 @@
MacroRepeatedSideEffectsCheck.cpp
MiscTidyModule.cpp
MoveConstructorInitCheck.cpp
+ NewDeleteOverloadsCheck.cpp
NoexceptMoveConstructorCheck.cpp
SizeofContainerCheck.cpp
StaticAssertCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
