[clang-tidy] Fix mpi checks when running multiple TUs per clang-tidy process

[njames93]

Summary

Both the mpi-type-mismatch and mpi-buffer-deref check make use of a static MPIFunctionClassifier object.
This causes issue as the classifier is initialized with the first ASTContext that produces a match.
If the check is enabled on multiple translation units in a single clang-tidy process, this classifier won't be reinitialized for each TU. I'm not an expert in the MPIFunctionClassifier but I'd imagine this is a source of UB.
It is suspected that this bug may result in the crash caused here: https://bugs.llvm.org/show_bug.cgi?id=48985. However even if not the case, this should still be addressed.

[njames93]

Ping?

[aaron.ballman]

LGTM, thanks!

clang-tools-extra/clang-tidy/mpi/BufferDerefCheck.cpp

29–30

This is racy, but I don't think we support running multiple checks in parallel, do we?

[njames93]

clang-tools-extra/clang-tidy/mpi/BufferDerefCheck.cpp

29–30

This isn't racy as one instance of a check will only ever run on one thread.

[aaron.ballman]

clang-tools-extra/clang-tidy/mpi/BufferDerefCheck.cpp

29–30

I was thinking if the check gets run multiple times in parallel on different source files (e.g., someone downstream who incorporates the checks as a library in a creative way rather than runs clang-tidy as a command line tool). But yeah, I'm not worried about it as I don't think we support that sort of thing.

[njames93]

clang-tools-extra/clang-tidy/mpi/BufferDerefCheck.cpp

29–30

Plenty of other checks hold state that is mutated in the check callback, so if this is a race, all the other checks that mutate state in the callbacks will also have a race.

clang-tools-extra/clang-tidy/mpi/BufferDerefCheck.h

 //===--- BufferDerefCheck.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_MPI_BUFFER_DEREF_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MPI_BUFFER_DEREF_H
 
 #include "../ClangTidyCheck.h"
+#include "clang/StaticAnalyzer/Checkers/MPIFunctionClassifier.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;
+  void onEndOfTranslationUnit() 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 };
+
+  Optional<ento::mpi::MPIFunctionClassifier> FuncClassifier;
 };
 
 } // namespace mpi
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MPI_BUFFER_DEREF_H

clang-tools-extra/clang-tidy/mpi/BufferDerefCheck.cpp

 //===--- BufferDerefCheck.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 "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;
 
+  if (!FuncClassifier)
+    FuncClassifier.emplace(*Result.Context);
+
   const IdentifierInfo *Identifier = CE->getDirectCallee()->getIdentifier();
-  if (!Identifier || !FuncClassifier.isMPIType(Identifier))
+  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.
if (!ArgType)
       return;
     BufferExprs.push_back(ArgExpr);
     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)) {
+  if (FuncClassifier->isPointToPointType(Identifier)) {
     AddBuffer(0);
-  } else if (FuncClassifier.isCollectiveType(Identifier)) {
-    if (FuncClassifier.isReduceType(Identifier)) {
+  } else if (FuncClassifier->isCollectiveType(Identifier)) {
+    if (FuncClassifier->isReduceType(Identifier)) {
       AddBuffer(0);
       AddBuffer(1);
-    } else if (FuncClassifier.isScatterType(Identifier) ||
-               FuncClassifier.isGatherType(Identifier) ||
-               FuncClassifier.isAlltoallType(Identifier)) {
+    } else if (FuncClassifier->isScatterType(Identifier) ||
+               FuncClassifier->isGatherType(Identifier) ||
+               FuncClassifier->isAlltoallType(Identifier)) {
       AddBuffer(0);
       AddBuffer(3);
-    } else if (FuncClassifier.isBcastType(Identifier)) {
+    } else if (FuncClassifier->isBcastType(Identifier)) {
       AddBuffer(0);
     }
   }
 
   checkBuffers(BufferTypes, BufferExprs);
 }
 
 void BufferDerefCheck::checkBuffers(ArrayRef<const Type *> BufferTypes,
if (IndirectionCount > 1) {
       }
 
       const auto Loc = BufferExprs[I]->getSourceRange().getBegin();
       diag(Loc, "buffer is insufficiently dereferenced: %0") << IndirectionDesc;
     }
   }
 }
 
