MPIBufferDerefCheck for Clang-Tidy

[Alexander_Droste]

Summary

This check verifies if a buffer passed to an MPI (Message Passing Interface)
function is sufficiently dereferenced. Buffers should be passed as a single
pointer or array. As MPI function signatures specify void * for their buffer
types, insufficiently dereferenced buffers can be passed, like for example
as double pointers or multidimensional arrays, without a compiler warning
emitted.

Instructions on how to apply the check can be found at: https://github.com/0ax1/MPI-Checker/tree/master/examples

[Alexander_Droste]

• make = count accurate for mpi-buffer-deref.rst

[Alexander_Droste]

• fix two MPIdatatype tags

[Alexander_Droste]

• remove trailing whitespace

[hokein]

Looks like the dependency of this patch D21962 got reverted by some reasons.

clang-tidy/mpi/BufferDerefCheck.cpp

64

Could you add some comments to describe what these statements do? It's confused when reading the magic number parameter. The same below.

110

size_t

[Alexander_Droste]

Hi, thanks for having a look!

clang-tidy/mpi/BufferDerefCheck.cpp

64

Sure, I'll extend the comment.

110

Hmm, this loop iterates the indirections in reverse order and exits the loop if i gets smaller than 0. size_t would therefore not work. I think it's safe to assume that the number of indirections will not exceed INT_MAX. How about an explicit cast? I can't use rbegin and rend, as the current index is needed for:

if (i > 0) {
  IndirectionDesc += "->";
}

[Alexander_Droste]

Looks like the dependency of this patch D21962 got reverted by some reasons.

I know, I updated the patch after that, in order to fix the problem.

[Alexander_Droste]

• clarify lambda argument with comment
• explicit cast to int for the number of indirections in for loop

[Alexander_Droste]

• add needed extra line for docs

[hokein]

clang-tidy/mpi/BufferDerefCheck.cpp

88

check whether BufferType is nullptr

111

I think you can simplify the code as below:

for (auto Iter = Indirections.rbegin(); Iter != Indirections.rend(); ++Iter) {
   if (!IndirectionDesc.empty()) IndirectionDesc += "->";
   if (Indirections[i] == IndirectionType::Pointer) {
      IndirectionDesc += "pointer";
   } else {
      IndirectionDesc += "array";
   }
}

docs/clang-tidy/checks/list.rst

113

Please also mention the check in docs/ReleaseNotes.rst. The same to mpi-type-mismatch.

[Alexander_Droste]

clang-tidy/mpi/BufferDerefCheck.cpp

88

Null pointer types are not added to the BufferType vector in addBuffer.

if (CE->getArg(BufferIdx)->isNullPointerConstant(
            *Result.Context, Expr::NPC_ValueDependentIsNull) ||

111

Good idea, that looks better.

docs/clang-tidy/checks/list.rst

113

Done.

[Alexander_Droste]

• add release docs
• simplify reverse for loop -> rbegin, rend

[Alexander_Droste]

clang-tidy/mpi/BufferDerefCheck.cpp

88

I just realized, you meant sth. different. I'll update the patch to check for nullptr in addBuffer.

[Alexander_Droste]

• check for nullptr in addBuffer

[hokein]

LGTM.

[Alexander_Droste]

Great; thanks again for the review!

[hokein]

Do you have commit access now?

[Alexander_Droste]

No but I guess it would be a good idea to ask for commit access.
I'll proceed like suggested here: http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

clang-tidy/mpi/BufferDerefCheck.h

+//===--- BufferDerefCheck.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_MPI_BUFFER_DEREF_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MPI_BUFFER_DEREF_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace mpi {
+
+/// This check verifies if a buffer passed to an MPI (Message Passing Interface)
+/// function is sufficiently dereferenced. Buffers should be passed as a single
+/// pointer or array. As MPI function signatures specify void * for their buffer
+/// types, insufficiently dereferenced buffers can be passed, like for example
+/// as double pointers or multidimensional arrays, without a compiler warning
+/// emitted.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/mpi-buffer-deref.html
+class BufferDerefCheck : public ClangTidyCheck {
+public:
+  BufferDerefCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  /// Checks for all buffers in an MPI call if they are sufficiently
+  /// dereferenced.
+  ///
+  /// \param BufferTypes buffer types
+  /// \param BufferExprs buffer arguments as expressions
+  void checkBuffers(ArrayRef<const Type *> BufferTypes,
+                    ArrayRef<const Expr *> BufferExprs);
+
+  enum class IndirectionType : unsigned char { Pointer, Array };
+};
+
+} // namespace mpi
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MPI_BUFFER_DEREF_H

