[clang-tidy] Add bugprone-placement-new-target-type-mismatch check

[DennisL]

Summary

Finds placement-new calls where the pointer type of the adress mismatches the type of the created value

[Eugene.Zelenko]

clang-tidy/misc/PlacementNewTargetSizeCheck.cpp

10 ↗

(On Diff #193318)

Unnecessary empty line. Please run Clang-format after fixing.

23 ↗

(On Diff #193318)

Please use static instead of anonymous namespaces. See LLVM Coding Guidelines.

26 ↗

(On Diff #193318)

Please don't use auto where type could not be easily deduced.

28 ↗

(On Diff #193318)

Please don't use auto where type could not be easily deduced.

47 ↗

(On Diff #193318)

auto could be used here, because of cast.

50 ↗

(On Diff #193318)

Please don't use auto where type could not be easily deduced.

55 ↗

(On Diff #193318)

Please don't use auto where type could not be easily deduced.

62 ↗

(On Diff #193318)

auto could be used here, because of cast.

63 ↗

(On Diff #193318)

Please don't use auto where type could not be easily deduced.

65 ↗

(On Diff #193318)

Please run Clang-format.

73 ↗

(On Diff #193318)

Unnecessary empty line.

docs/ReleaseNotes.rst

135

Please add one sentence description. Should be same as in documentation.

docs/clang-tidy/checks/misc-placement-new-target-size.rst

5 ↗

(On Diff #193318)

Unnecessary empty line.

[DennisL]

Updated patch to address reviewer feedback

[DennisL]

Remove debug output

[DennisL]

Simplify logic

[Eugene.Zelenko]

docs/clang-tidy/checks/misc-placement-new-target-size.rst

5 ↗

(On Diff #193318)

Somehow empty line is still there.

[DennisL]

Addressed reviewer feedback and also simplified the implementation, renamed the test to be more concise, and added more test cases.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

136

Please synchronize with documentation.

[DennisL]

Sync ReleaseNotes.rst with docs

[alexfh]

Looks like this check would fit better into the bugprone module.

[DennisL]

In D60139#1456123, @alexfh wrote:

Looks like this check would fit better into the bugprone module.

Excellent suggestion. Thanks. Will follow up with an updated Diff.

[DennisL]

The following has been updated:

• moved check to bugprone instead of misc
• more tests
• rewritten check logic

[DennisL]

Removed debug output

[DennisL]

• handle array to ptr decay
• updated error msg
• additional tests for arry to ptr decay

[JonasToth]

Hey Dennis,

my 2cents on the check. I think it is very good to have! Did you check coding guidelines if they say something to this issue? (e.g. cppcoreguidelines, hicpp, cert) As we have modules for them it would be great to make aliases to this check if they demand this to be checked.

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp

22

Please write full sentences with punctuation in comments.

23

placement-new? please make a new matcher (locally) to match that, because its is more specific.
You can use other checks as example, grep for "AST_MATCHER"

31

Please ellide braces.

39

braces, above the comment does not follow the correct-sentence style

43

Is this universally true? What about the nothrow-overload, would that interfere?

46

QualType is usually used a value-type, not const-ref. Please follow that for consistency.

51

braces.

docs/clang-tidy/checks/bugprone-placement-new-target-type-mismatch.rst

8

Please add examples to demonstrate good and bad code for the user.

test/clang-tidy/bugprone-placement-new-target-type-mismatch.cpp

6

Please add a test for the nothrow overload.

87

Please add test, where everything is hidden behind templates and only the type-substitution leads to the error.
A test including macros helps as well, to show they do not interfere (as should be the case with this check).

[Eugene.Zelenko]

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp

39

I would put nullptr as second argument for == operator.

[DennisL]

In D60139#1460233, @JonasToth wrote:

Hey Dennis,

my 2cents on the check. I think it is very good to have! Did you check coding guidelines if they say something to this issue? (e.g. cppcoreguidelines, hicpp, cert) As we have modules for them it would be great to make aliases to this check if they demand this to be checked.

Thanks for the great suggestions. Updated the diff according to the feedback. Also checked with cppcoreguidelines, hicpp as well as cert. Only cert has a related, yet different rule stating that calls to placement new shall be provided with properly aligned pointers. I'd say this should be a distinct check. Happy to work on it after this one.

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp

43

Thanks, rewrote that check.

[DennisL]

Changed the following

• addressed reviewer feedback
• fetch the placement parameter as written
• add further test cases as requested
• stylistic changes
• add nothrow parameter support
• ignore matches with unresolved template parameters
• add examples of bad and good code

[aaron.ballman]

In D60139#1461269, @DennisL wrote:

In D60139#1460233, @JonasToth wrote:

Hey Dennis,

my 2cents on the check. I think it is very good to have! Did you check coding guidelines if they say something to this issue? (e.g. cppcoreguidelines, hicpp, cert) As we have modules for them it would be great to make aliases to this check if they demand this to be checked.

Thanks for the great suggestions. Updated the diff according to the feedback. Also checked with cppcoreguidelines, hicpp as well as cert. Only cert has a related, yet different rule stating that calls to placement new shall be provided with properly aligned pointers. I'd say this should be a distinct check. Happy to work on it after this one.

What is the rationale for separating those checks? It sort of feels like there is some overlap between alignment, size, and buffer type.

I'm not certain I agree with the way this check operates -- what problem is it trying to solve? For instance, this code would trigger on this check, but is perfectly valid code:

static_assert(sizeof(int) == sizeof(float));
static_assert(alignof(int) == alignof(float));
int buffer;
float *fp = new (&buffer) float;

The situations I can think of where the type mismatch is an issue would be for types with nontrivial special members, so there are times when this check makes sense, but perhaps it's too restrictive currently, but you may have a different problem in mind?

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp

20

This should match on CXXNewExpr instead of Expr, then you won't need the dyn_cast below.

29

This should probably use cxxNewExpr() instead of expr() so you match on something more specific.

38

This assert message looks a bit incomplete. ;-)

43

You can use const auto * here.

test/clang-tidy/bugprone-placement-new-target-type-mismatch.cpp

56

While this code definitely has a bug from the buffer overflow, I don't think the diagnostic is valuable here. This is a very common pattern and I suspect this will yield a lot of false positives, unless the check starts taking alignment and buffer size into consideration.

clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "ParentVirtualCallCheck.h"
+#include "PlacementNewTargetTypeMismatch.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
     CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
         "bugprone-too-small-loop-variable");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
+    CheckFactories.registerCheck<PlacementNewTargetTypeMismatch>(
+        "bugprone-placement-new-target-type-mismatch");
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "bugprone-string-integer-assignment");

clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   ParentVirtualCallCheck.cpp
+  PlacementNewTargetTypeMismatch.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.h

+//===--- PlacementNewTargetTypeMismatch.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_BUGPRONE_PLACEMENTNEWTARGETTYPEMISMATCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PLACEMENTNEWTARGETTYPEMISMATCHCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds placement-new calls where the pointer type of the adress mismatches
+/// the type of the created value.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-placement-new-target-type-mismatch.html
+class PlacementNewTargetTypeMismatch : public ClangTidyCheck {
+public:
+  PlacementNewTargetTypeMismatch(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PLACEMENTNEWTARGETTYPEMISMATCHCHECK_H

clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp

+//===--- PlacementNewTargetTypeMismatch.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 "PlacementNewTargetTypeMismatch.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+namespace {
+AST_MATCHER(Expr, isPlacementNewExpr) {
+  const auto *NewExpr = dyn_cast<CXXNewExpr>(&Node);
+  return NewExpr && NewExpr->getNumPlacementArgs() != 0;
+}
+} // namespace
+
+void PlacementNewTargetTypeMismatch::registerMatchers(MatchFinder *Finder) {
+  // We only want the records that call 'new' with an adress parameter.
+  Finder->addMatcher(
+      expr(isPlacementNewExpr(), unless(hasDescendant(unresolvedLookupExpr())))
+          .bind("NewExpr"),
+      this);
+}
+
+void PlacementNewTargetTypeMismatch::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *NewExpr = Result.Nodes.getNodeAs<CXXNewExpr>("NewExpr");
+  assert(NewExpr && "Matched node bound by 'NewExpr' shoud be a 'CXXNewExpr'");
+  assert(NewExpr->getNumPlacementArgs() != 0 && "");
+
+  // Fetch the cast from the Expr of the placement argument, if it exists.
+  const Expr *PlacementExpr = NewExpr->getPlacementArg(0);
+  assert(PlacementExpr != nullptr && "PlacementExpr should not be null");
+  const CastExpr *Cast = dyn_cast<CastExpr>(PlacementExpr);
+  if (Cast == nullptr)
+    return;
+
+  QualType SubExprType = Cast->getSubExprAsWritten()->getType();
+
+  if ((!SubExprType->isPointerType() && !SubExprType->isArrayType()) ||
+      SubExprType->isVoidPointerType())
+    return;
+
+  QualType PlacementParameterType =
+      SubExprType->getPointeeOrArrayElementType()->getCanonicalTypeInternal();
+  QualType AllocatedType = NewExpr->getAllocatedType().getCanonicalType();
+
+  if (AllocatedType->isIncompleteType() ||
+      PlacementParameterType->isIncompleteType())
+    return;
+
+  if (PlacementParameterType != AllocatedType) {
+    diag(PlacementExpr->getExprLoc(),
+         "placement new parameter and allocated type mismatch");
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 - The `Acronyms` and `IncludeDefaultAcronyms` options for the
   :doc:`objc-property-declaration <clang-tidy/checks/objc-property-declaration>`
   check have been removed.
 
 - The :doc:`modernize-use-override
   <clang-tidy/checks/modernize-use-override>` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`bugprone-placement-new-target-type-mismatch
+  <clang-tidy/checks/bugprone-placement-new-target-type-mismatch>` check.
+
+  Finds placement-new calls where the pointer type of the adress mismatches the
+  type of the created value.
+
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 
   Analyzes OpenMP Structured Blocks and checks that no exception escapes
   out of the Structured Block it was thrown in.
 
 - New :doc:`openmp-use-default-none
   <clang-tidy/checks/openmp-use-default-none>` check.

docs/clang-tidy/checks/bugprone-placement-new-target-type-mismatch.rst

+.. title:: clang-tidy - misc-placement-new-target-type-mismatch
+
+misc-placement-new-target-type-mismatch
+=======================================
+
+Finds placement-new calls where the pointer type of the adress mismatches the
+type of the created value.
+
+Example:
+
+.. code-block:: c++
+
+  void ok() {
+    Foo * ptr = new Foo;
+    new (ptr) Foo;
+    ...
+  }
+
+  void not_ok() {
+    int * ptr = new int;
+    new (ptr) Foo;
+    ...
+  }

docs/clang-tidy/checks/list.rst

.. toctree::
    bugprone-lambda-function-name
    bugprone-macro-parentheses
    bugprone-macro-repeated-side-effects
    bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-misplaced-widening-cast
    bugprone-move-forwarding-reference
    bugprone-multiple-statement-macro
    bugprone-parent-virtual-call
+   bugprone-placement-new-target-type-mismatch
    bugprone-sizeof-container
    bugprone-sizeof-expression
    bugprone-string-constructor
    bugprone-string-integer-assignment
    bugprone-string-literal-with-embedded-nul
    bugprone-suspicious-enum-usage
    bugprone-suspicious-memset-usage
    bugprone-suspicious-missing-comma

test/clang-tidy/bugprone-placement-new-target-type-mismatch.cpp

+// RUN: %check_clang_tidy %s bugprone-placement-new-target-type-mismatch %t
+
+// definitions
+  
+namespace std {
+struct nothrow_t { explicit nothrow_t() = default; } nothrow;
+template<class T> T* addressof(T& arg) noexcept;
+template< class T > struct remove_reference      {typedef T type;};
+template< class T > struct remove_reference<T&>  {typedef T type;};
+template< class T > struct remove_reference<T&&> {typedef T type;};
+template< class T >
+T&& forward( typename std::remove_reference<T>::type& t ) noexcept;
+} // namespace std
+
+using size_type = unsigned long;
+void *operator new(size_type, void *);
+void *operator new[](size_type, void *);
+void* operator new(size_type size, const std::nothrow_t&) noexcept;
+void* operator new(size_type size, const std::nothrow_t&) noexcept;
+void* operator new[](size_type size, const std::nothrow_t&) noexcept;
+
+struct Foo {
+  int a;
+  int b;
+  int c;
+  int d;
+};
+
+template<typename T>
+T& getT() {
+  static T f;
+  return f;
+}
+
+// instances emitting warnings
+
+void f1() {
+  struct Dummy {
+    int a;
+    int b;
+  };
+  int *ptr = new int;
+  new (ptr) Dummy;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: placement new parameter and allocated type mismatch [bugprone-placement-new-target-type-mismatch]
+}
+
+void f2() {
+  int * ptr = new int;
+  new (ptr) Foo;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: placement new parameter and allocated type mismatch [bugprone-placement-new-target-type-mismatch]
+}
+
+void f3() {
+  char *ptr = new char[17*sizeof(char)];
+  new (ptr) float[13];
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: placement new parameter and allocated type mismatch [bugprone-placement-new-target-type-mismatch]
+}
+
+void f4() {
+  new (std::addressof(getT<int>())) Foo;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: placement new parameter and allocated type mismatch [bugprone-placement-new-target-type-mismatch]
+}
+
+void f5() {
+  char *ptr = new char[17*sizeof(char)];
+  new (ptr) float{13.f};
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: placement new parameter and allocated type mismatch [bugprone-placement-new-target-type-mismatch]
+}
+
+void f6() {
+  char array[17*sizeof(char)];
+  new (array) float{13.f};
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: placement new parameter and allocated type mismatch [bugprone-placement-new-target-type-mismatch]
+}
+
+// instances not emitting a warning
+
+void g1() {
+  Foo * ptr = new Foo;
+  new (ptr) Foo;
+}
+
+void g2() {
+  char *ptr = new char[17*sizeof(char)];
+  new ((float *)ptr) float{13.f};
+}
+
+void g3() {
+  char array[17*sizeof(char)];
+  new (array) char('A');
+}
+
+void g4() {
+  new ((void *)std::addressof(getT<Foo>())) Foo;
+}
+
+union
+{
+  char * buffer;
+} Union;
+
+template <typename T, typename... U>
+void g5(U &&... V) {
+  new (static_cast<T *>(Union.buffer)) T(std::forward<U>(V)...);
+}
+
+template <typename T, typename... U>
+void g6(U &&... V) {
+  new (std::nothrow) T(std::forward<U>(V)...);
+}
+
+template <typename T, typename... U> void g7(U &&... Us) {
+  new (static_cast<T *>(Union.buffer)) T(std::forward<U>(Us)...);
+}
+
+template <typename T, typename... U> void g8(U &&... Us) {
+  new (Union.buffer) T(std::forward<U>(Us)...);
+}