[clang-tidy] safety-no-vector-bool

[jbcoe]

Summary

Add a new clang-tidy module for safety-critical checks.

Include a check which finds instances of vector<bool> which is banned by 'High Integrity C++'.

[JonasToth]

i think the test is not mentioned in the release notes is it?

[JonasToth]

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp

51

i think all those diag() calls can be merged into one. inside the if/else-if you can just set a StringRef with the specific part of the warning, and have a parameterized diag() at the end of the function.

in NoMallocCheck there is a similar pattern:

const CallExpr *Call = nullptr;                                              
StringRef Recommendation;                                                    
                                                                             
if ((Call = Result.Nodes.getNodeAs<CallExpr>("aquisition")))                 
  Recommendation = "consider a container or a smart pointer";                
else if ((Call = Result.Nodes.getNodeAs<CallExpr>("realloc")))               
  Recommendation = "consider std::vector or std::string";                    
else if ((Call = Result.Nodes.getNodeAs<CallExpr>("free")))                  
  Recommendation = "use RAII";                                               
                                                                             
assert(Call && "Unhandled binding in the Matcher");                          
                                                                             
diag(Call->getLocStart(), "do not manage memory manually; %0")               
    << Recommendation << SourceRange(Call->getLocStart(), Call->getLocEnd());

53

maybe an safety else with an failing assert, so you can see that unexpected behaviour, see comment above

clang-tools-extra/test/clang-tidy/safety-no-vector-bool.cpp

38

what happens for types where std::vector<bool> would be an template argument? for example std::pair and tuple could contain a vector<bool>.
is there a warning as well?

[djehuti]

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp

51

Except with braces, right? (That's another High-Integrity C++ rule btw.) ;)

[Eugene.Zelenko]

Please mention new checks group in docs/clang-tidy/index.rst (in alphabetical order).

Please mention this check in docs/ReleaseNotes.rst (see previous releases as example).

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp

22

Unnecessary empty line.

clang-tools-extra/docs/clang-tidy/checks/safety-no-vector-bool.rst

8

may be `std::vector` will be better?

[jbcoe]

Responses to some change requests. Input needed on template argument matcher.

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp

51

I agree that this _can_ be done but I'm not convinced it helps readability. Repetition is partial and very localized. I'll happily make the change if you feel strongly that it's an improvement.

clang-tools-extra/test/clang-tidy/safety-no-vector-bool.cpp

38

Nicely spotted. Those won't get picked up right now and need to be.

I'm struggling to build a matcher for this. We really need to find any place where std::vector<bool> is used as a template argument.

[JonasToth]

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp

51

i think either is ok. maybe someone else prefers one strongly over the other, but i dont mind.

but i think the else path should exist, make an failing assert or sth like that, for the safety ;)

clang-tools-extra/test/clang-tidy/safety-no-vector-bool.cpp

38

i found hasAnyTemplateArgument in the ast matcher refrence. did u use that one?

[djehuti]

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp

53

You mean like High Integrity C++ rule 6.1.2? 😉

[jbcoe]

Handling code like std::pair<std::vector<bool>, int> is currently eluding me.

If I define std::pair to have members first and second then std::pair<std::vector<bool>, int> triggers the vector<bool> diagnostic but in the template, not the instantiation point.

I need to match class definitions that have vector<bool> as a template argument and exclude them from later matchers. I can't see how to do this right now. I suspect some more sophisticated matchers or approaches are needed.

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp

51

Agreed on the else, especially as it agrees with the safety-critical standards we're checking.

51

LLVM/Clang tends not to use braces for single statement control flow, but maybe we should follow safety-critical rules in a safety-critical check?

[jbcoe]

Minor changes in response to review comments.

Patch abandoned as approach taken cannot handle templates.

[jbcoe]

Posting a comment re-opened this patch. The approach taken cannot handle templates using std::vector<bool>, a radical rework is needed.

https://reviews.llvm.org/D29267 introduces the safety critical module with a much simpler check (no assembler).

clang-tools-extra/clang-tidy/CMakeLists.txt

 add_subdirectory(mpi)
 add_subdirectory(performance)
 add_subdirectory(readability)
+add_subdirectory(safety)
 add_subdirectory(utils)

clang-tools-extra/clang-tidy/safety/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidySafetyModule
+  SafetyTidyModule.cpp
+  NoVectorBoolCheck.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.h