clang-tidy/mpi/BufferDerefCheck.cpp

+//===--- BufferDerefCheck.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 "BufferDerefCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Checkers/MPIFunctionClassifier.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace mpi {
+
+void BufferDerefCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(callExpr().bind("CE"), this);
+}
+
+void BufferDerefCheck::check(const MatchFinder::MatchResult &Result) {
+  static ento::mpi::MPIFunctionClassifier FuncClassifier(*Result.Context);
+  const auto *CE = Result.Nodes.getNodeAs<CallExpr>("CE");
+  if (!CE->getDirectCallee())
+    return;
+
+  const IdentifierInfo *Identifier = CE->getDirectCallee()->getIdentifier();
+  if (!Identifier || !FuncClassifier.isMPIType(Identifier))
+    return;
+
+  // These containers are used, to capture the type and expression of a buffer.
+  SmallVector<const Type *, 1> BufferTypes;
+  SmallVector<const Expr *, 1> BufferExprs;
+
+  // Adds the type and expression of a buffer that is used in the MPI call
+  // expression to the captured containers.
+  auto addBuffer = [&CE, &Result, &BufferTypes,
+                    &BufferExprs](const size_t BufferIdx) {
+    // Skip null pointer constants and in place 'operators'.
+    if (CE->getArg(BufferIdx)->isNullPointerConstant(
+            *Result.Context, Expr::NPC_ValueDependentIsNull) ||
+        tooling::fixit::getText(*CE->getArg(BufferIdx), *Result.Context) ==
+            "MPI_IN_PLACE")
+      return;
+
+    const Type *ArgType =
+        CE->getArg(BufferIdx)->IgnoreImpCasts()->getType().getTypePtr();
+    BufferExprs.push_back(CE->getArg(BufferIdx));
+    BufferTypes.push_back(ArgType);
+  };
+
+  // Collect buffer types and argument expressions for all buffers used in the
+  // MPI call expression. The number passed to the lambda corresponds to the
+  // argument index of the currently verified MPI function call.
+  if (FuncClassifier.isPointToPointType(Identifier)) {
+    addBuffer(0);
+  } else if (FuncClassifier.isCollectiveType(Identifier)) {
+    if (FuncClassifier.isReduceType(Identifier)) {
+      addBuffer(0);
+      addBuffer(1);
+    } else if (FuncClassifier.isScatterType(Identifier) ||
+               FuncClassifier.isGatherType(Identifier) ||
+               FuncClassifier.isAlltoallType(Identifier)) {
+      addBuffer(0);
+      addBuffer(3);
+    } else if (FuncClassifier.isBcastType(Identifier)) {
+      addBuffer(0);
+    }
+  }
+
+  checkBuffers(BufferTypes, BufferExprs);
+}
+
+void BufferDerefCheck::checkBuffers(ArrayRef<const Type *> BufferTypes,
+                                    ArrayRef<const Expr *> BufferExprs) {
+  for (size_t i = 0; i < BufferTypes.size(); ++i) {
+    unsigned IndirectionCount = 0;
+    const Type *BufferType = BufferTypes[i];
+    llvm::SmallVector<IndirectionType, 1> Indirections;
+
+    // Capture the depth and types of indirections for the passed buffer.
+    while (true) {
+      if (BufferType->isPointerType()) {
+        BufferType = BufferType->getPointeeType().getTypePtr();
+        Indirections.push_back(IndirectionType::Pointer);
+      } else if (BufferType->isArrayType()) {
+        BufferType = BufferType->getArrayElementTypeNoTypeQual();
+        Indirections.push_back(IndirectionType::Array);
+      } else {
+        break;
+      }
+      ++IndirectionCount;
+    }
+
+    if (IndirectionCount > 1) {
+      // Referencing an array with '&' is valid, as this also points to the
+      // beginning of the array.
+      if (IndirectionCount == 2 &&
+          Indirections[0] == IndirectionType::Pointer &&
+          Indirections[1] == IndirectionType::Array)
+        return;
+
+      // Build the indirection description in reverse order of discovery.
+      std::string IndirectionDesc;
+      for (auto It = Indirections.rbegin(); It != Indirections.rend(); ++It) {
+        if (!IndirectionDesc.empty())
+          IndirectionDesc += "->";
+        if (*It == IndirectionType::Pointer) {
+          IndirectionDesc += "pointer";
+        } else {
+          IndirectionDesc += "array";
+        }
+      }
+
+      const auto Loc = BufferExprs[i]->getSourceRange().getBegin();
+      diag(Loc, "buffer is insufficiently dereferenced: %0") << IndirectionDesc;
+    }
+  }
+}
+
+} // namespace mpi
+} // namespace tidy
+} // namespace clang

