Index: clang-tidy/android/AndroidTidyModule.cpp =================================================================== --- clang-tidy/android/AndroidTidyModule.cpp +++ clang-tidy/android/AndroidTidyModule.cpp @@ -10,8 +10,16 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "CloexecAccept4Check.h" +#include "CloexecAcceptCheck.h" #include "CloexecCreatCheck.h" +#include "CloexecEpollCreate1Check.h" +#include "CloexecEpollCreateCheck.h" +#include "CloexecDupCheck.h" #include "CloexecFopenCheck.h" +#include "CloexecInotifyInit1Check.h" +#include "CloexecInotifyInitCheck.h" +#include "CloexecMemfdCreateCheck.h" #include "CloexecOpenCheck.h" #include "CloexecSocketCheck.h" @@ -25,8 +33,21 @@ class AndroidModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck("android-cloexec-accept4"); + CheckFactories.registerCheck("android-cloexec-accept"); CheckFactories.registerCheck("android-cloexec-creat"); + CheckFactories.registerCheck( + "android-cloexec-epoll-create1"); + CheckFactories.registerCheck( + "android-cloexec-epoll-create"); + CheckFactories.registerCheck("android-cloexec-dup"); CheckFactories.registerCheck("android-cloexec-fopen"); + CheckFactories.registerCheck( + "android-cloexec-inotify-init"); + CheckFactories.registerCheck( + "android-cloexec-inotify-init1"); + CheckFactories.registerCheck( + "android-cloexec-memfd-create"); CheckFactories.registerCheck("android-cloexec-open"); CheckFactories.registerCheck("android-cloexec-socket"); } Index: clang-tidy/android/CMakeLists.txt =================================================================== --- clang-tidy/android/CMakeLists.txt +++ clang-tidy/android/CMakeLists.txt @@ -2,8 +2,17 @@ add_clang_library(clangTidyAndroidModule AndroidTidyModule.cpp + CloexecAccept4Check.cpp + CloexecAcceptCheck.cpp + CloexecCheck.cpp CloexecCreatCheck.cpp + CloexecEpollCreate1Check.cpp + CloexecEpollCreateCheck.cpp + CloexecDupCheck.cpp CloexecFopenCheck.cpp + CloexecInotifyInit1Check.cpp + CloexecInotifyInitCheck.cpp + CloexecMemfdCreateCheck.cpp CloexecOpenCheck.cpp CloexecSocketCheck.cpp Index: clang-tidy/android/CloexecAccept4Check.h =================================================================== --- clang-tidy/android/CloexecAccept4Check.h +++ clang-tidy/android/CloexecAccept4Check.h @@ -1,4 +1,4 @@ -//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===// +//===--- CloexecAccept4Check.h - clang-tidy----------------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,23 +7,23 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_ACCEPT4_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_ACCEPT4_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { namespace android { -/// Finds code that uses socket() without using the SOCK_CLOEXEC flag. +/// Finds code that uses accept4() without using the SOCK_CLOEXEC flag. /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-accept4.html +class CloexecAccept4Check : public CloexecCheck { public: - CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + CloexecAccept4Check(StringRef Name, ClangTidyContext *Context) + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; @@ -32,4 +32,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_ACCEPT4_H Index: clang-tidy/android/CloexecAccept4Check.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecAccept4Check.cpp @@ -0,0 +1,40 @@ +//===--- CloexecAccept4Check.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 "CloexecAccept4Check.h" +#include "../utils/ASTUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +void CloexecAccept4Check::registerMatchers(MatchFinder *Finder) { + auto SockAddrPointerType = + hasType(pointsTo(recordDecl(isStruct(), hasName("sockaddr")))); + auto SockLenPointerType = hasType(pointsTo(namedDecl(hasName("socklen_t")))); + + registerMatchersImpl(Finder, + functionDecl(returns(isInteger()), hasName("accept4"), + hasParameter(0, hasType(isInteger())), + hasParameter(1, SockAddrPointerType), + hasParameter(2, SockLenPointerType), + hasParameter(3, hasType(isInteger())))); +} + +void CloexecAccept4Check::check(const MatchFinder::MatchResult &Result) { + insertMacroFlag(Result, /*MarcoFlag=*/"SOCK_CLOEXEC", /*ArgPos=*/3); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecAcceptCheck.h =================================================================== --- clang-tidy/android/CloexecAcceptCheck.h +++ clang-tidy/android/CloexecAcceptCheck.h @@ -1,4 +1,4 @@ -//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===// +//===--- CloexecAcceptCheck.h - clang-tidy-----------------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,23 +7,23 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_ACCEPT_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_ACCEPT_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { namespace android { -/// Finds code that uses socket() without using the SOCK_CLOEXEC flag. +/// accept() is better to be replaced by accept4(). /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-accept.html +class CloexecAcceptCheck : public CloexecCheck { public: - CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + CloexecAcceptCheck(StringRef Name, ClangTidyContext *Context) + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; @@ -32,4 +32,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_ACCEPT_H Index: clang-tidy/android/CloexecAcceptCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecAcceptCheck.cpp @@ -0,0 +1,47 @@ +//===--- CloexecAcceptCheck.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 "CloexecAcceptCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +void CloexecAcceptCheck::registerMatchers(MatchFinder *Finder) { + auto SockAddrPointerType = + hasType(pointsTo(recordDecl(isStruct(), hasName("sockaddr")))); + auto SockLenPointerType = hasType(pointsTo(namedDecl(hasName("socklen_t")))); + + registerMatchersImpl(Finder, + functionDecl(returns(isInteger()), hasName("accept"), + hasParameter(0, hasType(isInteger())), + hasParameter(1, SockAddrPointerType), + hasParameter(2, SockLenPointerType))); +} + +void CloexecAcceptCheck::check(const MatchFinder::MatchResult &Result) { + const std::string &ReplacementText = + (Twine("accept4(") + getSpellingArg(Result, 0) + ", " + + getSpellingArg(Result, 1) + ", " + getSpellingArg(Result, 2) + + ", SOCK_CLOEXEC)") + .str(); + + replaceFunc( + Result, + "prefer accept4() to accept() because accept4() allows SOCK_CLOEXEC", + ReplacementText); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecCheck.h =================================================================== --- /dev/null +++ clang-tidy/android/CloexecCheck.h @@ -0,0 +1,105 @@ +//===--- CloexecCheck.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. +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// This file contains the declaration of the CloexecCheck class, which is the +/// base class for all of the close-on-exec checks in Android module. +/// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace android { + +/// \brief The base class for all close-on-exec checks in Android module. +/// To be specific, there are some functions that need the close-on-exec flag to +/// prevent the file descriptor leakage on fork+exec and this class provides +/// utilities to identify and fix these C functions. +class CloexecCheck : public ClangTidyCheck { +public: + CloexecCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + +protected: + void + registerMatchersImpl(ast_matchers::MatchFinder *Finder, + ast_matchers::internal::Matcher Function); + + /// Currently, we have three types of fixes. + /// + /// Type1 is to insert the necessary macro flag in the flag argument. For + /// example, 'O_CLOEXEC' is required in function 'open()', so + /// \code + /// open(file, O_RDONLY); + /// \endcode + /// should be + /// \code + /// open(file, O_RDONLY | O_CLOEXE); + /// \endcode + /// + /// \param [out] Result MatchResult from AST matcher. + /// \param MacroFlag The macro name of the flag. + /// \param ArgPos The 0-based position of the flag argument. + void insertMacroFlag(const ast_matchers::MatchFinder::MatchResult &Result, + StringRef MacroFlag, int ArgPos); + + /// Type2 is to replace the API to another function that has required the + /// ability. For example: + /// \code + /// creat(path, mode); + /// \endcode + /// should be + /// \code + /// open(path, O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, mode) + /// \endcode + /// + /// \param [out] Result MatchResult from AST matcher. + /// \param WarningMsg The warning message. + /// \param FixMsg The fix message. + void replaceFunc(const ast_matchers::MatchFinder::MatchResult &Result, + StringRef WarningMsg, StringRef FixMsg); + + /// Type3 is also to add a flag to the corresponding argument, but this time, + /// the flag is some string and each char represents a mode rather than a + /// macro. For example, 'fopen' needs char 'e' in its mode argument string, so + /// \code + /// fopen(in_file, "r"); + /// \endcode + /// should be + /// \code + /// fopen(in_file, "re"); + /// \endcode + /// + /// \param [out] Result MatchResult from AST matcher. + /// \param Mode The required mode char. + /// \param ArgPos The 0-based position of the flag argument. + void insertStringFlag(const ast_matchers::MatchFinder::MatchResult &Result, + const char Mode, const int ArgPos); + + /// Helper function to get the spelling of a particular argument. + StringRef getSpellingArg(const ast_matchers::MatchFinder::MatchResult &Result, + int N) const; + + /// Binding name of the FuncDecl of a function call. + static const char *FuncDeclBindingStr; + + /// Binding name of the function call expression. + static const char *FuncBindingStr; +}; + +} // namespace android +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_H Index: clang-tidy/android/CloexecCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecCheck.cpp @@ -0,0 +1,114 @@ +//===--- CloexecCheck.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 "CloexecCheck.h" +#include "../utils/ASTUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +namespace { +// Helper function to form the correct string mode for Type3. +// Build the replace text. If it's string constant, add directly in the +// end of the string. Else, add . +std::string buildFixMsgForStringFlag(const Expr *Arg, const SourceManager &SM, + const LangOptions &LangOpts, char Mode) { + if (Arg->getLocStart().isMacroID()) + return (Lexer::getSourceText( + CharSourceRange::getTokenRange(Arg->getSourceRange()), SM, + LangOpts) + + " \"" + Twine(Mode) + "\"") + .str(); + + StringRef SR = cast(Arg->IgnoreParenCasts())->getString(); + return ("\"" + SR + Twine(Mode) + "\"").str(); +} +} // namespace + +const char *CloexecCheck::FuncDeclBindingStr = "funcDecl"; + +const char *CloexecCheck::FuncBindingStr ="func"; + +void CloexecCheck::registerMatchersImpl( + MatchFinder *Finder, internal::Matcher Function) { + // We assume all the checked APIs are C functions. + Finder->addMatcher( + callExpr( + callee(functionDecl(isExternC(), Function).bind(FuncDeclBindingStr))) + .bind(FuncBindingStr), + this); +} + +void CloexecCheck::insertMacroFlag(const MatchFinder::MatchResult &Result, + StringRef MacroFlag, int ArgPos) { + const auto *MatchedCall = Result.Nodes.getNodeAs(FuncBindingStr); + const auto *FlagArg = MatchedCall->getArg(ArgPos); + const auto *FD = Result.Nodes.getNodeAs(FuncDeclBindingStr); + SourceManager &SM = *Result.SourceManager; + + if (utils::exprHasBitFlagWithSpelling(FlagArg->IgnoreParenCasts(), SM, + Result.Context->getLangOpts(), + MacroFlag)) + return; + + SourceLocation EndLoc = + Lexer::getLocForEndOfToken(SM.getFileLoc(FlagArg->getLocEnd()), 0, SM, + Result.Context->getLangOpts()); + + diag(EndLoc, "%0 should use %1 where possible") + << FD << MacroFlag + << FixItHint::CreateInsertion(EndLoc, (Twine(" | ") + MacroFlag).str()); +} + +void CloexecCheck::replaceFunc(const MatchFinder::MatchResult &Result, + StringRef WarningMsg, StringRef FixMsg) { + const auto *MatchedCall = Result.Nodes.getNodeAs(FuncBindingStr); + diag(MatchedCall->getLocStart(), WarningMsg) + << FixItHint::CreateReplacement(MatchedCall->getSourceRange(), FixMsg); +} + +void CloexecCheck::insertStringFlag( + const ast_matchers::MatchFinder::MatchResult &Result, const char Mode, + const int ArgPos) { + const auto *MatchedCall = Result.Nodes.getNodeAs(FuncBindingStr); + const auto *FD = Result.Nodes.getNodeAs(FuncDeclBindingStr); + const auto *ModeArg = MatchedCall->getArg(ArgPos); + + // Check if the may be in the mode string. + const auto *ModeStr = dyn_cast(ModeArg->IgnoreParenCasts()); + if (!ModeStr || (ModeStr->getString().find(Mode) != StringRef::npos)) + return; + + const std::string &ReplacementText = buildFixMsgForStringFlag( + ModeArg, *Result.SourceManager, Result.Context->getLangOpts(), Mode); + + diag(ModeArg->getLocStart(), "use %0 mode '%1' to set O_CLOEXEC") + << FD << std::string(1, Mode) + << FixItHint::CreateReplacement(ModeArg->getSourceRange(), + ReplacementText); +} + +StringRef CloexecCheck::getSpellingArg(const MatchFinder::MatchResult &Result, + int N) const { + const auto *MatchedCall = Result.Nodes.getNodeAs(FuncBindingStr); + const SourceManager &SM = *Result.SourceManager; + return Lexer::getSourceText( + CharSourceRange::getTokenRange(MatchedCall->getArg(N)->getSourceRange()), + SM, Result.Context->getLangOpts()); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecCreatCheck.h =================================================================== --- clang-tidy/android/CloexecCreatCheck.h +++ clang-tidy/android/CloexecCreatCheck.h @@ -10,7 +10,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_CREAT_H #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_CREAT_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { @@ -20,10 +20,10 @@ /// Find the usage of creat() and redirect user to use open(). /// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-creat.html -class CloexecCreatCheck : public ClangTidyCheck { +class CloexecCreatCheck : public CloexecCheck { public: CloexecCreatCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; Index: clang-tidy/android/CloexecCreatCheck.cpp =================================================================== --- clang-tidy/android/CloexecCreatCheck.cpp +++ clang-tidy/android/CloexecCreatCheck.cpp @@ -10,7 +10,6 @@ #include "CloexecCreatCheck.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; @@ -21,37 +20,22 @@ void CloexecCreatCheck::registerMatchers(MatchFinder *Finder) { auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter()))); auto MODETType = hasType(namedDecl(hasName("mode_t"))); - - Finder->addMatcher( - callExpr(callee(functionDecl(isExternC(), returns(isInteger()), - hasName("creat"), - hasParameter(0, CharPointerType), - hasParameter(1, MODETType)) - .bind("funcDecl"))) - .bind("creatFn"), - this); + registerMatchersImpl(Finder, + functionDecl(isExternC(), returns(isInteger()), + hasName("creat"), + hasParameter(0, CharPointerType), + hasParameter(1, MODETType))); } void CloexecCreatCheck::check(const MatchFinder::MatchResult &Result) { - const auto *MatchedCall = Result.Nodes.getNodeAs("creatFn"); - const SourceManager &SM = *Result.SourceManager; - const std::string &ReplacementText = - (Twine("open (") + - Lexer::getSourceText(CharSourceRange::getTokenRange( - MatchedCall->getArg(0)->getSourceRange()), - SM, Result.Context->getLangOpts()) + + (Twine("open (") + getSpellingArg(Result, 0) + ", O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, " + - Lexer::getSourceText(CharSourceRange::getTokenRange( - MatchedCall->getArg(1)->getSourceRange()), - SM, Result.Context->getLangOpts()) + - ")") + getSpellingArg(Result, 1) + ")") .str(); - - diag(MatchedCall->getLocStart(), - "prefer open() to creat() because open() allows O_CLOEXEC") - << FixItHint::CreateReplacement(MatchedCall->getSourceRange(), - ReplacementText); + replaceFunc(Result, + "prefer open() to creat() because open() allows O_CLOEXEC", + ReplacementText); } } // namespace android Index: clang-tidy/android/CloexecDupCheck.h =================================================================== --- clang-tidy/android/CloexecDupCheck.h +++ clang-tidy/android/CloexecDupCheck.h @@ -1,4 +1,4 @@ -//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===// +//===--- CloexecDupCheck.h - clang-tidy-------------------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,23 +7,24 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_DUP_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_DUP_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { namespace android { -/// Finds code that uses socket() without using the SOCK_CLOEXEC flag. +/// dup() is better to be replaced by fcntl(), which has close-on-exec flag. +/// Find the usage of dup() and redirect user to use fcntl(). /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-dup.html +class CloexecDupCheck : public CloexecCheck { public: - CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + CloexecDupCheck(StringRef Name, ClangTidyContext *Context) + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; @@ -32,4 +33,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_DUP_H Index: clang-tidy/android/CloexecDupCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecDupCheck.cpp @@ -0,0 +1,38 @@ +//===--- CloexecDupCheck.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 "CloexecDupCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +void CloexecDupCheck::registerMatchers(MatchFinder *Finder) { + registerMatchersImpl(Finder, + functionDecl(returns(isInteger()), hasName("dup"), + hasParameter(0, hasType(isInteger())))); +} + +void CloexecDupCheck::check(const MatchFinder::MatchResult &Result) { + const std::string &ReplacementText = + (Twine("fcntl(") + getSpellingArg(Result, 0) + ", F_DUPFD_CLOEXEC)") + .str(); + + replaceFunc(Result, + "prefer fcntl() to dup() because fcntl() allows F_DUPFD_CLOEXEC", + ReplacementText); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecEpollCreate1Check.h =================================================================== --- clang-tidy/android/CloexecEpollCreate1Check.h +++ clang-tidy/android/CloexecEpollCreate1Check.h @@ -1,4 +1,4 @@ -//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===// +//===--- CloexecEpollCreate1Check.h - clang-tidy-----------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,23 +7,23 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_EPOLL_CREATE1_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_EPOLL_CREATE1_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { namespace android { -/// Finds code that uses socket() without using the SOCK_CLOEXEC flag. +/// Finds code that uses epoll_create1() without using the EPOLL_CLOEXEC flag. /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-epoll-create1.html +class CloexecEpollCreate1Check : public CloexecCheck { public: - CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + CloexecEpollCreate1Check(StringRef Name, ClangTidyContext *Context) + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; @@ -32,4 +32,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_EPOLL_CREATE1_H Index: clang-tidy/android/CloexecEpollCreate1Check.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecEpollCreate1Check.cpp @@ -0,0 +1,33 @@ +//===--- CloexecEpollCreate1Check.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 "CloexecEpollCreate1Check.h" +#include "../utils/ASTUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +void CloexecEpollCreate1Check::registerMatchers(MatchFinder *Finder) { + registerMatchersImpl( + Finder, functionDecl(returns(isInteger()), hasName("epoll_create1"), + hasParameter(0, hasType(isInteger())))); +} + +void CloexecEpollCreate1Check::check(const MatchFinder::MatchResult &Result) { + insertMacroFlag(Result, /*MarcoFlag=*/"EPOLL_CLOEXEC", /*ArgPos=*/0); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecEpollCreateCheck.h =================================================================== --- clang-tidy/android/CloexecEpollCreateCheck.h +++ clang-tidy/android/CloexecEpollCreateCheck.h @@ -1,4 +1,4 @@ -//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===// +//===--- CloexecEpollCreateCheck.h - clang-tidy------------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,23 +7,23 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_EPOLL_CREATE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_EPOLL_CREATE_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { namespace android { -/// Finds code that uses socket() without using the SOCK_CLOEXEC flag. +/// epoll_create() is better to be replaced by epoll_create1(). /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-epoll-create.html +class CloexecEpollCreateCheck : public CloexecCheck { public: - CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + CloexecEpollCreateCheck(StringRef Name, ClangTidyContext *Context) + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; @@ -32,4 +32,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_EPOLL_CREATE_H Index: clang-tidy/android/CloexecEpollCreateCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecEpollCreateCheck.cpp @@ -0,0 +1,36 @@ +//===--- CloexecEpollCreateCheck.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 "CloexecEpollCreateCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +void CloexecEpollCreateCheck::registerMatchers(MatchFinder *Finder) { + registerMatchersImpl( + Finder, functionDecl(returns(isInteger()), hasName("epoll_create"), + hasParameter(0, hasType(isInteger())))); +} + +void CloexecEpollCreateCheck::check(const MatchFinder::MatchResult &Result) { + replaceFunc(Result, + "prefer epoll_create() to epoll_create1() " + "because epoll_create1() allows " + "EPOLL_CLOEXEC", + "epoll_create1(EPOLL_CLOEXEC)"); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecFopenCheck.h =================================================================== --- clang-tidy/android/CloexecFopenCheck.h +++ clang-tidy/android/CloexecFopenCheck.h @@ -10,7 +10,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_FOPEN_H #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_FOPEN_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { @@ -23,10 +23,10 @@ /// constant propagation. /// /// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-fopen.html -class CloexecFopenCheck : public ClangTidyCheck { +class CloexecFopenCheck : public CloexecCheck { public: CloexecFopenCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; Index: clang-tidy/android/CloexecFopenCheck.cpp =================================================================== --- clang-tidy/android/CloexecFopenCheck.cpp +++ clang-tidy/android/CloexecFopenCheck.cpp @@ -18,55 +18,17 @@ namespace tidy { namespace android { -namespace { -static const char MODE = 'e'; - -// Build the replace text. If it's string constant, add 'e' directly in the end -// of the string. Else, add "e". -std::string BuildReplaceText(const Expr *Arg, const SourceManager &SM, - const LangOptions &LangOpts) { - if (Arg->getLocStart().isMacroID()) - return (Lexer::getSourceText( - CharSourceRange::getTokenRange(Arg->getSourceRange()), SM, - LangOpts) + - " \"" + Twine(MODE) + "\"") - .str(); - - StringRef SR = cast(Arg->IgnoreParenCasts())->getString(); - return ("\"" + SR + Twine(MODE) + "\"").str(); -} -} // namespace - void CloexecFopenCheck::registerMatchers(MatchFinder *Finder) { auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter()))); - - Finder->addMatcher( - callExpr(callee(functionDecl(isExternC(), returns(asString("FILE *")), - hasName("fopen"), - hasParameter(0, CharPointerType), - hasParameter(1, CharPointerType)) - .bind("funcDecl"))) - .bind("fopenFn"), - this); + registerMatchersImpl(Finder, + functionDecl(isExternC(), returns(asString("FILE *")), + hasName("fopen"), + hasParameter(0, CharPointerType), + hasParameter(1, CharPointerType))); } void CloexecFopenCheck::check(const MatchFinder::MatchResult &Result) { - const auto *MatchedCall = Result.Nodes.getNodeAs("fopenFn"); - const auto *FD = Result.Nodes.getNodeAs("funcDecl"); - const Expr *ModeArg = MatchedCall->getArg(1); - - // Check if the 'e' may be in the mode string. - const auto *ModeStr = dyn_cast(ModeArg->IgnoreParenCasts()); - if (!ModeStr || (ModeStr->getString().find(MODE) != StringRef::npos)) - return; - - const std::string &ReplacementText = BuildReplaceText( - ModeArg, *Result.SourceManager, Result.Context->getLangOpts()); - - diag(ModeArg->getLocStart(), "use %0 mode 'e' to set O_CLOEXEC") - << FD - << FixItHint::CreateReplacement(ModeArg->getSourceRange(), - ReplacementText); + insertStringFlag(Result, /*Mode=*/'e', /*ArgPos=*/1); } } // namespace android Index: clang-tidy/android/CloexecInotifyInit1Check.h =================================================================== --- clang-tidy/android/CloexecInotifyInit1Check.h +++ clang-tidy/android/CloexecInotifyInit1Check.h @@ -1,4 +1,4 @@ -//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===// +//===--- CloexecInotifyInit1Check.h - clang-tidy-----------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,23 +7,23 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_INOTIFY_INIT1_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_INOTIFY_INIT1_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { namespace android { -/// Finds code that uses socket() without using the SOCK_CLOEXEC flag. +/// Finds code that uses inotify_init1() without using the IN_CLOEXEC flag. /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-inotify-init1.html +class CloexecInotifyInit1Check : public CloexecCheck { public: - CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + CloexecInotifyInit1Check(StringRef Name, ClangTidyContext *Context) + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; @@ -32,4 +32,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_INOTIFY_INIT1_H Index: clang-tidy/android/CloexecInotifyInit1Check.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecInotifyInit1Check.cpp @@ -0,0 +1,33 @@ +//===--- CloexecInotifyInit1Check.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 "CloexecInotifyInit1Check.h" +#include "../utils/ASTUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +void CloexecInotifyInit1Check::registerMatchers(MatchFinder *Finder) { + registerMatchersImpl( + Finder, functionDecl(returns(isInteger()), hasName("inotify_init1"), + hasParameter(0, hasType(isInteger())))); +} + +void CloexecInotifyInit1Check::check(const MatchFinder::MatchResult &Result) { + insertMacroFlag(Result, /*MarcoFlag=*/"IN_CLOEXEC", /*ArgPos=*/0); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecInotifyInitCheck.h =================================================================== --- clang-tidy/android/CloexecInotifyInitCheck.h +++ clang-tidy/android/CloexecInotifyInitCheck.h @@ -1,4 +1,4 @@ -//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===// +//===--- CloexecInotifyInitCheck.h - clang-tidy------------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,23 +7,23 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_INOTIFY_INIT_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_INOTIFY_INIT_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { namespace android { -/// Finds code that uses socket() without using the SOCK_CLOEXEC flag. +/// inotify_init() is better to be replaced by inotify_init1(). /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-inotify-init.html +class CloexecInotifyInitCheck : public CloexecCheck { public: - CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + CloexecInotifyInitCheck(StringRef Name, ClangTidyContext *Context) + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; @@ -32,4 +32,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_INOTIFY_INIT_H Index: clang-tidy/android/CloexecInotifyInitCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecInotifyInitCheck.cpp @@ -0,0 +1,34 @@ +//===--- CloexecInotifyInitCheck.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 "CloexecInotifyInitCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +void CloexecInotifyInitCheck::registerMatchers(MatchFinder *Finder) { + registerMatchersImpl( + Finder, functionDecl(returns(isInteger()), hasName("inotify_init"))); +} + +void CloexecInotifyInitCheck::check(const MatchFinder::MatchResult &Result) { + replaceFunc(Result, /*WarningMsg=*/ + "prefer inotify_init() to inotify_init1() " + "because inotify_init1() allows IN_CLOEXEC", + /*FixMsg=*/"inotify_init1(IN_CLOEXEC)"); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecMemfdCreateCheck.h =================================================================== --- clang-tidy/android/CloexecMemfdCreateCheck.h +++ clang-tidy/android/CloexecMemfdCreateCheck.h @@ -1,4 +1,4 @@ -//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===// +//===--- CloexecMemfdCreateCheck.h - clang-tidy-----------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,23 +7,23 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_MEMFD_CREATE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_MEMFD_CREATE_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { namespace android { -/// Finds code that uses socket() without using the SOCK_CLOEXEC flag. +/// Finds code that uses memfd_create() without using the MFD_CLOEXEC flag. /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-memfd-create.html +class CloexecMemfdCreateCheck : public CloexecCheck { public: - CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + CloexecMemfdCreateCheck(StringRef Name, ClangTidyContext *Context) + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; @@ -32,4 +32,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_MEMFD_CREATE_H Index: clang-tidy/android/CloexecMemfdCreateCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/android/CloexecMemfdCreateCheck.cpp @@ -0,0 +1,32 @@ +//===--- CloexecMemfdCreateCheck.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 "CloexecMemfdCreateCheck.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace android { + +void CloexecMemfdCreateCheck::registerMatchers(MatchFinder *Finder) { + auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter()))); + registerMatchersImpl( + Finder, functionDecl(returns(isInteger()), hasName("memfd_create"), + hasParameter(0, CharPointerType), + hasParameter(1, hasType(isInteger())))); +} + +void CloexecMemfdCreateCheck::check(const MatchFinder::MatchResult &Result) { + insertMacroFlag(Result, "MFD_CLOEXEC", /*ArgPos=*/1); +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tidy/android/CloexecOpenCheck.h =================================================================== --- clang-tidy/android/CloexecOpenCheck.h +++ clang-tidy/android/CloexecOpenCheck.h @@ -10,7 +10,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_OPEN_H #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_OPEN_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { @@ -25,10 +25,10 @@ /// /// Only the symbolic 'O_CLOEXEC' macro definition is checked, not the concrete /// value. -class CloexecOpenCheck : public ClangTidyCheck { +class CloexecOpenCheck : public CloexecCheck { public: CloexecOpenCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; Index: clang-tidy/android/CloexecOpenCheck.cpp =================================================================== --- clang-tidy/android/CloexecOpenCheck.cpp +++ clang-tidy/android/CloexecOpenCheck.cpp @@ -8,10 +8,8 @@ //===----------------------------------------------------------------------===// #include "CloexecOpenCheck.h" -#include "../utils/ASTUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; @@ -19,54 +17,26 @@ namespace tidy { namespace android { -static constexpr const char *O_CLOEXEC = "O_CLOEXEC"; - void CloexecOpenCheck::registerMatchers(MatchFinder *Finder) { auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter()))); - - Finder->addMatcher( - callExpr(callee(functionDecl(isExternC(), returns(isInteger()), - hasAnyName("open", "open64"), - hasParameter(0, CharPointerType), - hasParameter(1, hasType(isInteger()))) - .bind("funcDecl"))) - .bind("openFn"), - this); - Finder->addMatcher( - callExpr(callee(functionDecl(isExternC(), returns(isInteger()), - hasName("openat"), - hasParameter(0, hasType(isInteger())), - hasParameter(1, CharPointerType), - hasParameter(2, hasType(isInteger()))) - .bind("funcDecl"))) - .bind("openatFn"), - this); + registerMatchersImpl(Finder, + functionDecl(isExternC(), returns(isInteger()), + hasAnyName("open", "open64"), + hasParameter(0, CharPointerType), + hasParameter(1, hasType(isInteger())))); + registerMatchersImpl(Finder, + functionDecl(isExternC(), returns(isInteger()), + hasName("openat"), + hasParameter(0, hasType(isInteger())), + hasParameter(1, CharPointerType), + hasParameter(2, hasType(isInteger())))); } void CloexecOpenCheck::check(const MatchFinder::MatchResult &Result) { - const Expr *FlagArg = nullptr; - if (const auto *OpenFnCall = Result.Nodes.getNodeAs("openFn")) - FlagArg = OpenFnCall->getArg(1); - else if (const auto *OpenFnCall = - Result.Nodes.getNodeAs("openatFn")) - FlagArg = OpenFnCall->getArg(2); - assert(FlagArg); - - const auto *FD = Result.Nodes.getNodeAs("funcDecl"); - - // Check the required flag. - SourceManager &SM = *Result.SourceManager; - if (utils::exprHasBitFlagWithSpelling(FlagArg->IgnoreParenCasts(), SM, - Result.Context->getLangOpts(), O_CLOEXEC)) - return; - - SourceLocation EndLoc = - Lexer::getLocForEndOfToken(SM.getFileLoc(FlagArg->getLocEnd()), 0, SM, - Result.Context->getLangOpts()); - - diag(EndLoc, "%0 should use %1 where possible") - << FD << O_CLOEXEC - << FixItHint::CreateInsertion(EndLoc, (Twine(" | ") + O_CLOEXEC).str()); + const auto *FD = Result.Nodes.getNodeAs(FuncDeclBindingStr); + assert(FD->param_size() > 1); + int ArgPos = (FD->param_size() > 2) ? 2 : 1; + insertMacroFlag(Result, /*MarcoFlag=*/"O_CLOEXEC", ArgPos); } } // namespace android Index: clang-tidy/android/CloexecSocketCheck.h =================================================================== --- clang-tidy/android/CloexecSocketCheck.h +++ clang-tidy/android/CloexecSocketCheck.h @@ -10,7 +10,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H -#include "../ClangTidy.h" +#include "CloexecCheck.h" namespace clang { namespace tidy { @@ -20,10 +20,10 @@ /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html -class CloexecSocketCheck : public ClangTidyCheck { +class CloexecSocketCheck : public CloexecCheck { public: CloexecSocketCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : CloexecCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; Index: clang-tidy/android/CloexecSocketCheck.cpp =================================================================== --- clang-tidy/android/CloexecSocketCheck.cpp +++ clang-tidy/android/CloexecSocketCheck.cpp @@ -8,7 +8,6 @@ //===----------------------------------------------------------------------===// #include "CloexecSocketCheck.h" -#include "../utils/ASTUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" @@ -18,38 +17,17 @@ namespace tidy { namespace android { -static constexpr const char *SOCK_CLOEXEC = "SOCK_CLOEXEC"; - void CloexecSocketCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher( - callExpr(callee(functionDecl(isExternC(), returns(isInteger()), - hasName("socket"), - hasParameter(0, hasType(isInteger())), - hasParameter(1, hasType(isInteger())), - hasParameter(2, hasType(isInteger()))) - .bind("funcDecl"))) - .bind("socketFn"), - this); + registerMatchersImpl(Finder, + functionDecl(isExternC(), returns(isInteger()), + hasName("socket"), + hasParameter(0, hasType(isInteger())), + hasParameter(1, hasType(isInteger())), + hasParameter(2, hasType(isInteger())))); } void CloexecSocketCheck::check(const MatchFinder::MatchResult &Result) { - const auto *MatchedCall = Result.Nodes.getNodeAs("socketFn"); - const auto *FD = Result.Nodes.getNodeAs("funcDecl"); - const Expr *FlagArg = MatchedCall->getArg(1); - SourceManager &SM = *Result.SourceManager; - - if (utils::exprHasBitFlagWithSpelling(FlagArg->IgnoreParenCasts(), SM, - Result.Context->getLangOpts(), SOCK_CLOEXEC)) - return; - - SourceLocation EndLoc = - Lexer::getLocForEndOfToken(SM.getFileLoc(FlagArg->getLocEnd()), 0, SM, - Result.Context->getLangOpts()); - - diag(EndLoc, "%0 should use %1 where possible") - << FD << SOCK_CLOEXEC - << FixItHint::CreateInsertion(EndLoc, - (Twine(" | ") + SOCK_CLOEXEC).str()); + insertMacroFlag(Result, /*MarcoFlag=*/"SOCK_CLOEXEC", /*ArgPos=*/1); } } // namespace android Index: clang-tidy/hicpp/ExceptionBaseclassCheck.h =================================================================== --- clang-tidy/hicpp/ExceptionBaseclassCheck.h +++ clang-tidy/hicpp/ExceptionBaseclassCheck.h @@ -16,7 +16,7 @@ namespace tidy { namespace hicpp { -/// FIXME: Write a short description. +/// Check for thrown exceptions and enforce they are all derived from std::exception. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-exception-baseclass.html Index: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp =================================================================== --- clang-tidy/hicpp/ExceptionBaseclassCheck.cpp +++ clang-tidy/hicpp/ExceptionBaseclassCheck.cpp @@ -22,8 +22,11 @@ return; Finder->addMatcher( - cxxThrowExpr(has(expr(unless(hasType(cxxRecordDecl( - isSameOrDerivedFrom(hasName("std::exception")))))))) + cxxThrowExpr( + allOf( + has(expr(unless(hasType(cxxRecordDecl( + isSameOrDerivedFrom(hasName("std::exception"))))))), + eachOf(has(expr(hasType(namedDecl().bind("decl")))), anything()))) .bind("bad_throw"), this); } @@ -32,8 +35,12 @@ const auto *BadThrow = Result.Nodes.getNodeAs("bad_throw"); diag(BadThrow->getLocStart(), - "throwing value which type is not derived from std::exception") - << SourceRange(BadThrow->getLocStart(), BadThrow->getLocEnd()); + "throwing an exception whose type is not derived from 'std::exception'") + << BadThrow->getSourceRange(); + + const auto *TypeDecl = Result.Nodes.getNodeAs("decl"); + if (TypeDecl != nullptr) + diag(TypeDecl->getLocStart(), "type defined here", DiagnosticIDs::Note); } } // namespace hicpp Index: clang-tidy/hicpp/HICPPTidyModule.cpp =================================================================== --- clang-tidy/hicpp/HICPPTidyModule.cpp +++ clang-tidy/hicpp/HICPPTidyModule.cpp @@ -21,6 +21,7 @@ #include "../modernize/UseEqualsDefaultCheck.h" #include "../modernize/UseEqualsDeleteCheck.h" #include "../modernize/UseOverrideCheck.h" +#include "../readability/BracesAroundStatementsCheck.h" #include "../readability/FunctionSizeCheck.h" #include "../readability/IdentifierNamingCheck.h" #include "ExceptionBaseclassCheck.h" @@ -33,6 +34,8 @@ class HICPPModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "hicpp-braces-around-statements"); CheckFactories.registerCheck( "hicpp-exception-baseclass"); CheckFactories.registerCheck( Index: clang-tidy/modernize/MakeSmartPtrCheck.h =================================================================== --- clang-tidy/modernize/MakeSmartPtrCheck.h +++ clang-tidy/modernize/MakeSmartPtrCheck.h @@ -57,7 +57,8 @@ void checkReset(SourceManager &SM, const CXXMemberCallExpr *Member, const CXXNewExpr *New); - void replaceNew(DiagnosticBuilder &Diag, const CXXNewExpr *New, + /// Returns true when the fixes for replacing CXXNewExpr are generated. + bool replaceNew(DiagnosticBuilder &Diag, const CXXNewExpr *New, SourceManager &SM); void insertHeader(DiagnosticBuilder &Diag, FileID FD); }; Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp =================================================================== --- clang-tidy/modernize/MakeSmartPtrCheck.cpp +++ clang-tidy/modernize/MakeSmartPtrCheck.cpp @@ -86,7 +86,8 @@ cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType( equalsBoundNode(PointerType))))), CanCallCtor) - .bind(NewExpression))) + .bind(NewExpression)), + unless(isInTemplateInstantiation())) .bind(ConstructorCall)))), this); @@ -94,7 +95,8 @@ cxxMemberCallExpr( thisPointerType(getSmartPointerTypeMatcher()), callee(cxxMethodDecl(hasName("reset"))), - hasArgument(0, cxxNewExpr(CanCallCtor).bind(NewExpression))) + hasArgument(0, cxxNewExpr(CanCallCtor).bind(NewExpression)), + unless(isInTemplateInstantiation())) .bind(ResetCall), this); } @@ -147,6 +149,10 @@ return; } + if (!replaceNew(Diag, New, SM)) { + return; + } + // Find the location of the template's left angle. size_t LAngle = ExprStr.find("<"); SourceLocation ConstructCallEnd; @@ -179,7 +185,6 @@ ")"); } - replaceNew(Diag, New, SM); insertHeader(Diag, SM.getFileID(ConstructCallStart)); } @@ -214,6 +219,10 @@ return; } + if (!replaceNew(Diag, New, SM)) { + return; + } + Diag << FixItHint::CreateReplacement( CharSourceRange::getCharRange(OperatorLoc, ExprEnd), (llvm::Twine(" = ") + MakeSmartPtrFunctionName + "<" + @@ -223,11 +232,10 @@ if (Expr->isArrow()) Diag << FixItHint::CreateInsertion(ExprStart, "*"); - replaceNew(Diag, New, SM); insertHeader(Diag, SM.getFileID(OperatorLoc)); } -void MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag, +bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag, const CXXNewExpr *New, SourceManager& SM) { SourceLocation NewStart = New->getSourceRange().getBegin(); @@ -254,6 +262,22 @@ break; } case CXXNewExpr::CallInit: { + // FIXME: Add fixes for constructors with initializer-list parameters. + // Unlike ordinal cases, braced list can not be deduced in + // std::make_smart_ptr, we need to specify the type explicitly in the fixes: + // struct S { S(std::initializer_list, int); }; + // smart_ptr(new S({1, 2, 3}, 1)); // C++98 call-style initialization + // smart_ptr(new S({}, 1)); + // The above samples have to be replaced with: + // std::make_smart_ptr(std::initializer_list({1, 2, 3}), 1); + // std::make_smart_ptr(std::initializer_list({}), 1); + if (const auto *CE = New->getConstructExpr()) { + for (const auto *Arg : CE->arguments()) { + if (llvm::isa(Arg)) { + return false; + } + } + } if (ArraySizeExpr.empty()) { SourceRange InitRange = New->getDirectInitRange(); Diag << FixItHint::CreateRemoval( @@ -274,19 +298,16 @@ SourceRange InitRange; if (const auto *NewConstruct = New->getConstructExpr()) { if (NewConstruct->isStdInitListInitialization()) { - // Direct Initialization with the initializer-list constructor. - // struct S { S(std::initializer_list); }; - // smart_ptr(new S{1, 2, 3}); - // smart_ptr(new S{}); // use initializer-list consturctor - // The brace has to be kept, so this has to be replaced with: - // std::make_smart_ptr({1, 2, 3}); - // std::make_smart_ptr({}); - unsigned NumArgs = NewConstruct->getNumArgs(); - if (NumArgs == 0) { - return; - } - InitRange = SourceRange(NewConstruct->getArg(0)->getLocStart(), - NewConstruct->getArg(NumArgs - 1)->getLocEnd()); + // FIXME: Add fixes for direct initialization with the initializer-list + // constructor. Similar to the above CallInit case, the type has to be + // specified explicitly in the fixes. + // struct S { S(std::initializer_list); }; + // smart_ptr(new S{1, 2, 3}); // C++11 direct list-initialization + // smart_ptr(new S{}); // use initializer-list consturctor + // The above cases have to be replaced with: + // std::make_smart_ptr(std::initializer_list({1, 2, 3})); + // std::make_smart_ptr(std::initializer_list({})); + return false; } else { // Direct initialization with ordinary constructors. // struct S { S(int x); S(); }; @@ -316,6 +337,7 @@ break; } } + return true; } void MakeSmartPtrCheck::insertHeader(DiagnosticBuilder &Diag, FileID FD) { Index: clang-tidy/modernize/UseEqualsDefaultCheck.h =================================================================== --- clang-tidy/modernize/UseEqualsDefaultCheck.h +++ clang-tidy/modernize/UseEqualsDefaultCheck.h @@ -37,10 +37,13 @@ /// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html class UseEqualsDefaultCheck : public ClangTidyCheck { public: - UseEqualsDefaultCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + UseEqualsDefaultCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const bool IgnoreMacros; }; } // namespace modernize Index: clang-tidy/modernize/UseEqualsDefaultCheck.cpp =================================================================== --- clang-tidy/modernize/UseEqualsDefaultCheck.cpp +++ clang-tidy/modernize/UseEqualsDefaultCheck.cpp @@ -197,36 +197,46 @@ return !Invalid && std::strspn(Text.data(), " \t\r\n") == Text.size(); } +UseEqualsDefaultCheck::UseEqualsDefaultCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true) != 0) {} + +void UseEqualsDefaultCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IgnoreMacros", IgnoreMacros); +} + void UseEqualsDefaultCheck::registerMatchers(MatchFinder *Finder) { - if (getLangOpts().CPlusPlus) { - // Destructor. - Finder->addMatcher(cxxDestructorDecl(isDefinition()).bind(SpecialFunction), - this); - Finder->addMatcher( - cxxConstructorDecl( - isDefinition(), - anyOf( - // Default constructor. - allOf(unless(hasAnyConstructorInitializer(isWritten())), - parameterCountIs(0)), - // Copy constructor. - allOf(isCopyConstructor(), - // Discard constructors that can be used as a copy - // constructor because all the other arguments have - // default values. - parameterCountIs(1)))) - .bind(SpecialFunction), - this); - // Copy-assignment operator. - Finder->addMatcher( - cxxMethodDecl(isDefinition(), isCopyAssignmentOperator(), - // isCopyAssignmentOperator() allows the parameter to be - // passed by value, and in this case it cannot be - // defaulted. - hasParameter(0, hasType(lValueReferenceType()))) - .bind(SpecialFunction), - this); - } + if (!getLangOpts().CPlusPlus) + return; + + // Destructor. + Finder->addMatcher(cxxDestructorDecl(isDefinition()).bind(SpecialFunction), + this); + Finder->addMatcher( + cxxConstructorDecl( + isDefinition(), + anyOf( + // Default constructor. + allOf(unless(hasAnyConstructorInitializer(isWritten())), + parameterCountIs(0)), + // Copy constructor. + allOf(isCopyConstructor(), + // Discard constructors that can be used as a copy + // constructor because all the other arguments have + // default values. + parameterCountIs(1)))) + .bind(SpecialFunction), + this); + // Copy-assignment operator. + Finder->addMatcher( + cxxMethodDecl(isDefinition(), isCopyAssignmentOperator(), + // isCopyAssignmentOperator() allows the parameter to be + // passed by value, and in this case it cannot be + // defaulted. + hasParameter(0, hasType(lValueReferenceType()))) + .bind(SpecialFunction), + this); } void UseEqualsDefaultCheck::check(const MatchFinder::MatchResult &Result) { @@ -236,6 +246,9 @@ const auto *SpecialFunctionDecl = Result.Nodes.getNodeAs(SpecialFunction); + if (IgnoreMacros && SpecialFunctionDecl->getLocation().isMacroID()) + return; + // Discard explicitly deleted/defaulted special member functions and those // that are not user-provided (automatically generated). if (SpecialFunctionDecl->isDeleted() || Index: clangd/ClangdLSPServer.h =================================================================== --- clangd/ClangdLSPServer.h +++ clangd/ClangdLSPServer.h @@ -26,7 +26,7 @@ /// dispatch and ClangdServer together. class ClangdLSPServer { public: - ClangdLSPServer(JSONOutput &Out, bool RunSynchronously, + ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount, llvm::Optional ResourceDir); /// Run LSP server loop, receiving input for it from \p In. \p In must be Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ clangd/ClangdLSPServer.cpp @@ -220,10 +220,10 @@ R"(,"result":[)" + Locations + R"(]})"); } -ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, bool RunSynchronously, +ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount, llvm::Optional ResourceDir) : Out(Out), DiagConsumer(*this), - Server(CDB, DiagConsumer, FSProvider, RunSynchronously, ResourceDir) {} + Server(CDB, DiagConsumer, FSProvider, AsyncThreadsCount, ResourceDir) {} void ClangdLSPServer::run(std::istream &In) { assert(!IsDone && "Run was called before"); Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -101,11 +101,20 @@ class ClangdServer; -/// Handles running WorkerRequests of ClangdServer on a separate threads. -/// Currently runs only one worker thread. +/// Returns a number of a default async threads to use for ClangdScheduler. +/// Returned value is always >= 1 (i.e. will not cause requests to be processed +/// synchronously). +unsigned getDefaultAsyncThreadsCount(); + +/// Handles running WorkerRequests of ClangdServer on a number of worker +/// threads. class ClangdScheduler { public: - ClangdScheduler(bool RunSynchronously); + /// If \p AsyncThreadsCount is 0, requests added using addToFront and addToEnd + /// will be processed synchronously on the calling thread. + // Otherwise, \p AsyncThreadsCount threads will be created to schedule the + // requests. + ClangdScheduler(unsigned AsyncThreadsCount); ~ClangdScheduler(); /// Add a new request to run function \p F with args \p As to the start of the @@ -146,17 +155,16 @@ private: bool RunSynchronously; std::mutex Mutex; - /// We run some tasks on a separate threads(parsing, CppFile cleanup). - /// This thread looks into RequestQueue to find requests to handle and - /// terminates when Done is set to true. - std::thread Worker; - /// Setting Done to true will make the worker thread terminate. + /// We run some tasks on separate threads(parsing, CppFile cleanup). + /// These threads looks into RequestQueue to find requests to handle and + /// terminate when Done is set to true. + std::vector Workers; + /// Setting Done to true will make the worker threads terminate. bool Done = false; - /// A queue of requests. - /// FIXME(krasimir): code completion should always have priority over parsing - /// for diagnostics. + /// A queue of requests. Elements of this vector are async computations (i.e. + /// results of calling std::async(std::launch::deferred, ...)). std::deque> RequestQueue; - /// Condition variable to wake up the worker thread. + /// Condition variable to wake up worker threads. std::condition_variable RequestCV; }; @@ -165,22 +173,19 @@ /// diagnostics for tracked files). class ClangdServer { public: - /// Creates a new ClangdServer. If \p RunSynchronously is false, no worker - /// thread will be created and all requests will be completed synchronously on - /// the calling thread (this is mostly used for tests). If \p RunSynchronously - /// is true, a worker thread will be created to parse files in the background - /// and provide diagnostics results via DiagConsumer.onDiagnosticsReady - /// callback. File accesses for each instance of parsing will be conducted via - /// a vfs::FileSystem provided by \p FSProvider. Results of code - /// completion/diagnostics also include a tag, that \p FSProvider returns - /// along with the vfs::FileSystem. - /// When \p ResourceDir is set, it will be used to search for internal headers + /// Creates a new ClangdServer. To server parsing requests ClangdScheduler, + /// that spawns \p AsyncThreadsCount worker threads will be created (when \p + /// AsyncThreadsCount is 0, requests will be processed on the calling thread). + /// instance of parsing will be conducted via a vfs::FileSystem provided by \p + /// FSProvider. Results of code completion/diagnostics also include a tag, + /// that \p FSProvider returns along with the vfs::FileSystem. When \p + /// ResourceDir is set, it will be used to search for internal headers /// (overriding defaults and -resource-dir compiler flag, if set). If \p /// ResourceDir is None, ClangdServer will attempt to set it to a standard /// location, obtained via CompilerInvocation::GetResourcePath. ClangdServer(GlobalCompilationDatabase &CDB, DiagnosticsConsumer &DiagConsumer, - FileSystemProvider &FSProvider, bool RunSynchronously, + FileSystemProvider &FSProvider, unsigned AsyncThreadsCount, llvm::Optional ResourceDir = llvm::None); /// Add a \p File to the list of tracked C++ files or update the contents if @@ -192,10 +197,13 @@ std::future addDocument(PathRef File, StringRef Contents); /// Remove \p File from list of tracked files, schedule a request to free /// resources associated with it. - /// \return A future that will become ready the file is removed and all - /// associated reosources are freed. + /// \return A future that will become ready when the file is removed and all + /// associated resources are freed. std::future removeDocument(PathRef File); /// Force \p File to be reparsed using the latest contents. + /// Will also check if CompileCommand, provided by GlobalCompilationDatabase + /// for \p File has changed. If it has, will remove currently stored Preamble + /// and AST and rebuild them from scratch. std::future forceReparse(PathRef File); /// Run code completion for \p File at \p Pos. If \p OverridenContents is not @@ -233,6 +241,13 @@ std::string dumpAST(PathRef File); private: + std::future + scheduleReparseAndDiags(PathRef File, VersionedDraft Contents, + std::shared_ptr Resources, + Tagged> TaggedFS); + + std::future scheduleCancelRebuild(std::shared_ptr Resources); + GlobalCompilationDatabase &CDB; DiagnosticsConsumer &DiagConsumer; FileSystemProvider &FSProvider; Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -78,40 +78,52 @@ return make_tagged(vfs::getRealFileSystem(), VFSTag()); } -ClangdScheduler::ClangdScheduler(bool RunSynchronously) - : RunSynchronously(RunSynchronously) { +unsigned clangd::getDefaultAsyncThreadsCount() { + unsigned HardwareConcurrency = std::thread::hardware_concurrency(); + // C++ standard says that hardware_concurrency() + // may return 0, fallback to 1 worker thread in + // that case. + if (HardwareConcurrency == 0) + return 1; + return HardwareConcurrency; +} + +ClangdScheduler::ClangdScheduler(unsigned AsyncThreadsCount) + : RunSynchronously(AsyncThreadsCount == 0) { if (RunSynchronously) { // Don't start the worker thread if we're running synchronously return; } - // Initialize Worker in ctor body, rather than init list to avoid potentially - // using not-yet-initialized members - Worker = std::thread([this]() { - while (true) { - std::future Request; - - // Pick request from the queue - { - std::unique_lock Lock(Mutex); - // Wait for more requests. - RequestCV.wait(Lock, [this] { return !RequestQueue.empty() || Done; }); - if (Done) - return; - - assert(!RequestQueue.empty() && "RequestQueue was empty"); - - // We process requests starting from the front of the queue. Users of - // ClangdScheduler have a way to prioritise their requests by putting - // them to the either side of the queue (using either addToEnd or - // addToFront). - Request = std::move(RequestQueue.front()); - RequestQueue.pop_front(); - } // unlock Mutex - - Request.get(); - } - }); + Workers.reserve(AsyncThreadsCount); + for (unsigned I = 0; I < AsyncThreadsCount; ++I) { + Workers.push_back(std::thread([this]() { + while (true) { + std::future Request; + + // Pick request from the queue + { + std::unique_lock Lock(Mutex); + // Wait for more requests. + RequestCV.wait(Lock, + [this] { return !RequestQueue.empty() || Done; }); + if (Done) + return; + + assert(!RequestQueue.empty() && "RequestQueue was empty"); + + // We process requests starting from the front of the queue. Users of + // ClangdScheduler have a way to prioritise their requests by putting + // them to the either side of the queue (using either addToEnd or + // addToFront). + Request = std::move(RequestQueue.front()); + RequestQueue.pop_front(); + } // unlock Mutex + + Request.get(); + } + })); + } } ClangdScheduler::~ClangdScheduler() { @@ -123,19 +135,21 @@ // Wake up the worker thread Done = true; } // unlock Mutex - RequestCV.notify_one(); - Worker.join(); + RequestCV.notify_all(); + + for (auto &Worker : Workers) + Worker.join(); } ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB, DiagnosticsConsumer &DiagConsumer, FileSystemProvider &FSProvider, - bool RunSynchronously, + unsigned AsyncThreadsCount, llvm::Optional ResourceDir) : CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider), ResourceDir(ResourceDir ? ResourceDir->str() : getStandardResourceDir()), PCHs(std::make_shared()), - WorkScheduler(RunSynchronously) {} + WorkScheduler(AsyncThreadsCount) {} std::future ClangdServer::addDocument(PathRef File, StringRef Contents) { DocVersion Version = DraftMgr.updateDraft(File, Contents); @@ -143,67 +157,31 @@ auto TaggedFS = FSProvider.getTaggedFileSystem(File); std::shared_ptr Resources = Units.getOrCreateFile(File, ResourceDir, CDB, PCHs, TaggedFS.Value); - - std::future>> DeferredRebuild = - Resources->deferRebuild(Contents, TaggedFS.Value); - std::promise DonePromise; - std::future DoneFuture = DonePromise.get_future(); - - Path FileStr = File; - VFSTag Tag = TaggedFS.Tag; - auto ReparseAndPublishDiags = - [this, FileStr, Version, - Tag](std::future>> - DeferredRebuild, - std::promise DonePromise) -> void { - FulfillPromiseGuard Guard(DonePromise); - - auto CurrentVersion = DraftMgr.getVersion(FileStr); - if (CurrentVersion != Version) - return; // This request is outdated - - auto Diags = DeferredRebuild.get(); - if (!Diags) - return; // A new reparse was requested before this one completed. - DiagConsumer.onDiagnosticsReady(FileStr, - make_tagged(std::move(*Diags), Tag)); - }; - - WorkScheduler.addToFront(std::move(ReparseAndPublishDiags), - std::move(DeferredRebuild), std::move(DonePromise)); - return DoneFuture; + return scheduleReparseAndDiags(File, VersionedDraft{Version, Contents.str()}, + std::move(Resources), std::move(TaggedFS)); } std::future ClangdServer::removeDocument(PathRef File) { - auto Version = DraftMgr.removeDraft(File); - Path FileStr = File; - - std::promise DonePromise; - std::future DoneFuture = DonePromise.get_future(); - - auto RemoveDocFromCollection = [this, FileStr, - Version](std::promise DonePromise) { - FulfillPromiseGuard Guard(DonePromise); - - if (Version != DraftMgr.getVersion(FileStr)) - return; // This request is outdated, do nothing - - std::shared_ptr File = Units.removeIfPresent(FileStr); - if (!File) - return; - // Cancel all ongoing rebuilds, so that we don't do extra work before - // deleting this file. - File->cancelRebuilds(); - }; - WorkScheduler.addToFront(std::move(RemoveDocFromCollection), - std::move(DonePromise)); - return DoneFuture; + DraftMgr.removeDraft(File); + std::shared_ptr Resources = Units.removeIfPresent(File); + return scheduleCancelRebuild(std::move(Resources)); } std::future ClangdServer::forceReparse(PathRef File) { - // The addDocument schedules the reparse even if the contents of the file - // never changed, so we just call it here. - return addDocument(File, getDocument(File)); + auto FileContents = DraftMgr.getDraft(File); + assert(FileContents.Draft && + "forceReparse() was called for non-added document"); + + auto TaggedFS = FSProvider.getTaggedFileSystem(File); + auto Recreated = Units.recreateFileIfCompileCommandChanged( + File, ResourceDir, CDB, PCHs, TaggedFS.Value); + + // Note that std::future from this cleanup action is ignored. + scheduleCancelRebuild(std::move(Recreated.RemovedFile)); + // Schedule a reparse. + return scheduleReparseAndDiags(File, std::move(FileContents), + std::move(Recreated.FileInCollection), + std::move(TaggedFS)); } Tagged> @@ -306,3 +284,60 @@ }); return make_tagged(std::move(Result), TaggedFS.Tag); } + +std::future ClangdServer::scheduleReparseAndDiags( + PathRef File, VersionedDraft Contents, std::shared_ptr Resources, + Tagged> TaggedFS) { + + assert(Contents.Draft && "Draft must have contents"); + std::future>> DeferredRebuild = + Resources->deferRebuild(*Contents.Draft, TaggedFS.Value); + std::promise DonePromise; + std::future DoneFuture = DonePromise.get_future(); + + DocVersion Version = Contents.Version; + Path FileStr = File; + VFSTag Tag = TaggedFS.Tag; + auto ReparseAndPublishDiags = + [this, FileStr, Version, + Tag](std::future>> + DeferredRebuild, + std::promise DonePromise) -> void { + FulfillPromiseGuard Guard(DonePromise); + + auto CurrentVersion = DraftMgr.getVersion(FileStr); + if (CurrentVersion != Version) + return; // This request is outdated + + auto Diags = DeferredRebuild.get(); + if (!Diags) + return; // A new reparse was requested before this one completed. + DiagConsumer.onDiagnosticsReady(FileStr, + make_tagged(std::move(*Diags), Tag)); + }; + + WorkScheduler.addToFront(std::move(ReparseAndPublishDiags), + std::move(DeferredRebuild), std::move(DonePromise)); + return DoneFuture; +} + +std::future +ClangdServer::scheduleCancelRebuild(std::shared_ptr Resources) { + std::promise DonePromise; + std::future DoneFuture = DonePromise.get_future(); + if (!Resources) { + // No need to schedule any cleanup. + DonePromise.set_value(); + return DoneFuture; + } + + std::future DeferredCancel = Resources->deferCancelRebuild(); + auto CancelReparses = [Resources](std::promise DonePromise, + std::future DeferredCancel) { + FulfillPromiseGuard Guard(DonePromise); + DeferredCancel.get(); + }; + WorkScheduler.addToFront(std::move(CancelReparses), std::move(DonePromise), + std::move(DeferredCancel)); + return DoneFuture; +} Index: clangd/ClangdUnit.h =================================================================== --- clangd/ClangdUnit.h +++ clangd/ClangdUnit.h @@ -151,9 +151,17 @@ CppFile(CppFile const &) = delete; CppFile(CppFile &&) = delete; - /// Cancels all scheduled rebuilds and sets AST and Preamble to nulls. + /// Cancels a scheduled rebuild, if any, and sets AST and Preamble to nulls. /// If a rebuild is in progress, will wait for it to finish. - void cancelRebuilds(); + void cancelRebuild(); + + /// Similar to deferRebuild, but sets both Preamble and AST to nulls instead + /// of doing an actual parsing. Returned future is a deferred computation that + /// will wait for any ongoing rebuilds to finish and actually set the AST and + /// Preamble to nulls. It can be run on a different thread. + /// This function is useful to cancel ongoing rebuilds, if any, before + /// removing CppFile. + std::future deferCancelRebuild(); /// Rebuild AST and Preamble synchronously on the calling thread. /// Returns a list of diagnostics or a llvm::None, if another rebuild was Index: clangd/ClangdUnit.cpp =================================================================== --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -711,10 +711,12 @@ ASTFuture = ASTPromise.get_future(); } -void CppFile::cancelRebuilds() { +void CppFile::cancelRebuild() { deferCancelRebuild().get(); } + +std::future CppFile::deferCancelRebuild() { std::unique_lock Lock(Mutex); // Cancel an ongoing rebuild, if any, and wait for it to finish. - ++this->RebuildCounter; + unsigned RequestRebuildCounter = ++this->RebuildCounter; // Rebuild asserts that futures aren't ready if rebuild is cancelled. // We want to keep this invariant. if (futureIsReady(PreambleFuture)) { @@ -725,12 +727,28 @@ ASTPromise = std::promise>(); ASTFuture = ASTPromise.get_future(); } - // Now wait for rebuild to finish. - RebuildCond.wait(Lock, [this]() { return !this->RebuildInProgress; }); - // Return empty results for futures. - PreamblePromise.set_value(nullptr); - ASTPromise.set_value(std::make_shared(llvm::None)); + Lock.unlock(); + // Notify about changes to RebuildCounter. + RebuildCond.notify_all(); + + std::shared_ptr That = shared_from_this(); + return std::async(std::launch::deferred, [That, RequestRebuildCounter]() { + std::unique_lock Lock(That->Mutex); + CppFile *This = &*That; + This->RebuildCond.wait(Lock, [This, RequestRebuildCounter]() { + return !This->RebuildInProgress || + This->RebuildCounter != RequestRebuildCounter; + }); + + // This computation got cancelled itself, do nothing. + if (This->RebuildCounter != RequestRebuildCounter) + return; + + // Set empty results for Promises. + That->PreamblePromise.set_value(nullptr); + That->ASTPromise.set_value(std::make_shared(llvm::None)); + }); } llvm::Optional> @@ -767,6 +785,8 @@ this->ASTFuture = this->ASTPromise.get_future(); } } // unlock Mutex. + // Notify about changes to RebuildCounter. + RebuildCond.notify_all(); // A helper to function to finish the rebuild. May be run on a different // thread. @@ -916,7 +936,10 @@ if (WasCancelledBeforeConstruction) return; - File.RebuildCond.wait(Lock, [&File]() { return !File.RebuildInProgress; }); + File.RebuildCond.wait(Lock, [&File, RequestRebuildCounter]() { + return !File.RebuildInProgress || + File.RebuildCounter != RequestRebuildCounter; + }); WasCancelledBeforeConstruction = File.RebuildCounter != RequestRebuildCounter; if (WasCancelledBeforeConstruction) Index: clangd/ClangdUnitStore.h =================================================================== --- clangd/ClangdUnitStore.h +++ clangd/ClangdUnitStore.h @@ -42,6 +42,25 @@ return It->second; } + struct RecreateResult { + /// A CppFile, stored in this CppFileCollection for the corresponding + /// filepath after calling recreateFileIfCompileCommandChanged. + std::shared_ptr FileInCollection; + /// If a new CppFile had to be created to account for changed + /// CompileCommand, a previous CppFile instance will be returned in this + /// field. + std::shared_ptr RemovedFile; + }; + + /// Similar to getOrCreateFile, but will replace a current CppFile for \p File + /// with a new one if CompileCommand, provided by \p CDB has changed. + /// If a currently stored CppFile had to be replaced, the previous instance + /// will be returned in RecreateResult.RemovedFile. + RecreateResult recreateFileIfCompileCommandChanged( + PathRef File, PathRef ResourceDir, GlobalCompilationDatabase &CDB, + std::shared_ptr PCHs, + IntrusiveRefCntPtr VFS); + std::shared_ptr getFile(PathRef File) { std::lock_guard Lock(Mutex); @@ -59,6 +78,9 @@ tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase &CDB, PathRef File, PathRef ResourceDir); + bool compileCommandsAreEqual(tooling::CompileCommand const &LHS, + tooling::CompileCommand const &RHS); + std::mutex Mutex; llvm::StringMap> OpenedFiles; }; Index: clangd/ClangdUnitStore.cpp =================================================================== --- clangd/ClangdUnitStore.cpp +++ clangd/ClangdUnitStore.cpp @@ -9,6 +9,7 @@ #include "ClangdUnitStore.h" #include "llvm/Support/Path.h" +#include using namespace clang::clangd; using namespace clang; @@ -25,6 +26,32 @@ return Result; } +CppFileCollection::RecreateResult +CppFileCollection::recreateFileIfCompileCommandChanged( + PathRef File, PathRef ResourceDir, GlobalCompilationDatabase &CDB, + std::shared_ptr PCHs, + IntrusiveRefCntPtr VFS) { + auto NewCommand = getCompileCommand(CDB, File, ResourceDir); + + std::lock_guard Lock(Mutex); + + RecreateResult Result; + + auto It = OpenedFiles.find(File); + if (It == OpenedFiles.end()) { + It = OpenedFiles + .try_emplace(File, CppFile::Create(File, std::move(NewCommand), + std::move(PCHs))) + .first; + } else if (!compileCommandsAreEqual(It->second->getCompileCommand(), + NewCommand)) { + Result.RemovedFile = std::move(It->second); + It->second = CppFile::Create(File, std::move(NewCommand), std::move(PCHs)); + } + Result.FileInCollection = It->second; + return Result; +} + tooling::CompileCommand CppFileCollection::getCompileCommand(GlobalCompilationDatabase &CDB, PathRef File, PathRef ResourceDir) { @@ -39,3 +66,12 @@ std::string(ResourceDir)); return std::move(Commands.front()); } + +bool CppFileCollection::compileCommandsAreEqual( + tooling::CompileCommand const &LHS, tooling::CompileCommand const &RHS) { + // tooling::CompileCommand.Output is ignored, it's not relevant for clangd. + return LHS.Directory == RHS.Directory && + LHS.CommandLine.size() == RHS.CommandLine.size() && + std::equal(LHS.CommandLine.begin(), LHS.CommandLine.end(), + RHS.CommandLine.begin()); +} Index: clangd/tool/ClangdMain.cpp =================================================================== --- clangd/tool/ClangdMain.cpp +++ clangd/tool/ClangdMain.cpp @@ -16,14 +16,20 @@ #include #include #include +#include using namespace clang; using namespace clang::clangd; -static llvm::cl::opt - RunSynchronously("run-synchronously", - llvm::cl::desc("Parse on main thread"), - llvm::cl::init(false), llvm::cl::Hidden); +static llvm::cl::opt + WorkerThreadsCount("j", + llvm::cl::desc("Number of async workers used by clangd"), + llvm::cl::init(getDefaultAsyncThreadsCount())); + +static llvm::cl::opt RunSynchronously( + "run-synchronously", + llvm::cl::desc("Parse on main thread. If set, -j is ignored"), + llvm::cl::init(false), llvm::cl::Hidden); static llvm::cl::opt ResourceDir("resource-dir", @@ -33,6 +39,17 @@ int main(int argc, char *argv[]) { llvm::cl::ParseCommandLineOptions(argc, argv, "clangd"); + if (!RunSynchronously && WorkerThreadsCount == 0) { + llvm::errs() << "A number of worker threads cannot be 0. Did you mean to " + "specify -run-synchronously?"; + return 1; + } + + // Ignore -j option if -run-synchonously is used. + // FIXME: a warning should be shown here. + if (RunSynchronously) + WorkerThreadsCount = 0; + llvm::raw_ostream &Outs = llvm::outs(); llvm::raw_ostream &Logs = llvm::errs(); JSONOutput Out(Outs, Logs); @@ -43,6 +60,7 @@ llvm::Optional ResourceDirRef = None; if (!ResourceDir.empty()) ResourceDirRef = ResourceDir; - ClangdLSPServer LSPServer(Out, RunSynchronously, ResourceDirRef); + + ClangdLSPServer LSPServer(Out, WorkerThreadsCount, ResourceDirRef); LSPServer.run(std::cin); } Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -76,12 +76,62 @@ ``AllowConditionalIntegerCasts`` -> ``AllowIntegerConditions``, ``AllowConditionalPointerCasts`` -> ``AllowPointerConditions``. +- New `android-cloexec-accept + `_ check + + Detects usage of ``accept()``. + +- New `android-cloexec-accept4 + `_ check + + Checks if the required file flag ``SOCK_CLOEXEC`` is present in the argument of + ``accept4()``. + +- New `android-cloexec-dup + `_ check + + Detects usage of ``dup()``. + +- New `android-cloexec-inotify-init + `_ check + + Detects usage of ``inotify_init()``. + +- New `android-cloexec-epoll-create1 + `_ check + + Checks if the required file flag ``EPOLL_CLOEXEC`` is present in the argument of + ``epoll_create1()``. + +- New `android-cloexec-epoll-create + `_ check + + Detects usage of ``epoll_create()``. + +- New `android-cloexec-memfd_create + `_ check + + Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument + of ``memfd_create()``. + - New `bugprone-integer-division `_ check Finds cases where integer division in a floating point context is likely to cause unintended loss of precision. +- New `hicpp-exception-baseclass + `_ check + + Ensures that all exception will be instances of ``std::exception`` and classes + that are derived from it. + +- New `android-cloexec-inotify-init1 + `_ check + + Checks if the required file flag ``IN_CLOEXEC`` is present in the argument of + ``inotify_init1()``. + - New `readability-static-accessed-through-instance `_ check @@ -92,6 +142,8 @@ `_ option. +- Added alias `hicpp-braces-around-statements `_ + Improvements to include-fixer ----------------------------- Index: docs/clang-tidy/checks/android-cloexec-accept.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/android-cloexec-accept.rst @@ -0,0 +1,18 @@ +.. title:: clang-tidy - android-cloexec-accept + +android-cloexec-accept +====================== + +The usage of ``accept()`` is not recommended, it's better to use ``accept4()``. +Without this flag, an opened sensitive file descriptor would remain open across +a fork+exec to a lower-privileged SELinux domain. + +Examples: + +.. code-block:: c++ + + accept(sockfd, addr, addrlen); + + // becomes + + accept4(sockfd, addr, addrlen, SOCK_CLOEXEC); Index: docs/clang-tidy/checks/android-cloexec-accept4.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/android-cloexec-accept4.rst @@ -0,0 +1,18 @@ +.. title:: clang-tidy - android-cloexec-accept4 + +android-cloexec-accept4 +======================= + +``accept4()`` should include ``SOCK_CLOEXEC`` in its type argument to avoid the +file descriptor leakage. Without this flag, an opened sensitive file would +remain open across a fork+exec to a lower-privileged SELinux domain. + +Examples: + +.. code-block:: c++ + + accept4(sockfd, addr, addrlen, SOCK_NONBLOCK); + + // becomes + + accept4(sockfd, addr, addrlen, SOCK_NONBLOCK | SOCK_CLOEXEC); Index: docs/clang-tidy/checks/android-cloexec-dup.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/android-cloexec-dup.rst @@ -0,0 +1,18 @@ +.. title:: clang-tidy - android-cloexec-dup + +android-cloexec-dup +=================== + +The usage of ``dup()`` is not recommended, it's better to use ``fcntl()``, +which can set the close-on-exec flag. Otherwise, an opened sensitive file would +remain open across a fork+exec to a lower-privileged SELinux domain. + +Examples: + +.. code-block:: c++ + + int fd = dup(oldfd); + + // becomes + + int fd = fcntl(oldfd, F_DUPFD_CLOEXEC); Index: docs/clang-tidy/checks/android-cloexec-epoll-create.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/android-cloexec-epoll-create.rst @@ -0,0 +1,17 @@ +.. title:: clang-tidy - android-cloexec-epoll-create + +android-cloexec-epoll-create +============================ + +The usage of ``epoll_create()`` is not recommended, it's better to use +``epoll_create1()``, which allows close-on-exec. + +Examples: + +.. code-block:: c++ + + epoll_create(size); + + // becomes + + epoll_create1(EPOLL_CLOEXEC); Index: docs/clang-tidy/checks/android-cloexec-epoll-create1.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/android-cloexec-epoll-create1.rst @@ -0,0 +1,18 @@ +.. title:: clang-tidy - android-cloexec-epoll-create1 + +android-cloexec-epoll-create1 +============================= + +``epoll_create1()`` should include ``EPOLL_CLOEXEC`` in its type argument to +avoid the file descriptor leakage. Without this flag, an opened sensitive file +would remain open across a fork+exec to a lower-privileged SELinux domain. + +Examples: + +.. code-block:: c++ + + epoll_create1(0); + + // becomes + + epoll_create1(EPOLL_CLOEXEC); Index: docs/clang-tidy/checks/android-cloexec-inotify-init.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/android-cloexec-inotify-init.rst @@ -0,0 +1,17 @@ +.. title:: clang-tidy - android-cloexec-inotify-init + +android-cloexec-inotify-init +============================ + +The usage of ``inotify_init()`` is not recommended, it's better to use +``inotify_init1()``. + +Examples: + +.. code-block:: c++ + + inotify_init(); + + // becomes + + inotify_init1(IN_CLOEXEC); Index: docs/clang-tidy/checks/android-cloexec-inotify-init1.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/android-cloexec-inotify-init1.rst @@ -0,0 +1,18 @@ +.. title:: clang-tidy - android-cloexec-inotify-init1 + +android-cloexec-inotify-init1 +============================= + +``inotify_init1()`` should include ``IN_CLOEXEC`` in its type argument to avoid the +file descriptor leakage. Without this flag, an opened sensitive file would +remain open across a fork+exec to a lower-privileged SELinux domain. + +Examples: + +.. code-block:: c++ + + inotify_init1(IN_NONBLOCK); + + // becomes + + inotify_init1(IN_NONBLOCK | IN_CLOEXEC); Index: docs/clang-tidy/checks/android-cloexec-memfd-create.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/android-cloexec-memfd-create.rst @@ -0,0 +1,18 @@ +.. title:: clang-tidy - android-cloexec-memfd-create + +android-cloexec-memfd-create +============================ + +``memfd_create()`` should include ``MFD_CLOEXEC`` in its type argument to avoid +the file descriptor leakage. Without this flag, an opened sensitive file would +remain open across a fork+exec to a lower-privileged SELinux domain. + +Examples: + +.. code-block:: c++ + + memfd_create(name, MFD_ALLOW_SEALING); + + // becomes + + memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); Index: docs/clang-tidy/checks/hicpp-braces-around-statements.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/hicpp-braces-around-statements.rst @@ -0,0 +1,11 @@ +.. title:: clang-tidy - hicpp-braces-around-statements +.. meta:: + :http-equiv=refresh: 5;URL=readability-braces-around-statements.html + +hicpp-braces-around-statements +============================== + +The hicpp-braces-around-statements check is an alias, please see +`readability-braces-around-statements `_ +for more information. +It enforces the `rule 6.1.1 `_. Index: docs/clang-tidy/checks/hicpp-exception-baseclass.rst =================================================================== --- docs/clang-tidy/checks/hicpp-exception-baseclass.rst +++ docs/clang-tidy/checks/hicpp-exception-baseclass.rst @@ -5,6 +5,7 @@ Ensure that every value that in a ``throw`` expression is an instance of ``std::exception``. + This enforces `rule 15.1 `_ of the High Integrity C++ Coding Standard. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -4,8 +4,16 @@ ================= .. toctree:: + android-cloexec-accept + android-cloexec-accept4 android-cloexec-creat + android-cloexec-epoll-create + android-cloexec-epoll-create1 + android-cloexec-dup android-cloexec-fopen + android-cloexec-inotify-init + android-cloexec-inotify-init1 + android-cloexec-memfd-create android-cloexec-open android-cloexec-socket boost-use-to-string @@ -61,6 +69,7 @@ google-runtime-member-string-references google-runtime-operator google-runtime-references + hicpp-braces-around-statements (redirects to readability-braces-around-statements) hicpp-exception-baseclass hicpp-explicit-conversions (redirects to google-explicit-constructor) hicpp-function-size (redirects to readability-function-size) Index: test/clang-tidy/android-cloexec-accept.cpp =================================================================== --- /dev/null +++ test/clang-tidy/android-cloexec-accept.cpp @@ -0,0 +1,28 @@ +// RUN: %check_clang_tidy %s android-cloexec-accept %t + +struct sockaddr {}; +typedef int socklen_t; +#define NULL 0 + +extern "C" int accept(int sockfd, struct sockaddr *addr, socklen_t *addrlen); + +void f() { + accept(0, NULL, NULL); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer accept4() to accept() because accept4() allows SOCK_CLOEXEC [android-cloexec-accept] + // CHECK-FIXES: accept4(0, NULL, NULL, SOCK_CLOEXEC); +} + +namespace i { +int accept(int sockfd, struct sockaddr *addr, socklen_t *addrlen); +void g() { + accept(0, NULL, NULL); +} +} // namespace i + +class C { +public: + int accept(int sockfd, struct sockaddr *addr, socklen_t *addrlen); + void h() { + accept(0, NULL, NULL); + } +}; Index: test/clang-tidy/android-cloexec-accept4.cpp =================================================================== --- /dev/null +++ test/clang-tidy/android-cloexec-accept4.cpp @@ -0,0 +1,66 @@ +// RUN: %check_clang_tidy %s android-cloexec-accept4 %t + +typedef int socklen_t; +struct sockaddr {}; + +#define SOCK_NONBLOCK 1 +#define __O_CLOEXEC 3 +#define SOCK_CLOEXEC __O_CLOEXEC +#define TEMP_FAILURE_RETRY(exp) \ + ({ \ + int _rc; \ + do { \ + _rc = (exp); \ + } while (_rc == -1); \ + }) +#define NULL 0 + +extern "C" int accept4(int sockfd, struct sockaddr *addr, socklen_t *addrlen, int flags); + +void a() { + accept4(0, NULL, NULL, SOCK_NONBLOCK); + // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'accept4' should use SOCK_CLOEXEC where possible [android-cloexec-accept4] + // CHECK-FIXES: accept4(0, NULL, NULL, SOCK_NONBLOCK | SOCK_CLOEXEC); + TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, SOCK_NONBLOCK)); + // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: 'accept4' + // CHECK-FIXES: TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, SOCK_NONBLOCK | SOCK_CLOEXEC)); +} + +void f() { + accept4(0, NULL, NULL, 3); + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 'accept4' + // CHECK-FIXES: accept4(0, NULL, NULL, 3 | SOCK_CLOEXEC); + TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, 3)); + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: 'accept4' + // CHECK-FIXES: TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, 3 | SOCK_CLOEXEC)); + + int flag = SOCK_NONBLOCK; + accept4(0, NULL, NULL, flag); + TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, flag)); +} + +namespace i { +int accept4(int sockfd, struct sockaddr *addr, socklen_t *addrlen, int flags); + +void d() { + accept4(0, NULL, NULL, SOCK_NONBLOCK); + TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, SOCK_NONBLOCK)); +} + +} // namespace i + +void e() { + accept4(0, NULL, NULL, SOCK_CLOEXEC); + TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, SOCK_CLOEXEC)); + accept4(0, NULL, NULL, SOCK_NONBLOCK | SOCK_CLOEXEC); + TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, SOCK_NONBLOCK | SOCK_CLOEXEC)); +} + +class G { +public: + int accept4(int sockfd, struct sockaddr *addr, socklen_t *addrlen, int flags); + void d() { + accept4(0, NULL, NULL, SOCK_NONBLOCK); + TEMP_FAILURE_RETRY(accept4(0, NULL, NULL, SOCK_NONBLOCK)); + } +}; Index: test/clang-tidy/android-cloexec-dup.cpp =================================================================== --- /dev/null +++ test/clang-tidy/android-cloexec-dup.cpp @@ -0,0 +1,31 @@ +// RUN: %check_clang_tidy %s android-cloexec-dup %t + +extern "C" int dup(int oldfd); +void f() { + dup(1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer fcntl() to dup() because fcntl() allows F_DUPFD_CLOEXEC [android-cloexec-dup] + // CHECK-FIXES: fcntl(1, F_DUPFD_CLOEXEC); + int oldfd = 0; + dup(oldfd); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer + // CHECK-FIXES: fcntl(oldfd, F_DUPFD_CLOEXEC); +} + +namespace i { +int dup(int oldfd); +void g() { + dup(0); + int oldfd = 1; + dup(oldfd); +} +} // namespace i + +class C { +public: + int dup(int oldfd); + void h() { + dup(0); + int oldfd = 1; + dup(oldfd); + } +}; Index: test/clang-tidy/android-cloexec-epoll-create.cpp =================================================================== --- /dev/null +++ test/clang-tidy/android-cloexec-epoll-create.cpp @@ -0,0 +1,24 @@ +// RUN: %check_clang_tidy %s android-cloexec-epoll-create %t + +extern "C" int epoll_create(int size); + +void f() { + epoll_create(0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer epoll_create() to epoll_create1() because epoll_create1() allows EPOLL_CLOEXEC [android-cloexec-epoll-create] + // CHECK-FIXES: epoll_create1(EPOLL_CLOEXEC); +} + +namespace i { +int epoll_create(int size); +void g() { + epoll_create(0); +} +} // namespace i + +class C { +public: + int epoll_create(int size); + void h() { + epoll_create(0); + } +}; Index: test/clang-tidy/android-cloexec-epoll-create1.cpp =================================================================== --- /dev/null +++ test/clang-tidy/android-cloexec-epoll-create1.cpp @@ -0,0 +1,59 @@ +// RUN: %check_clang_tidy %s android-cloexec-epoll-create1 %t + +#define __O_CLOEXEC 3 +#define EPOLL_CLOEXEC __O_CLOEXEC +#define TEMP_FAILURE_RETRY(exp) \ + ({ \ + int _rc; \ + do { \ + _rc = (exp); \ + } while (_rc == -1); \ + }) + +extern "C" int epoll_create1(int flags); + +void a() { + epoll_create1(0); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'epoll_create1' should use EPOLL_CLOEXEC where possible [android-cloexec-epoll-create1] + // CHECK-FIXES: epoll_create1(EPOLL_CLOEXEC); + TEMP_FAILURE_RETRY(epoll_create1(0)); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'epoll_create1' + // CHECK-FIXES: TEMP_FAILURE_RETRY(epoll_create1(EPOLL_CLOEXEC)); +} + +void f() { + epoll_create1(3); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'epoll_create1' + // CHECK-FIXES: epoll_create1(EPOLL_CLOEXEC); + TEMP_FAILURE_RETRY(epoll_create1(3)); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'epoll_create1' + // CHECK-FIXES: TEMP_FAILURE_RETRY(epoll_create1(EPOLL_CLOEXEC)); + + int flag = 0; + epoll_create1(EPOLL_CLOEXEC); + TEMP_FAILURE_RETRY(epoll_create1(EPOLL_CLOEXEC)); +} + +namespace i { +int epoll_create1(int flags); + +void d() { + epoll_create1(0); + TEMP_FAILURE_RETRY(epoll_create1(0)); +} + +} // namespace i + +void e() { + epoll_create1(EPOLL_CLOEXEC); + TEMP_FAILURE_RETRY(epoll_create1(EPOLL_CLOEXEC)); +} + +class G { +public: + int epoll_create1(int flags); + void d() { + epoll_create1(EPOLL_CLOEXEC); + TEMP_FAILURE_RETRY(epoll_create1(EPOLL_CLOEXEC)); + } +}; Index: test/clang-tidy/android-cloexec-inotify-init.cpp =================================================================== --- /dev/null +++ test/clang-tidy/android-cloexec-inotify-init.cpp @@ -0,0 +1,24 @@ +// RUN: %check_clang_tidy %s android-cloexec-inotify-init %t + +extern "C" int inotify_init(); + +void f() { + inotify_init(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer inotify_init() to inotify_init1() because inotify_init1() allows IN_CLOEXEC [android-cloexec-inotify-init] + // CHECK-FIXES: inotify_init1(IN_CLOEXEC); +} + +namespace i { +int inotify_init(); +void g() { + inotify_init(); +} +} // namespace i + +class C { +public: + int inotify_init(); + void h() { + inotify_init(); + } +}; Index: test/clang-tidy/android-cloexec-inotify-init1.cpp =================================================================== --- /dev/null +++ test/clang-tidy/android-cloexec-inotify-init1.cpp @@ -0,0 +1,64 @@ +// RUN: %check_clang_tidy %s android-cloexec-inotify-init1 %t + +#define IN_NONBLOCK 1 +#define __O_CLOEXEC 3 +#define IN_CLOEXEC __O_CLOEXEC +#define TEMP_FAILURE_RETRY(exp) \ + ({ \ + int _rc; \ + do { \ + _rc = (exp); \ + } while (_rc == -1); \ + }) + +extern "C" int inotify_init1(int flags); + +void a() { + inotify_init1(IN_NONBLOCK); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 'inotify_init1' should use IN_CLOEXEC where possible [android-cloexec-inotify-init1] + // CHECK-FIXES: inotify_init1(IN_NONBLOCK | IN_CLOEXEC); + TEMP_FAILURE_RETRY(inotify_init1(IN_NONBLOCK)); + // CHECK-MESSAGES: :[[@LINE-1]]:47: warning: 'inotify_init1' + // CHECK-FIXES: TEMP_FAILURE_RETRY(inotify_init1(IN_NONBLOCK | IN_CLOEXEC)); +} + +void f() { + inotify_init1(0); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'inotify_init1' + // CHECK-FIXES: inotify_init1(IN_CLOEXEC); + TEMP_FAILURE_RETRY(inotify_init1(0)); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'inotify_init1' + // CHECK-FIXES: TEMP_FAILURE_RETRY(inotify_init1(IN_CLOEXEC)); + + int flag = 1; + inotify_init1(flag); + TEMP_FAILURE_RETRY(inotify_init1(flag)); +} + +namespace i { +int inotify_init1(int flags); + +void d() { + inotify_init1(IN_NONBLOCK); + TEMP_FAILURE_RETRY(inotify_init1(IN_NONBLOCK)); +} + +} // namespace i + +void e() { + inotify_init1(IN_CLOEXEC); + TEMP_FAILURE_RETRY(inotify_init1(IN_CLOEXEC)); + inotify_init1(IN_NONBLOCK | IN_CLOEXEC); + TEMP_FAILURE_RETRY(inotify_init1(IN_NONBLOCK | IN_CLOEXEC)); +} + +class G { +public: + int inotify_init1(int flags); + void d() { + inotify_init1(IN_CLOEXEC); + TEMP_FAILURE_RETRY(inotify_init1(IN_CLOEXEC)); + inotify_init1(IN_NONBLOCK | IN_CLOEXEC); + TEMP_FAILURE_RETRY(inotify_init1(IN_NONBLOCK | IN_CLOEXEC)); + } +}; Index: test/clang-tidy/android-cloexec-memfd-create.cpp =================================================================== --- /dev/null +++ test/clang-tidy/android-cloexec-memfd-create.cpp @@ -0,0 +1,63 @@ +// RUN: %check_clang_tidy %s android-cloexec-memfd-create %t + +#define MFD_ALLOW_SEALING 1 +#define __O_CLOEXEC 3 +#define MFD_CLOEXEC __O_CLOEXEC +#define TEMP_FAILURE_RETRY(exp) \ + ({ \ + int _rc; \ + do { \ + _rc = (exp); \ + } while (_rc == -1); \ + }) +#define NULL 0 + +extern "C" int memfd_create(const char *name, unsigned int flags); + +void a() { + memfd_create(NULL, MFD_ALLOW_SEALING); + // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'memfd_create' should use MFD_CLOEXEC where possible [android-cloexec-memfd-create] + // CHECK-FIXES: memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC) + TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING)); + // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: 'memfd_create' + // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC)) +} + +void f() { + memfd_create(NULL, 3); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'memfd_create' + // CHECK-FIXES: memfd_create(NULL, 3 | MFD_CLOEXEC) + TEMP_FAILURE_RETRY(memfd_create(NULL, 3)); + // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: 'memfd_create' + // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, 3 | MFD_CLOEXEC)) + + int flag = 3; + memfd_create(NULL, flag); + TEMP_FAILURE_RETRY(memfd_create(NULL, flag)); +} + +namespace i { +int memfd_create(const char *name, unsigned int flags); + +void d() { + memfd_create(NULL, MFD_ALLOW_SEALING); + TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING)); +} + +} // namespace i + +void e() { + memfd_create(NULL, MFD_CLOEXEC); + TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_CLOEXEC)); + memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC); + TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC)); +} + +class G { +public: + int memfd_create(const char *name, unsigned int flags); + void d() { + memfd_create(NULL, MFD_ALLOW_SEALING); + TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING)); + } +}; Index: test/clang-tidy/hicpp-exception-baseclass.cpp =================================================================== --- test/clang-tidy/hicpp-exception-baseclass.cpp +++ test/clang-tidy/hicpp-exception-baseclass.cpp @@ -1,28 +1,31 @@ -// RUN: %check_clang_tidy %s hicpp-exception-baseclass %t +// RUN: %check_clang_tidy %s hicpp-exception-baseclass %t -- -- -fcxx-exceptions namespace std { class exception {}; } // namespace std class derived_exception : public std::exception {}; +class deep_hierarchy : public derived_exception {}; class non_derived_exception {}; void problematic() { try { throw int(42); // Built in is not allowed -// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing value which type is not derived from std::exception - } catch (int e) { // Catch clauses other then std::exception derived classes are bad as well + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception' + } catch (int e) { } throw int(42); // Bad -// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing value which type is not derived from std::exception + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception' try { throw non_derived_exception(); // Some class is not allowed -// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing value which type is not derived from std::exception - } catch (non_derived_exception &e) { // Catching non std::exception + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception' + // CHECK-MESSAGES: 9:1: note: type defined here + } catch (non_derived_exception &e) { } throw non_derived_exception(); // Bad -// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing value which type is not derived from std::exception + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception' + // CHECK-MESSAGES: 9:1: note: type defined here } void allowed_throws() { @@ -37,4 +40,30 @@ } catch (derived_exception &e) { // Ok } throw derived_exception(); // Ok + + try { + throw deep_hierarchy(); // Ok, multiple levels of inheritance + } catch (deep_hierarchy &e) { // Ok + } + throw deep_hierarchy(); // Ok +} + +// Templated function that throws exception based on template type +template +void ThrowException() { throw T(); } +#define THROW_EXCEPTION(CLASS) ThrowException() + +template +class generic_exception : std::exception {}; + +void generic_exceptions() { + THROW_EXCEPTION(int); + THROW_EXCEPTION(std::exception); + THROW_EXCEPTION(derived_exception); + THROW_EXCEPTION(deep_hierarchy); + THROW_EXCEPTION(non_derived_exception); + + throw generic_exception(); + + THROW_EXCEPTION(generic_exception); } Index: test/clang-tidy/modernize-make-unique.cpp =================================================================== --- test/clang-tidy/modernize-make-unique.cpp +++ test/clang-tidy/modernize-make-unique.cpp @@ -246,52 +246,75 @@ std::unique_ptr PE1 = std::unique_ptr(new E{}); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead // CHECK-FIXES: std::unique_ptr PE1 = std::make_unique(); + PE1.reset(new E{}); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead + // CHECK-FIXES: PE1 = std::make_unique(); + + //============================================================================ + // NOTE: For initlializer-list constructors, the check only gives warnings, + // and no fixes are generated. + //============================================================================ // Initialization with the initializer-list constructor. std::unique_ptr PE2 = std::unique_ptr(new E{1, 2}); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead - // CHECK-FIXES: std::unique_ptr PE2 = std::make_unique({1, 2}); + // CHECK-FIXES: std::unique_ptr PE2 = std::unique_ptr(new E{1, 2}); + PE2.reset(new E{1, 2}); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead + // CHECK-FIXES: PE2.reset(new E{1, 2}); // Initialization with default constructor. std::unique_ptr PF1 = std::unique_ptr(new F()); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead // CHECK-FIXES: std::unique_ptr PF1 = std::make_unique(); + PF1.reset(new F()); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead + // CHECK-FIXES: PF1 = std::make_unique(); // Initialization with default constructor. std::unique_ptr PF2 = std::unique_ptr(new F{}); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead // CHECK-FIXES: std::unique_ptr PF2 = std::make_unique(); + PF2.reset(new F()); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead + // CHECK-FIXES: PF2 = std::make_unique(); // Initialization with the initializer-list constructor. std::unique_ptr PF3 = std::unique_ptr(new F{1}); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead - // CHECK-FIXES: std::unique_ptr PF3 = std::make_unique({1}); + // CHECK-FIXES: std::unique_ptr PF3 = std::unique_ptr(new F{1}); + PF3.reset(new F{1}); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead + // CHECK-FIXES: PF3.reset(new F{1}); // Initialization with the initializer-list constructor. std::unique_ptr PF4 = std::unique_ptr(new F{1, 2}); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead - // CHECK-FIXES: std::unique_ptr PF4 = std::make_unique({1, 2}); + // CHECK-FIXES: std::unique_ptr PF4 = std::unique_ptr(new F{1, 2}); // Initialization with the initializer-list constructor. std::unique_ptr PF5 = std::unique_ptr(new F({1, 2})); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead - // CHECK-FIXES: std::unique_ptr PF5 = std::make_unique({1, 2}); + // CHECK-FIXES: std::unique_ptr PF5 = std::unique_ptr(new F({1, 2})); // Initialization with the initializer-list constructor as the default // constructor is not present. std::unique_ptr PG1 = std::unique_ptr(new G{}); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead - // CHECK-FIXES: std::unique_ptr PG1 = std::make_unique({}); + // CHECK-FIXES: std::unique_ptr PG1 = std::unique_ptr(new G{}); + PG1.reset(new G{}); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead + // CHECK-FIXES: PG1.reset(new G{}); // Initialization with the initializer-list constructor. std::unique_ptr PG2 = std::unique_ptr(new G{1}); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead - // CHECK-FIXES: std::unique_ptr PG2 = std::make_unique({1}); + // CHECK-FIXES: std::unique_ptr PG2 = std::unique_ptr(new G{1}); // Initialization with the initializer-list constructor. std::unique_ptr PG3 = std::unique_ptr(new G{1, 2}); // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead - // CHECK-FIXES: std::unique_ptr PG3 = std::make_unique({1, 2}); + // CHECK-FIXES: std::unique_ptr PG3 = std::unique_ptr(new G{1, 2}); std::unique_ptr FF = std::unique_ptr(new Foo()); // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: @@ -428,3 +451,15 @@ // CHECK-FIXES: (*this) = std::make_unique(); } }; + +// Ignore statements inside a template instantiation. +template +void template_fun(T* t) { + std::unique_ptr t2 = std::unique_ptr(new T); + t2.reset(new T); +} + +void invoke_template() { + Foo* foo; + template_fun(foo); +} Index: test/clang-tidy/modernize-use-equals-default-copy.cpp =================================================================== --- test/clang-tidy/modernize-use-equals-default-copy.cpp +++ test/clang-tidy/modernize-use-equals-default-copy.cpp @@ -1,4 +1,6 @@ -// RUN: %check_clang_tidy %s modernize-use-equals-default %t -- -- -std=c++11 -fno-delayed-template-parsing -fexceptions +// RUN: %check_clang_tidy %s modernize-use-equals-default %t -- \ +// RUN: -config="{CheckOptions: [{key: modernize-use-equals-default.IgnoreMacros, value: 0}]}" \ +// RUN: -- -std=c++11 -fno-delayed-template-parsing -fexceptions // Out of line definition. struct OL { Index: test/clang-tidy/modernize-use-equals-default-macros.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-equals-default-macros.cpp @@ -0,0 +1,13 @@ +// RUN: %check_clang_tidy %s modernize-use-equals-default %t -- \ +// RUN: -config="{CheckOptions: [{key: modernize-use-equals-default.IgnoreMacros, value: 0}]}" \ +// RUN: -- -std=c++11 + +#define STRUCT_WITH_DEFAULT(_base, _type) \ + struct _type { \ + _type() {} \ + _base value; \ + }; + +STRUCT_WITH_DEFAULT(unsigned char, InMacro) +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial default constructor +// CHECK-MESSAGES: :[[@LINE-6]]:13: note: Index: test/clang-tidy/modernize-use-equals-default.cpp =================================================================== --- test/clang-tidy/modernize-use-equals-default.cpp +++ test/clang-tidy/modernize-use-equals-default.cpp @@ -204,6 +204,4 @@ _base value; \ }; -STRUCT_WITH_DEFAULT(unsigned char, Hex8Default) -// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial default constructor -// CHECK-MESSAGES: :[[@LINE-6]]:13: note: +STRUCT_WITH_DEFAULT(unsigned char, InMacro) Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -136,18 +137,23 @@ static const std::chrono::seconds DefaultFutureTimeout = std::chrono::seconds(10); +static bool diagsContainErrors(ArrayRef Diagnostics) { + for (const auto &DiagAndFixIts : Diagnostics) { + // FIXME: severities returned by clangd should have a descriptive + // diagnostic severity enum + const int ErrorSeverity = 1; + if (DiagAndFixIts.Diag.severity == ErrorSeverity) + return true; + } + return false; +} + class ErrorCheckingDiagConsumer : public DiagnosticsConsumer { public: void onDiagnosticsReady(PathRef File, Tagged> Diagnostics) override { - bool HadError = false; - for (const auto &DiagAndFixIts : Diagnostics.Value) { - // FIXME: severities returned by clangd should have a descriptive - // diagnostic severity enum - const int ErrorSeverity = 1; - HadError = DiagAndFixIts.Diag.severity == ErrorSeverity; - } + bool HadError = diagsContainErrors(Diagnostics.Value); std::lock_guard Lock(Mutex); HadErrorInLastDiags = HadError; @@ -196,24 +202,47 @@ std::vector ExtraClangFlags; }; +IntrusiveRefCntPtr +buildTestFS(llvm::StringMap const &Files) { + IntrusiveRefCntPtr MemFS( + new vfs::InMemoryFileSystem); + for (auto &FileAndContents : Files) + MemFS->addFile(FileAndContents.first(), time_t(), + llvm::MemoryBuffer::getMemBuffer(FileAndContents.second, + FileAndContents.first())); + + auto OverlayFS = IntrusiveRefCntPtr( + new vfs::OverlayFileSystem(vfs::getTempOnlyFS())); + OverlayFS->pushOverlay(std::move(MemFS)); + return OverlayFS; +} + +class ConstantFSProvider : public FileSystemProvider { +public: + ConstantFSProvider(IntrusiveRefCntPtr FS, + VFSTag Tag = VFSTag()) + : FS(std::move(FS)), Tag(std::move(Tag)) {} + + Tagged> + getTaggedFileSystem(PathRef File) override { + return make_tagged(FS, Tag); + } + +private: + IntrusiveRefCntPtr FS; + VFSTag Tag; +}; + class MockFSProvider : public FileSystemProvider { public: Tagged> getTaggedFileSystem(PathRef File) override { - IntrusiveRefCntPtr MemFS( - new vfs::InMemoryFileSystem); - if (ExpectedFile) + if (ExpectedFile) { EXPECT_EQ(*ExpectedFile, File); + } - for (auto &FileAndContents : Files) - MemFS->addFile(FileAndContents.first(), time_t(), - llvm::MemoryBuffer::getMemBuffer(FileAndContents.second, - FileAndContents.first())); - - auto OverlayFS = IntrusiveRefCntPtr( - new vfs::OverlayFileSystem(vfs::getTempOnlyFS())); - OverlayFS->pushOverlay(std::move(MemFS)); - return make_tagged(OverlayFS, Tag); + auto FS = buildTestFS(Files); + return make_tagged(FS, Tag); } llvm::Optional> ExpectedFile; @@ -272,8 +301,7 @@ MockFSProvider FS; ErrorCheckingDiagConsumer DiagConsumer; MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); - ClangdServer Server(CDB, DiagConsumer, FS, - /*RunSynchronously=*/false); + ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount()); for (const auto &FileWithContents : ExtraFiles) FS.Files[getVirtualTestFilePath(FileWithContents.first)] = FileWithContents.second; @@ -282,7 +310,8 @@ FS.ExpectedFile = SourceFilename; - // Have to sync reparses because RunSynchronously is false. + // Have to sync reparses because requests are processed on the calling + // thread. auto AddDocFuture = Server.addDocument(SourceFilename, SourceContents); auto Result = dumpASTWithoutMemoryLocs(Server, SourceFilename); @@ -334,8 +363,7 @@ MockFSProvider FS; ErrorCheckingDiagConsumer DiagConsumer; MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); - ClangdServer Server(CDB, DiagConsumer, FS, - /*RunSynchronously=*/false); + ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount()); const auto SourceContents = R"cpp( #include "foo.h" @@ -379,8 +407,7 @@ ErrorCheckingDiagConsumer DiagConsumer; MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); - ClangdServer Server(CDB, DiagConsumer, FS, - /*RunSynchronously=*/false); + ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount()); const auto SourceContents = R"cpp( #include "foo.h" @@ -425,16 +452,17 @@ MockFSProvider FS; ErrorCheckingDiagConsumer DiagConsumer; MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); + // Run ClangdServer synchronously. ClangdServer Server(CDB, DiagConsumer, FS, - /*RunSynchronously=*/true); + /*AsyncThreadsCount=*/0); auto FooCpp = getVirtualTestFilePath("foo.cpp"); const auto SourceContents = "int a;"; FS.Files[FooCpp] = SourceContents; FS.ExpectedFile = FooCpp; - // No need to sync reparses, because RunSynchronously is set - // to true. + // No need to sync reparses, because requests are processed on the calling + // thread. FS.Tag = "123"; Server.addDocument(FooCpp, SourceContents); EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS.Tag); @@ -457,8 +485,9 @@ {"-xc++", "-target", "x86_64-linux-unknown", "-m64", "--gcc-toolchain=/randomusr", "-stdlib=libstdc++"}); + // Run ClangdServer synchronously. ClangdServer Server(CDB, DiagConsumer, FS, - /*RunSynchronously=*/true); + /*AsyncThreadsCount=*/0); // Just a random gcc version string SmallString<8> Version("4.9.3"); @@ -487,8 +516,8 @@ )cpp"; FS.Files[FooCpp] = SourceContents; - // No need to sync reparses, because RunSynchronously is set - // to true. + // No need to sync reparses, because requests are processed on the calling + // thread. Server.addDocument(FooCpp, SourceContents); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); @@ -501,6 +530,53 @@ } #endif // LLVM_ON_UNIX +TEST_F(ClangdVFSTest, ForceReparseCompileCommand) { + MockFSProvider FS; + ErrorCheckingDiagConsumer DiagConsumer; + MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); + ClangdServer Server(CDB, DiagConsumer, FS, + /*AsyncThreadsCount=*/0); + // No need to sync reparses, because reparses are performed on the calling + // thread to true. + + auto FooCpp = getVirtualTestFilePath("foo.cpp"); + const auto SourceContents1 = R"cpp( +template +struct foo { T x; }; +)cpp"; + const auto SourceContents2 = R"cpp( +template +struct bar { T x; }; +)cpp"; + + FS.Files[FooCpp] = ""; + FS.ExpectedFile = FooCpp; + + // First parse files in C mode and check they produce errors. + CDB.ExtraClangFlags = {"-xc"}; + Server.addDocument(FooCpp, SourceContents1); + EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); + Server.addDocument(FooCpp, SourceContents2); + EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); + + // Now switch to C++ mode. + CDB.ExtraClangFlags = {"-xc++"}; + // Currently, addDocument never checks if CompileCommand has changed, so we + // expect to see the errors. + Server.addDocument(FooCpp, SourceContents1); + EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); + Server.addDocument(FooCpp, SourceContents2); + EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); + // But forceReparse should reparse the file with proper flags. + Server.forceReparse(FooCpp); + EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); + // Subsequent addDocument calls should finish without errors too. + Server.addDocument(FooCpp, SourceContents1); + EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); + Server.addDocument(FooCpp, SourceContents2); + EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); +} + class ClangdCompletionTest : public ClangdVFSTest { protected: bool ContainsItem(std::vector const &Items, StringRef Name) { @@ -517,8 +593,7 @@ ErrorCheckingDiagConsumer DiagConsumer; MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); - ClangdServer Server(CDB, DiagConsumer, FS, - /*RunSynchronously=*/false); + ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount()); auto FooCpp = getVirtualTestFilePath("foo.cpp"); const auto SourceContents = R"cpp( @@ -568,5 +643,249 @@ } } +class ClangdThreadingTest : public ClangdVFSTest {}; + +TEST_F(ClangdThreadingTest, StressTest) { + // Without 'static' clang gives an error for a usage inside TestDiagConsumer. + static const unsigned FilesCount = 5; + const unsigned RequestsCount = 500; + // Blocking requests wait for the parsing to complete, they slow down the test + // dramatically, so they are issued rarely. Each + // BlockingRequestInterval-request will be a blocking one. + const unsigned BlockingRequestInterval = 40; + + const auto SourceContentsWithoutErrors = R"cpp( +int a; +int b; +int c; +int d; +)cpp"; + + const auto SourceContentsWithErrors = R"cpp( +int a = x; +int b; +int c; +int d; +)cpp"; + + // Giving invalid line and column number should not crash ClangdServer, but + // just to make sure we're sometimes hitting the bounds inside the file we + // limit the intervals of line and column number that are generated. + unsigned MaxLineForFileRequests = 7; + unsigned MaxColumnForFileRequests = 10; + + std::vector> FilePaths; + FilePaths.reserve(FilesCount); + for (unsigned I = 0; I < FilesCount; ++I) + FilePaths.push_back(getVirtualTestFilePath(std::string("Foo") + + std::to_string(I) + ".cpp")); + // Mark all of those files as existing. + llvm::StringMap FileContents; + for (auto &&FilePath : FilePaths) + FileContents[FilePath] = ""; + + ConstantFSProvider FS(buildTestFS(FileContents)); + + struct FileStat { + unsigned HitsWithoutErrors = 0; + unsigned HitsWithErrors = 0; + bool HadErrorsInLastDiags = false; + }; + + class TestDiagConsumer : public DiagnosticsConsumer { + public: + TestDiagConsumer() : Stats(FilesCount, FileStat()) {} + + void onDiagnosticsReady( + PathRef File, + Tagged> Diagnostics) override { + StringRef FileIndexStr = llvm::sys::path::stem(File); + ASSERT_TRUE(FileIndexStr.consume_front("Foo")); + + unsigned long FileIndex = std::stoul(FileIndexStr.str()); + + bool HadError = diagsContainErrors(Diagnostics.Value); + + std::lock_guard Lock(Mutex); + if (HadError) + Stats[FileIndex].HitsWithErrors++; + else + Stats[FileIndex].HitsWithoutErrors++; + Stats[FileIndex].HadErrorsInLastDiags = HadError; + } + + std::vector takeFileStats() { + std::lock_guard Lock(Mutex); + return std::move(Stats); + } + + private: + std::mutex Mutex; + std::vector Stats; + }; + + struct RequestStats { + unsigned RequestsWithoutErrors = 0; + unsigned RequestsWithErrors = 0; + bool LastContentsHadErrors = false; + bool FileIsRemoved = true; + std::future LastRequestFuture; + }; + + std::vector ReqStats; + ReqStats.reserve(FilesCount); + for (unsigned FileIndex = 0; FileIndex < FilesCount; ++FileIndex) + ReqStats.emplace_back(); + + TestDiagConsumer DiagConsumer; + { + MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); + ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount()); + + // Prepare some random distributions for the test. + std::random_device RandGen; + + std::uniform_int_distribution FileIndexDist(0, FilesCount - 1); + // Pass a text that contains compiler errors to addDocument in about 20% of + // all requests. + std::bernoulli_distribution ShouldHaveErrorsDist(0.2); + // Line and Column numbers for requests that need them. + std::uniform_int_distribution LineDist(0, MaxLineForFileRequests); + std::uniform_int_distribution ColumnDist(0, MaxColumnForFileRequests); + + // Some helpers. + auto UpdateStatsOnAddDocument = [&](unsigned FileIndex, bool HadErrors, + std::future Future) { + auto &Stats = ReqStats[FileIndex]; + + if (HadErrors) + ++Stats.RequestsWithErrors; + else + ++Stats.RequestsWithoutErrors; + Stats.LastContentsHadErrors = HadErrors; + Stats.FileIsRemoved = false; + Stats.LastRequestFuture = std::move(Future); + }; + + auto UpdateStatsOnRemoveDocument = [&](unsigned FileIndex, + std::future Future) { + auto &Stats = ReqStats[FileIndex]; + + Stats.FileIsRemoved = true; + Stats.LastRequestFuture = std::move(Future); + }; + + auto UpdateStatsOnForceReparse = [&](unsigned FileIndex, + std::future Future) { + auto &Stats = ReqStats[FileIndex]; + + Stats.LastRequestFuture = std::move(Future); + if (Stats.LastContentsHadErrors) + ++Stats.RequestsWithErrors; + else + ++Stats.RequestsWithoutErrors; + }; + + auto AddDocument = [&](unsigned FileIndex) { + bool ShouldHaveErrors = ShouldHaveErrorsDist(RandGen); + auto Future = Server.addDocument( + FilePaths[FileIndex], ShouldHaveErrors ? SourceContentsWithErrors + : SourceContentsWithoutErrors); + UpdateStatsOnAddDocument(FileIndex, ShouldHaveErrors, std::move(Future)); + }; + + // Various requests that we would randomly run. + auto AddDocumentRequest = [&]() { + unsigned FileIndex = FileIndexDist(RandGen); + AddDocument(FileIndex); + }; + + auto ForceReparseRequest = [&]() { + unsigned FileIndex = FileIndexDist(RandGen); + // Make sure we don't violate the ClangdServer's contract. + if (ReqStats[FileIndex].FileIsRemoved) + AddDocument(FileIndex); + + auto Future = Server.forceReparse(FilePaths[FileIndex]); + UpdateStatsOnForceReparse(FileIndex, std::move(Future)); + }; + + auto RemoveDocumentRequest = [&]() { + unsigned FileIndex = FileIndexDist(RandGen); + // Make sure we don't violate the ClangdServer's contract. + if (ReqStats[FileIndex].FileIsRemoved) + AddDocument(FileIndex); + + auto Future = Server.removeDocument(FilePaths[FileIndex]); + UpdateStatsOnRemoveDocument(FileIndex, std::move(Future)); + }; + + auto CodeCompletionRequest = [&]() { + unsigned FileIndex = FileIndexDist(RandGen); + // Make sure we don't violate the ClangdServer's contract. + if (ReqStats[FileIndex].FileIsRemoved) + AddDocument(FileIndex); + + Position Pos{LineDist(RandGen), ColumnDist(RandGen)}; + Server.codeComplete(FilePaths[FileIndex], Pos); + }; + + auto FindDefinitionsRequest = [&]() { + unsigned FileIndex = FileIndexDist(RandGen); + // Make sure we don't violate the ClangdServer's contract. + if (ReqStats[FileIndex].FileIsRemoved) + AddDocument(FileIndex); + + Position Pos{LineDist(RandGen), ColumnDist(RandGen)}; + Server.findDefinitions(FilePaths[FileIndex], Pos); + }; + + std::vector> AsyncRequests = { + AddDocumentRequest, ForceReparseRequest, RemoveDocumentRequest}; + std::vector> BlockingRequests = { + CodeCompletionRequest, FindDefinitionsRequest}; + + // Bash requests to ClangdServer in a loop. + std::uniform_int_distribution AsyncRequestIndexDist( + 0, AsyncRequests.size() - 1); + std::uniform_int_distribution BlockingRequestIndexDist( + 0, BlockingRequests.size() - 1); + for (unsigned I = 1; I <= RequestsCount; ++I) { + if (I % BlockingRequestInterval != 0) { + // Issue an async request most of the time. It should be fast. + unsigned RequestIndex = AsyncRequestIndexDist(RandGen); + AsyncRequests[RequestIndex](); + } else { + // Issue a blocking request once in a while. + auto RequestIndex = BlockingRequestIndexDist(RandGen); + BlockingRequests[RequestIndex](); + } + } + + // Wait for last requests to finish. + for (auto &ReqStat : ReqStats) { + if (!ReqStat.LastRequestFuture.valid()) + continue; // We never ran any requests for this file. + + // Future should be ready much earlier than in 5 seconds, the timeout is + // there to check we won't wait indefinitely. + ASSERT_EQ(ReqStat.LastRequestFuture.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + } + } // Wait for ClangdServer to shutdown before proceeding. + + // Check some invariants about the state of the program. + std::vector Stats = DiagConsumer.takeFileStats(); + for (unsigned I = 0; I < FilesCount; ++I) { + if (!ReqStats[I].FileIsRemoved) { + ASSERT_EQ(Stats[I].HadErrorsInLastDiags, + ReqStats[I].LastContentsHadErrors); + } + + ASSERT_LE(Stats[I].HitsWithErrors, ReqStats[I].RequestsWithErrors); + ASSERT_LE(Stats[I].HitsWithoutErrors, ReqStats[I].RequestsWithoutErrors); + } +} + } // namespace clangd } // namespace clang