+//===--- NoVectorBoolCheck.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_SAFETY_NO_VECTOR_BOOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_SAFETY_NO_VECTOR_BOOL_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace safety {
+
+/// This check finds instances of `std::vector<bool>`. 
+/// No fix is offered.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/safety-no-vector-bool.html
+class NoVectorBoolCheck : public ClangTidyCheck {
+public:
+  NoVectorBoolCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace safety
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_SAFETY_NO_VECTOR_BOOL_H

clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp

+//===--- NoVectorBoolCheck.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 "NoVectorBoolCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace safety {
+
+void NoVectorBoolCheck::registerMatchers(MatchFinder *Finder) {
+  const auto isVectorBool = classTemplateSpecializationDecl(
+      hasName("::std::vector"),
+      hasTemplateArgument(0, refersToType(booleanType())),
+      hasTemplateArgument(1, refersToType(hasDeclaration(cxxRecordDecl(
+                                 matchesName("::std::allocator"))))));
+
+  Finder->addMatcher(fieldDecl(hasType(isVectorBool)).bind("member"), this);
+
+  Finder->addMatcher(varDecl(hasType(isVectorBool)).bind("variable"), this);
+
+  Finder->addMatcher(
+      functionDecl(returns(hasDeclaration(isVectorBool))).bind("function"),
+      this);
+}
+
+void NoVectorBoolCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FieldDecl>("member")) {
+    diag(MatchedDecl->getLocation(),
+         "member %0 is an instance of std::vector<bool>")
+        << MatchedDecl;
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<VarDecl>("variable")) {
+    diag(MatchedDecl->getLocation(),
+         " variable %0 is an instance of std::vector<bool>")
+        << MatchedDecl;
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<FunctionDecl>("function")) {
+    diag(MatchedDecl->getLocation(),
+         " function %0 returns an instance of std::vector<bool>")
+        << MatchedDecl;
+  } else {
+    llvm_unreachable("Unhandled binding in the matcher.");
+  }
+}
+
+} // namespace safety
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/safety/SafetyTidyModule.cpp

+//===------- SafetyTidyModule.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 "NoVectorBoolCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace safety {
+
+class SafetyModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<NoVectorBoolCheck>(
+        "safety-no-vector-bool");
+  }
+};
+
+// Register the SafetyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<SafetyModule>
+    X("safety-module", "Adds safety-critical checks.");
+
+} // namespace safety
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the SafetyModule.
+volatile int SafetyModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/tool/CMakeLists.txt

   clangTidyMPIModule
   clangTidyPerformanceModule
   clangTidyReadabilityModule
+  clangTidySafetyModule
   clangTooling
   )
 

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

 static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination =
     ReadabilityModuleAnchorSource;
 
+// This anchor is used to force the linker to link the SafetyModule.
+extern volatile int SafetyModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED SafetyModuleAnchorDestination =
+    SafetyModuleAnchorSource;
+
 } // namespace tidy
 } // namespace clang
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release
+   safety-no-vector-bool

clang-tools-extra/docs/clang-tidy/checks/safety-no-vector-bool.rst

+.. title:: clang-tidy - safety-no-vector-bool
+
+safety-no-vector-bool
+=====================
+
+``std::vector<bool>`` cannot be accessed safely concurrently in the same way as
+other ``std::vector`` specializations and is banned by some
+safefty-critical-c++ standards like High Integrity C++.
+
+This check finds instances of ``std::vector<bool>``. No fix is offered.

clang-tools-extra/test/clang-tidy/safety-no-vector-bool.cpp

+// RUN: %check_clang_tidy %s safety-no-vector-bool %t
+
+namespace std
+{
+  template <class T> 
+  class allocator {};
+
+  template <class T, class A=allocator<T>>
+  class vector {};
+}
+
+std::vector<bool> v0;
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: variable 'v0' is an instance of std::vector<bool> [safety-no-vector-bool]
+
+std::vector<bool> vb();
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: function 'vb' returns an instance of std::vector<bool> [safety-no-vector-bool]
+
+class C {
+  std::vector<bool> v_;
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: member 'v_' is an instance of std::vector<bool> [safety-no-vector-bool]
+};
+
+typedef bool BoolTypedef;
+std::vector<BoolTypedef> v1;
+// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: variable 'v1' is an instance of std::vector<bool> [safety-no-vector-bool]
+
+using vbool = std::vector<bool>;
+vbool v2;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'v2' is an instance of std::vector<bool> [safety-no-vector-bool]
+
+std::vector<int> v3;  
+
+template <class T>
+class user_allocator {};
+
+std::vector<bool, user_allocator<bool>> v4;  
+