[clang-tidy] Fix invalid read on destruction

[nik]

Summary

...in case the clang tidy plugin is linked into the clang binary.

Valgrind's memcheck reports:

8949== Invalid read ==8866== Invalid read of size 4

8866== at 0x164D248B: fetch_sub (atomic_base.h:524)

8866== by 0x164D248B: llvm::ThreadSafeRefCountedBase<clang::ast_matchers::internal::DynMatcherInterface>::Release() const (IntrusiveRefCntPtr.h:98)

8866== by 0x164CE16C: llvm::IntrusiveRefCntPtrInfo<clang::ast_matchers::internal::DynMatcherInterface>::release(clang::ast_matchers::internal::DynMatcherInterface*) (IntrusiveRefCntPtr.h:127)

8866== by 0x164C8D5C: llvm::IntrusiveRefCntPtr<clang::ast_matchers::internal::DynMatcherInterface>::release() (IntrusiveRefCntPtr.h:190)

8866== by 0x164C3B87: llvm::IntrusiveRefCntPtr<clang::ast_matchers::internal::DynMatcherInterface>::~IntrusiveRefCntPtr() (IntrusiveRefCntPtr.h:157)

8866== by 0x164BB4F1: clang::ast_matchers::internal::DynTypedMatcher::~DynTypedMatcher() (ASTMatchersInternal.h:341)

8866== by 0x164BB529: clang::ast_matchers::internal::Matcher<clang::QualType>::~Matcher() (ASTMatchersInternal.h:496)

8866== by 0xD7AE614: __cxa_finalize (cxa_finalize.c:83)

8866== by 0x164B3082: ??? (in /d2/llvm/8/qtc/builds/DebugShared/lib/libclangTidyModernizeModule.so.8)

8866== by 0x4010B72: _dl_fini (dl-fini.c:138)

8866== by 0xD7AE040: __run_exit_handlers (exit.c:108)

8866== by 0xD7AE139: exit (exit.c:139)

8866== by 0xD78CB9D: (below main) (libc-start.c:344)

8866== Address 0x19dd9bc8 is 8 bytes inside a block of size 16 free'd

8866== at 0x4C3123B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)

8866== by 0x1469BB99: clang::ast_matchers::internal::(anonymous namespace)::TrueMatcherImpl::~TrueMatcherImpl() (ASTMatchersInternal.cpp:126)

8866== by 0x1469BBC5: llvm::object_deleter<clang::ast_matchers::internal::(anonymous namespace)::TrueMatcherImpl>::call(void*) (ManagedStatic.h:30)

8866== by 0x9ABFF26: llvm::ManagedStaticBase::destroy() const (ManagedStatic.cpp:72)

8866== by 0x9ABFF94: llvm::llvm_shutdown() (ManagedStatic.cpp:84)

8866== by 0x9A65232: llvm::InitLLVM::~InitLLVM() (InitLLVM.cpp:52)

8866== by 0x14B0C8: main (driver.cpp:323)

8866== Block was alloc'd at

8866== at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)

8866== by 0x1469BB36: llvm::object_creator<clang::ast_matchers::internal::(anonymous namespace)::TrueMatcherImpl>::call() (ManagedStatic.h:24)

8866== by 0x9ABFD99: llvm::ManagedStaticBase::RegisterManagedStatic(void* (*)(), void (*)(void*)) const (ManagedStatic.cpp:42)

8866== by 0x1469B5DF: llvm::ManagedStatic<clang::ast_matchers::internal::(anonymous namespace)::TrueMatcherImpl, llvm::object_creator<clang::ast_matchers::internal::(anonymous namespace)::TrueMatcherImpl>, llvm::object_deleter<clang::ast_matchers::internal::(anonymous namespace)::TrueMatcherImpl> >::operator*() (ManagedStatic.h:67)

8866== by 0x14698F9D: clang::ast_matchers::internal::DynTypedMatcher::trueMatcher(clang::ast_type_traits::ASTNodeKind) (ASTMatchersInternal.cpp:195)

8866== by 0x164C9D3B: _ZNK5clang12ast_matchers8internal11TrueMatchercvNS1_7MatcherIT_EEINS_8QualTypeEEEv (ASTMatchersInternal.h:1247)

8866== by 0x16501458: __static_initialization_and_destruction_0(int, int) (LoopConvertCheck.cpp:48)

8866== by 0x16501976: _GLOBAL__sub_I_LoopConvertCheck.cpp (LoopConvertCheck.cpp:920)

8866== by 0x4010732: call_init (dl-init.c:72)

8866== by 0x4010732: _dl_init (dl-init.c:119)

8866== by 0x40010C9: ??? (in /lib/x86_64-linux-gnu/ld-2.27.so)

[gribozavr]

This fix works. The alternative would have been to wrap these variables into llvm::ManagedStatic, just like the problematic TrueMatcherInstance in ASTMatchersInternal.cpp.

clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp

 static const char BeginCallName[] = "beginCall";
 static const char EndCallName[] = "endCall";
 static const char ConditionEndVarName[] = "conditionEndVar";
 static const char EndVarName[] = "endVar";
 static const char DerefByValueResultName[] = "derefByValueResult";
 static const char DerefByRefResultName[] = "derefByRefResult";
 
 // shared matchers
-static const TypeMatcher AnyType = anything();
+static const TypeMatcher AnyType() { return anything(); }
 
