diff --git a/clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt b/clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt @@ -4,6 +4,7 @@ ) add_clang_library(clangTidyLinuxKernelModule + ExtraSemiCheck.cpp LinuxKernelTidyModule.cpp MustCheckErrsCheck.cpp diff --git a/clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h b/clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h @@ -0,0 +1,45 @@ +//===--- ExtraSemiCheck.h - clang-tidy --------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMICHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMICHECK_H + +#include "../ClangTidyCheck.h" +#include "clang/Lex/MacroInfo.h" +#include + +namespace clang { +namespace tidy { +namespace linuxkernel { + +class ExtraSemiCheck : public ClangTidyCheck { + + enum ExtraSemiFixerKind { ESFK_None, ESFK_Switch, ESFK_TrailingMacro }; + +public: + ExtraSemiCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override final; + + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void checkMacro(SourceManager &SM, const Token &MacroNameTok, + const MacroInfo *MI); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + std::vector SuspectMacros; + enum ExtraSemiFixerKind FixerKind; + const std::string ExtraSemiFixerKindName; +}; + +} // namespace linuxkernel +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMICHECK_H diff --git a/clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp b/clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp @@ -0,0 +1,205 @@ +//===--- ExtraSemiCheck.cpp - clang-tidy ----------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ExtraSemiCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace linuxkernel { + +class ExtraSemiCheckPPCallbacks : public PPCallbacks { +public: + ExtraSemiCheckPPCallbacks(Preprocessor *PP, ExtraSemiCheck *Check) + : PP(PP), Check(Check) {} + + void MacroDefined(const Token &MacroNameTok, + const MacroDirective *MD) override { + const MacroInfo *MI = MD->getMacroInfo(); + + if (MI->isBuiltinMacro() || MI->isObjectLike()) + return; + Check->checkMacro(PP->getSourceManager(), MacroNameTok, MI); + } + +private: + Preprocessor *PP; + ExtraSemiCheck *Check; +}; + +ExtraSemiCheck::ExtraSemiCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), FixerKind(ESFK_None), + ExtraSemiFixerKindName(Options.get("Fixer", ExtraSemiFixerKindName)) { + if (ExtraSemiFixerKindName == "Switch") + FixerKind = ESFK_Switch; + else if (ExtraSemiFixerKindName == "TrailingMacro") + FixerKind = ESFK_TrailingMacro; +} + +void ExtraSemiCheck::registerMatchers(MatchFinder *Finder) { + if (FixerKind == ESFK_Switch) { + // From the reproducer + // void foo (int a) { + // switch (a) {}; + // } + // The AST + // `-FunctionDecl + // |-ParmVarDecl + // `-CompoundStmt <--- "comp", 'C' + // |-SwitchStmt <-- "switch", 'S' + // | |-ImplicitCastExpr + // | | `-DeclRefExpr + // | `-CompoundStmt + // `-NullStmt <-------------- 'N' + Finder->addMatcher( + compoundStmt(has(switchStmt().bind("switch"))).bind("comp"), this); + } else if (FixerKind == ESFK_TrailingMacro) { + // From the reproducer + // #define M(a) a++; + // int f() { + // int v = 0; + // M(v); + // return v; + // } + // The AST + // `-FunctionDecl + // `-CompoundStmt <--- "comp", 'C' + // ... + // |-UnaryOperator + // | `-DeclRefExpr <-------- 'E' + // |-NullStmt <------ "null" 'N' + // ... + Finder->addMatcher(compoundStmt(has(nullStmt().bind("null"))).bind("comp"), + this); + } +} + +void ExtraSemiCheck::registerPPCallbacks(const SourceManager &SM, + Preprocessor *PP, + Preprocessor *ModuleExpanderPP) { + if (FixerKind == ESFK_TrailingMacro) { + ModuleExpanderPP->addPPCallbacks( + std::make_unique(ModuleExpanderPP, this)); + } +} + +void ExtraSemiCheck::check(const MatchFinder::MatchResult &Result) { + if (FixerKind == ESFK_Switch) { + const auto *C = Result.Nodes.getNodeAs("comp"); + const auto *S = Result.Nodes.getNodeAs("switch"); + + auto Current = C->body_begin(); + auto Next = Current; + Next++; + while (Next != C->body_end()) { + if (*Current == S) { + if (const auto *N = dyn_cast(*Next)) { + // This code has the same AST as the reproducer + // + // #define EMPTY() + // void foo (int a) { + // switch (a) {} EMPTY(); + // } + // + // But we do not want to remove the ; because the + // macro may only be conditionally empty. + // ex/ the release version of a debug macro + // + // So check that the NullStmt does not have a + // leading empty macro. + if (!N->hasLeadingEmptyMacro() && + S->getBody()->getEndLoc().isValid() && + N->getSemiLoc().isValid()) { + auto H = FixItHint::CreateReplacement( + SourceRange(S->getBody()->getEndLoc(), N->getSemiLoc()), "}"); + diag(N->getSemiLoc(), "unneeded semicolon") << H; + break; + } + } + } + Current = Next; + Next++; + } + } else if (FixerKind == ESFK_TrailingMacro) { + const auto *C = Result.Nodes.getNodeAs("comp"); + const auto *N = Result.Nodes.getNodeAs("null"); + + auto Current = C->body_begin(); + auto Next = Current; + Next++; + while (Next != C->body_end()) { + + if (*Next == N) { + // This code has the same AST as the reproducer + // + // #define NOT_EMPTY(a) a++; + // #define EMPTY() + // void foo (int a) { + // NOT_EMPTY(a); + // EMPTY(); + // } + // + // But we do not want to remove the ; because the + // macro may only be conditionally empty. + // ex/ the release version of a debug macro + // + // So check that the NullStmt does not have a + // leading empty macro. + if (!N->hasLeadingEmptyMacro() && N->getEndLoc().isValid()) { + if (const auto *E = dyn_cast(*Current)) { + SourceLocation Loc = E->getEndLoc(); + if (Loc.isMacroID()) { + SourceManager &SM = Result.Context->getSourceManager(); + FullSourceLoc SpellingLoc = + FullSourceLoc(Loc, SM).getSpellingLoc(); + + for (const MacroInfo *MI : SuspectMacros) { + auto TI = MI->tokens_begin(); + for (; TI != MI->tokens_end(); TI++) { + SourceLocation L = TI->getLocation(); + if (SpellingLoc == L) + break; + } + if (TI != MI->tokens_end()) { + const Token &Tok = + MI->getReplacementToken(MI->getNumTokens() - 1); + SourceLocation FixLoc = Tok.getLocation(); + SourceLocation EndLoc = Tok.getEndLoc(); + diag(FixLoc, "unneeded semicolon") + << FixItHint::CreateRemoval(SourceRange(FixLoc, EndLoc)); + break; + } + } + } + } + } + } + Current = Next; + Next++; + } + } +} +void ExtraSemiCheck::checkMacro(SourceManager &SM, const Token &MacroNameTok, + const MacroInfo *MI) { + unsigned Num = MI->getNumTokens(); + if (Num && MI->getReplacementToken(Num - 1).is(tok::semi)) + SuspectMacros.push_back(MI); +} + +void ExtraSemiCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "Fixer", ""); +} + +} // namespace linuxkernel +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp b/clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp --- a/clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp @@ -9,6 +9,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "ExtraSemiCheck.h" #include "MustCheckErrsCheck.h" namespace clang { @@ -19,6 +20,7 @@ class LinuxKernelModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck("linuxkernel-extra-semi"); CheckFactories.registerCheck( "linuxkernel-must-check-errs"); } diff --git a/clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c b/clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c @@ -0,0 +1,32 @@ +// RUN: %check_clang_tidy %s linuxkernel-extra-semi %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: [{key: linuxkernel-extra-semi.Fixer, \ +// RUN: value: 'TrailingMacro'}, \ +// RUN: ]}" + +#define M(a) a++; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: unneeded semicolon +// CHECK-FIXES: #define M(a) a++{{$}} + +int f() { + int v = 0; + M(v); + return v; +} + +// A corner case +// An empty macro could conditionally contain another path so +// do not warn or fix. +#ifdef UNSET_CONDITION +#define E() ; +#else +#define E() +#endif +#define N(a) a++; + +int g() { + int v = 0; + N(v) + E(); + return v; +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/linuxkernel-switch-semi.c b/clang-tools-extra/test/clang-tidy/checkers/linuxkernel-switch-semi.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/linuxkernel-switch-semi.c @@ -0,0 +1,41 @@ +// RUN: %check_clang_tidy %s linuxkernel-extra-semi %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: [{key: linuxkernel-extra-semi.Fixer, \ +// RUN: value: 'Switch'}, \ +// RUN: ]}" + +int f(int a) { + switch (a) { + case 1: + return 0; + default: + break; + }; + // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: unneeded semicolon + // CHECK-FIXES: }{{$}} + return 0; +} + +// A normal switch, should not produce a warning +int p1(int a) { + switch (a) { + case 1: + return 0; + default: + break; + } + return 0; +} + +#define EMPTY_MACRO() +// A corner case, do not fix an empty macro +int p2(int a) { + switch (a) { + case 1: + return 0; + default: + break; + } + EMPTY_MACRO(); + return 0; +}