clang-tidy/mpi/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMPIModule
+  BufferDerefCheck.cpp
   MPITidyModule.cpp
   TypeMismatchCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   clangStaticAnalyzerCheckers
   )

clang-tidy/mpi/MPITidyModule.cpp

 //===--- MPITidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "BufferDerefCheck.h"
 #include "TypeMismatchCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace mpi {
 
 class MPIModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<BufferDerefCheck>("mpi-buffer-deref");
     CheckFactories.registerCheck<TypeMismatchCheck>("mpi-type-mismatch");
   }
 };
 
 } // namespace mpi
 
 // Register the MPITidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<mpi::MPIModule>
     X("mpi-module", "Adds MPI clang-tidy checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the MPIModule.
 volatile int MPIModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

docs/ReleaseNotes.rst

- New `cppcoreguidelines-slicing
 
   Flags slicing of member variables or vtable.
 
 - New `cppcoreguidelines-special-member-functions
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html>`_ check
 
   Flags classes where some, but not all, special member functions are user-defined.
 
+- New `mpi-buffer-deref
+  <http://clang.llvm.org/extra/clang-tidy/checks/mpi-buffer-deref.html>`_ check
+
+  Flags buffers which are insufficiently dereferenced when passed to an MPI function call.
+
+- New `mpi-type-mismatch
+  <http://clang.llvm.org/extra/clang-tidy/checks/mpi-type-mismatch.html>`_ check
+
+  Flags MPI function calls with a buffer type and MPI data type mismatch.
+
 - New `performance-inefficient-string-concatenation
   <http://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-string-concatenation.html>`_ check
 
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-shrink-to-fit
    modernize-use-auto
    modernize-use-bool-literals
    modernize-use-default
    modernize-use-emplace
    modernize-use-nullptr
    modernize-use-override
    modernize-use-using
+   mpi-buffer-deref
    mpi-type-mismatch
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop
    performance-inefficient-string-concatenation
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    readability-avoid-const-params-in-decls

docs/clang-tidy/checks/mpi-buffer-deref.rst

+.. title:: clang-tidy - mpi-buffer-deref
+
+mpi-buffer-deref
+================
+
+This check verifies if a buffer passed to an MPI (Message Passing Interface)
+function is sufficiently dereferenced. Buffers should be passed as a single
+pointer or array. As MPI function signatures specify ``void *`` for their buffer
+types, insufficiently dereferenced buffers can be passed, like for example as
+double pointers or multidimensional arrays, without a compiler warning emitted.
+
+Examples:
+.. code:: c++
+
+  // A double pointer is passed to the MPI function.
+  char *buf;
+  MPI_Send(&buf, 1, MPI_CHAR, 0, 0, MPI_COMM_WORLD);
+
+  // A multidimensional array is passed to the MPI function.
+  short buf[1][1];
+  MPI_Send(buf, 1, MPI_SHORT, 0, 0, MPI_COMM_WORLD);
+
+  // A pointer to an array is passed to the MPI function.
+  short *buf[1];
+  MPI_Send(buf, 1, MPI_SHORT, 0, 0, MPI_COMM_WORLD);

test/clang-tidy/Inputs/mpi-type-mismatch/mpimock.h

 typedef int uint16_t;
 typedef int int64_t;
 namespace std { template<class T> struct complex { T real; T imag; }; }
 
 // These defines are used to mock MPI constants.
 #define MPI_DATATYPE_NULL 0
 #define MPI_CHAR 0
 #define MPI_BYTE 0
+#define MPI_SHORT 0
 #define MPI_INT 0
 #define MPI_LONG 0
 #define MPI_LONG_DOUBLE 0
 #define MPI_UNSIGNED 0
 #define MPI_INT8_T 0
 #define MPI_UINT8_T 0
 #define MPI_UINT16_T 0
+#define MPI_C_FLOAT_COMPLEX 0
 #define MPI_C_LONG_DOUBLE_COMPLEX 0
 #define MPI_FLOAT 0
 #define MPI_DOUBLE 0
 #define MPI_CXX_BOOL 0
 #define MPI_CXX_FLOAT_COMPLEX 0
 #define MPI_CXX_DOUBLE_COMPLEX 0
 #define MPI_CXX_LONG_DOUBLE_COMPLEX 0
 #define MPI_IN_PLACE 0

test/clang-tidy/mpi-buffer-deref.cpp

+// RUN: %check_clang_tidy %s mpi-buffer-deref %t -- -- -I %S/Inputs/mpi-type-mismatch
+
+#include "mpimock.h"
+
+void negativeTests() {
+  char *buf;
+  MPI_Send(&buf, 1, MPI_CHAR, 0, 0, MPI_COMM_WORLD);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: buffer is insufficiently dereferenced: pointer->pointer [mpi-buffer-deref]
+
+  unsigned **buf2;
+  MPI_Send(buf2, 1, MPI_UNSIGNED, 0, 0, MPI_COMM_WORLD);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: buffer is insufficiently dereferenced: pointer->pointer
+
+  short buf3[1][1];
+  MPI_Send(buf3, 1, MPI_SHORT, 0, 0, MPI_COMM_WORLD);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: buffer is insufficiently dereferenced: array->array
+
+  long double _Complex *buf4[1];
+  MPI_Send(buf4, 1, MPI_C_LONG_DOUBLE_COMPLEX, 0, 0, MPI_COMM_WORLD);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: buffer is insufficiently dereferenced: pointer->array
+
+  std::complex<float> *buf5[1][1];
+  MPI_Send(&buf5, 1, MPI_CXX_FLOAT_COMPLEX, 0, 0, MPI_COMM_WORLD);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: buffer is insufficiently dereferenced: pointer->array->array->pointer
+}
+
+void positiveTests() {
+  char buf;
+  MPI_Send(&buf, 1, MPI_CHAR, 0, 0, MPI_COMM_WORLD);
+
+  unsigned *buf2;
+  MPI_Send(buf2, 1, MPI_UNSIGNED, 0, 0, MPI_COMM_WORLD);
+
+  short buf3[1][1];
+  MPI_Send(buf3[0], 1, MPI_SHORT, 0, 0, MPI_COMM_WORLD);
+
+  long double _Complex *buf4[1];
+  MPI_Send(*buf4, 1, MPI_C_LONG_DOUBLE_COMPLEX, 0, 0, MPI_COMM_WORLD);
+
+  long double _Complex buf5[1];
+  MPI_Send(buf5, 1, MPI_C_LONG_DOUBLE_COMPLEX, 0, 0, MPI_COMM_WORLD);
+
+  std::complex<float> *buf6[1][1];
+  MPI_Send(*buf6[0], 1, MPI_CXX_FLOAT_COMPLEX, 0, 0, MPI_COMM_WORLD);
+
+  // Referencing an array with '&' is valid, as this also points to the
+  // beginning of the array.
+  long double _Complex buf7[1];
+  MPI_Send(&buf7, 1, MPI_C_LONG_DOUBLE_COMPLEX, 0, 0, MPI_COMM_WORLD);
+}