-static const StatementMatcher IntegerComparisonMatcher =
-    expr(ignoringParenImpCasts(
-        declRefExpr(to(varDecl(hasType(isInteger())).bind(ConditionVarName)))));
+static const StatementMatcher IntegerComparisonMatcher() {
+  return expr(ignoringParenImpCasts(
+}
 
-static const DeclarationMatcher InitToZeroMatcher =
-    varDecl(hasInitializer(ignoringParenImpCasts(integerLiteral(equals(0)))))
-        .bind(InitVarName);
+static const DeclarationMatcher InitToZeroMatcher() {
+  return varDecl(
+             hasInitializer(ignoringParenImpCasts(integerLiteral(equals(0)))))
+}
 
-static const StatementMatcher IncrementVarMatcher =
-    declRefExpr(to(varDecl(hasType(isInteger())).bind(IncrementVarName)));
+static const StatementMatcher IncrementVarMatcher() {
+  return declRefExpr(to(varDecl(hasType(isInteger())).bind(IncrementVarName)));
+}
 
 /// \brief The matcher for loops over arrays.
 ///
 /// In this general example, assuming 'j' and 'k' are of integral type:
 /// \code
 ///   for (int i = 0; j < 3 + 2; ++k) { ... }
 /// \endcode
 /// The following string identifiers are bound to these parts of the AST:

 ///   - The index variable is only used as an array index.
 ///   - All arrays indexed by the loop are the same.
 StatementMatcher makeArrayLoopMatcher() {
   StatementMatcher ArrayBoundMatcher =
       expr(hasType(isInteger())).bind(ConditionBoundName);
 
   return forStmt(
              unless(isInTemplateInstantiation()),
-             hasLoopInit(declStmt(hasSingleDecl(InitToZeroMatcher))),
+             hasLoopInit(declStmt(hasSingleDecl(InitToZeroMatcher()))),
              hasCondition(anyOf(
                  binaryOperator(hasOperatorName("<"),
-                                hasLHS(IntegerComparisonMatcher),
+                                hasLHS(IntegerComparisonMatcher()),
                                 hasRHS(ArrayBoundMatcher)),
                  binaryOperator(hasOperatorName(">"), hasLHS(ArrayBoundMatcher),
-                                hasRHS(IntegerComparisonMatcher)))),
+                                hasRHS(IntegerComparisonMatcher())))),
              hasIncrement(unaryOperator(hasOperatorName("++"),
-                                        hasUnaryOperand(IncrementVarMatcher))))
+                                        hasUnaryOperand(IncrementVarMatcher()))))
       .bind(LoopNameArray);
 }
 
 /// \brief The matcher used for iterator-based for loops.
 ///
 /// This matcher is more flexible than array-based loops. It will match
 /// catch loops of the following textual forms (regardless of whether the
 /// iterator type is actually a pointer type or a class type):
return forStmt(
                                       hasRHS(IteratorBoundMatcher)),
                        binaryOperator(hasOperatorName("!="),
                                       hasLHS(IteratorBoundMatcher),
                                       hasRHS(IteratorComparisonMatcher)),
                        OverloadedNEQMatcher)),
              hasIncrement(anyOf(
                  unaryOperator(hasOperatorName("++"),
                                hasUnaryOperand(declRefExpr(
-                                   to(varDecl(hasType(pointsTo(AnyType)))
+                                   to(varDecl(hasType(pointsTo(AnyType())))
                                           .bind(IncrementVarName))))),
                  cxxOperatorCallExpr(
                      hasOverloadedOperatorName("++"),
                      hasArgument(
                          0, declRefExpr(to(varDecl(TestDerefReturnsByValue)
                                                .bind(IncrementVarName))))))))
       .bind(LoopNameIterator);
 }
StatementMatcher IndexBoundMatcher =
       expr(anyOf(ignoringParenImpCasts(declRefExpr(to(
                      varDecl(hasType(isInteger())).bind(ConditionEndVarName)))),
                  EndInitMatcher));
 
   return forStmt(
              unless(isInTemplateInstantiation()),
              hasLoopInit(
                  anyOf(declStmt(declCountIs(2),
-                                containsDeclaration(0, InitToZeroMatcher),
+                                containsDeclaration(0, InitToZeroMatcher()),
                                 containsDeclaration(1, EndDeclMatcher)),
-                       declStmt(hasSingleDecl(InitToZeroMatcher)))),
+                       declStmt(hasSingleDecl(InitToZeroMatcher())))),
              hasCondition(anyOf(
                  binaryOperator(hasOperatorName("<"),
-                                hasLHS(IntegerComparisonMatcher),
+                                hasLHS(IntegerComparisonMatcher()),
                                 hasRHS(IndexBoundMatcher)),
                  binaryOperator(hasOperatorName(">"), hasLHS(IndexBoundMatcher),
-                                hasRHS(IntegerComparisonMatcher)))),
+                                hasRHS(IntegerComparisonMatcher())))),
              hasIncrement(unaryOperator(hasOperatorName("++"),
-                                        hasUnaryOperand(IncrementVarMatcher))))
+                                        hasUnaryOperand(IncrementVarMatcher()))))
       .bind(LoopNamePseudoArray);
 }
 
 /// \brief Determine whether Init appears to be an initializing an iterator.
 ///
 /// If it is, returns the object whose begin() or end() method is called, and
 /// the output parameter isArrow is set to indicate whether the initialization
 /// is called via . or ->.