+void BufferDerefCheck::onEndOfTranslationUnit() { FuncClassifier.reset(); }
 } // namespace mpi
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/mpi/TypeMismatchCheck.h

 //===--- TypeMismatchCheck.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_MPI_TYPE_MISMATCH_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MPI_TYPE_MISMATCH_H
 
 #include "../ClangTidyCheck.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Checkers/MPIFunctionClassifier.h"
 
 namespace clang {
 namespace tidy {
 namespace mpi {
 
 /// This check verifies if buffer type and MPI (Message Passing Interface)
 /// datatype pairs match. All MPI datatypes defined by the MPI standard (3.1)
 /// are verified by this check. User defined typedefs, custom MPI datatypes and
 /// null pointer constants are skipped, in the course of verification.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/mpi-type-mismatch.html
 class TypeMismatchCheck : public ClangTidyCheck {
 public:
   TypeMismatchCheck(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;
+
 private:
   /// Check if the buffer type MPI datatype pairs match.
   ///
   /// \param BufferTypes buffer types
   /// \param BufferExprs buffer arguments as expressions
   /// \param MPIDatatypes MPI datatype
   /// \param LO language options
   void checkArguments(ArrayRef<const Type *> BufferTypes,
                       ArrayRef<const Expr *> BufferExprs,
                       ArrayRef<StringRef> MPIDatatypes, const LangOptions &LO);
+
+  Optional<ento::mpi::MPIFunctionClassifier> FuncClassifier;
 };
 
 } // namespace mpi
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MPI_TYPE_MISMATCH_H

clang-tools-extra/clang-tidy/mpi/TypeMismatchCheck.cpp

 //===--- TypeMismatchCheck.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 "TypeMismatchCheck.h"
 #include "clang/Lex/Lexer.h"
-#include "clang/StaticAnalyzer/Checkers/MPIFunctionClassifier.h"
 #include "clang/Tooling/FixIt.h"
 #include <map>
 #include <unordered_set>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
static const Type *argumentType(const CallExpr *const CE, const size_t Idx) {
   return QT.getTypePtr()->getPointeeOrArrayElementType();
 }
 
 void TypeMismatchCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(callExpr().bind("CE"), this);
 }
 
 void TypeMismatchCheck::check(const MatchFinder::MatchResult &Result) {
-  static ento::mpi::MPIFunctionClassifier FuncClassifier(*Result.Context);
   const auto *const CE = Result.Nodes.getNodeAs<CallExpr>("CE");
   if (!CE->getDirectCallee())
     return;
 
+  if (!FuncClassifier)
+    FuncClassifier.emplace(*Result.Context);
+
   const IdentifierInfo *Identifier = CE->getDirectCallee()->getIdentifier();
-  if (!Identifier || !FuncClassifier.isMPIType(Identifier))
+  if (!Identifier || !FuncClassifier->isMPIType(Identifier))
     return;
 
   // These containers are used, to capture buffer, MPI datatype pairs.
   SmallVector<const Type *, 1> BufferTypes;
   SmallVector<const Expr *, 1> BufferExprs;
   SmallVector<StringRef, 1> MPIDatatypes;
 
   // Adds a buffer, MPI datatype pair of an MPI call expression to the
if (!isStandardMPIDatatype(std::string(MPIDatatype)) ||
       return;
 
     BufferTypes.push_back(ArgType);
     BufferExprs.push_back(CE->getArg(BufferIdx));
     MPIDatatypes.push_back(MPIDatatype);
   };
 
   // Collect all buffer, MPI datatype pairs for the inspected call expression.
-  if (FuncClassifier.isPointToPointType(Identifier)) {
+  if (FuncClassifier->isPointToPointType(Identifier)) {
     AddPair(0, 2);
-  } else if (FuncClassifier.isCollectiveType(Identifier)) {
-    if (FuncClassifier.isReduceType(Identifier)) {
+  } else if (FuncClassifier->isCollectiveType(Identifier)) {
+    if (FuncClassifier->isReduceType(Identifier)) {
       AddPair(0, 3);
       AddPair(1, 3);
-    } else if (FuncClassifier.isScatterType(Identifier) ||
-               FuncClassifier.isGatherType(Identifier) ||
-               FuncClassifier.isAlltoallType(Identifier)) {
+    } else if (FuncClassifier->isScatterType(Identifier) ||
+               FuncClassifier->isGatherType(Identifier) ||
+               FuncClassifier->isAlltoallType(Identifier)) {
       AddPair(0, 2);
       AddPair(3, 5);
-    } else if (FuncClassifier.isBcastType(Identifier)) {
+    } else if (FuncClassifier->isBcastType(Identifier)) {
       AddPair(0, 2);
     }
   }
   checkArguments(BufferTypes, BufferExprs, MPIDatatypes, getLangOpts());
 }
 
 void TypeMismatchCheck::checkArguments(ArrayRef<const Type *> BufferTypes,
                                        ArrayRef<const Expr *> BufferExprs,
for (size_t I = 0; I < MPIDatatypes.size(); ++I) {
     if (Error) {
       const auto Loc = BufferExprs[I]->getSourceRange().getBegin();
       diag(Loc, "buffer type '%0' does not match the MPI datatype '%1'")
           << BufferTypeName << MPIDatatypes[I];
     }
   }
 }
 
+void TypeMismatchCheck::onEndOfTranslationUnit() { FuncClassifier.reset(); }
 } // namespace mpi
 } // namespace tidy
 } // namespace clang