[clang-tidy] safety-no-assembler

[jbcoe]

Summary

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

Include a check for inline assembler.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

60 ↗

(On Diff #86234)

Please take look on Release Notes format in previous releases branches (3.9 and 40).

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

9 ↗

(On Diff #86234)

Link to particular standard will be helpful.

[jbcoe]

Add link to HIC++ website and fix release notes.

[idlecode]

Standard you linked mentions portability as the reason inline assembler should be avoided. Should it really be named 'safety'?

[jbcoe]

High Integrity C++ is often used as a standard for safety-critical systems. High Integrity C++ requires no assembler due to portability issues. Not my choice of wording.
I plan to implement more of the High Integrity C++ checks, some of which are more obviously safety-related.

[idlecode]

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

156 ↗

(On Diff #86289)

safety-no-vector-bool seems to belong to the other patch.

clang-tools-extra/test/clang-tidy/safety-no-assembler.cpp

2 ↗

(On Diff #86289)

Maybe this check should handle FileScopeAsmDecl and AsmLabelAttr too?

__asm__(".symver foo, bar@v");
static int s asm("spam");

[jbcoe]

clang-tools-extra/test/clang-tidy/safety-no-assembler.cpp

2 ↗

(On Diff #86289)

Agreed.

[aaron.ballman]

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

32 ↗

(On Diff #86289)

The diagnostic text doesn't help the user to understand why the code is being diagnosed. Also, does printing the source text add any clarity? The diagnostic already appears on the line in which the assembly statement appears, and since this is a statement (rather than an expression), it seems unlikely to be useful to repeat that text.

[alexfh]

I wonder whether there's a compiler diagnostic for this purpose. Compiler diagnostics are more efficient at reaching users and should be preferred where they are appropriate (this seems like one of such cases).

[jbcoe]

Improve diagnostic message. Find other sorts of inline assembler. Minor fixes for other review comments.

[jbcoe]

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

32 ↗

(On Diff #86289)

Agreed. Fixed.

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

156 ↗

(On Diff #86289)

Well spotted. Thought I'd got rid of all of those.

[JonasToth]

out of curiousity:
i would like to contribute to the safety module as well. but currently there is no starting point in master. is there a way to get some code that i can start with? I think the module file where the registration happens would be enough.

this is not specifically patch related, but dunno where to ask else.

e.g. could it be possible to commit the std::vector<bool> thing, even though temlate parameters dont work right now. this could be added as a known limitation.

[jbcoe]

@JonasToth My main intention with this patch is to provide such a starting point. A few people have mentioned that they'd be keen to contribute checks.

[JonasToth]

alright :)

[jbcoe]

@alexfh Can we defer moving this to a compiler diagnostic? I'm keen to get a target in place for people to write more safety checks.

[JonasToth]

In D29267#667614, @jbcoe wrote:

@alexfh Can we defer moving this to a compiler diagnostic? I'm keen to get a target in place for people to write more safety checks.

+1 from me :)
and assembler might be bad style, but is it worth a warning? I think people writing assembly in their code wouldnt be amused and turn it off anyway, since they believe its necessary and worth it.

[idlecode]

LGTM. HIC++ Standard seems worth implementing.

[aaron.ballman]

In D29267#667487, @alexfh wrote:

I wonder whether there's a compiler diagnostic for this purpose. Compiler diagnostics are more efficient at reaching users and should be preferred where they are appropriate (this seems like one of such cases).

I don't think a compiler diagnostic would be appropriate for this. It would be incredibly chatty for people who do need to use the assembler.

[jbcoe]

@idlecode please can you approve if it LGTY?

[idlecode]

Sure, but I don't have commit rights so someone else have to push it :)

[aaron.ballman]

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

13 ↗

(On Diff #87207)

Is this include needed?

36 ↗

(On Diff #87207)

No need for this to be an Optional, is there?

[Eugene.Zelenko]

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

13 ↗

(On Diff #87207)

Will be good idea to run Include What You Use.

[aaron.ballman]

There were review comments still outstanding when you commit the patch. Can you please address those post-commit?

[jbcoe]

@aaron.ballman I will address post-approval comments post-commit.

[jbcoe]

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

13 ↗

(On Diff #87207)

Fixed unnecessary include.

36 ↗

(On Diff #87207)

I think optional makes intent clearer but am used to dealing with immutable objects.
I've changed the code to just use SourceLocation.

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

 add_subdirectory(llvm)
 add_subdirectory(cppcoreguidelines)
 add_subdirectory(google)
 add_subdirectory(misc)
 add_subdirectory(modernize)
 add_subdirectory(mpi)
 add_subdirectory(performance)
 add_subdirectory(readability)
+add_subdirectory(safety)
 add_subdirectory(utils)

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

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

clang-tools-extra/trunk/clang-tidy/safety/NoAssemblerCheck.h

+//===--- NoAssemblerCheck.h - clang-tidy-------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_SAFETY_NO_ASSEMBLER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_SAFETY_NO_ASSEMBLER_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace safety {
+
+/// Find assembler statements. No fix is offered.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/safety-no-assembler.html
+class NoAssemblerCheck : public ClangTidyCheck {
+public:
+  NoAssemblerCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace safety
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_SAFETY_NO_ASSEMBLER_H

clang-tools-extra/trunk/clang-tidy/safety/NoAssemblerCheck.cpp

+//===--- NoAssemblerCheck.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 "NoAssemblerCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace ast_matchers {
+AST_MATCHER(VarDecl, isAsm) { return Node.hasAttr<clang::AsmLabelAttr>(); }
+const internal::VariadicDynCastAllOfMatcher<Decl, FileScopeAsmDecl>
+    fileScopeAsmDecl;
+}
+}
+
+namespace clang {
+namespace tidy {
+namespace safety {
+
+void NoAssemblerCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(asmStmt().bind("asm-stmt"), this);
+  Finder->addMatcher(fileScopeAsmDecl().bind("asm-file-scope"), this);
+  Finder->addMatcher(varDecl(isAsm()).bind("asm-var"), this);
+}
+
+void NoAssemblerCheck::check(const MatchFinder::MatchResult &Result) {
+  Optional<SourceLocation> ASMLocation;
+  if (const auto *ASM = Result.Nodes.getNodeAs<AsmStmt>("asm-stmt"))
+    ASMLocation = ASM->getAsmLoc();
+  else if (const auto *ASM =
+               Result.Nodes.getNodeAs<FileScopeAsmDecl>("asm-file-scope"))
+    ASMLocation = ASM->getAsmLoc();
+  else if (const auto *ASM = Result.Nodes.getNodeAs<VarDecl>("asm-var"))
+    ASMLocation = ASM->getLocation();
+  else
+    llvm_unreachable("Unhandled case in matcher.");
+
+  diag(*ASMLocation, "do not use inline assembler in safety-critical code");
+}
+
+} // namespace safety
+} // namespace tidy
+} // namespace clang

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

+//===------- SafetyTidyModule.cpp - clang-tidy ----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "NoAssemblerCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace safety {
+
+class SafetyModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<NoAssemblerCheck>(
+        "safety-no-assembler");
+  }
+};
+
+// Register the SafetyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<SafetyModule>
+    X("safety-module", "Adds safety-critical checks.");
+
+} // namespace safety
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the SafetyModule.
+volatile int SafetyModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

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

target_link_libraries(clang-tidy
   clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
   clangTidyMPIModule
   clangTidyPerformanceModule
   clangTidyReadabilityModule
+  clangTidySafetyModule
   clangTooling
   )
 
 install(TARGETS clang-tidy
   RUNTIME DESTINATION bin)
 
 install(PROGRAMS clang-tidy-diff.py DESTINATION share/clang)
 install(PROGRAMS run-clang-tidy.py DESTINATION share/clang)

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

 static int LLVM_ATTRIBUTE_UNUSED PerformanceModuleAnchorDestination =
     PerformanceModuleAnchorSource;
 
 // This anchor is used to force the linker to link the ReadabilityModule.
 extern volatile int ReadabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination =
     ReadabilityModuleAnchorSource;
 
+// This anchor is used to force the linker to link the SafetyModule.
+extern volatile int SafetyModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED SafetyModuleAnchorDestination =
+    SafetyModuleAnchorSource;
+
 } // namespace tidy
 } // namespace clang
 
 int main(int argc, const char **argv) {
   return clang::tidy::clangTidyMain(argc, argv);
 }

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
-The improvements are...
+- New `safety-no-assembler
+  <http://clang.llvm.org/extra/clang-tidy/checks/safety-no-assembler.html>`_ check
+
+  Finds uses of inline assembler.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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

.. toctree::
    readability-redundant-function-ptr-dereference
    readability-redundant-member-init
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release
+   safety-no-assembler

clang-tools-extra/trunk/docs/clang-tidy/checks/safety-no-assembler.rst

+.. title:: clang-tidy - safety-no-assembler
+
+safety-no-assembler
+===================
+
+Check for assembler statements. No fix is offered.
+
+Inline assembler is forbidden by safety-critical C++ standards like `High
+Intergrity C++ <http://www.codingstandard.com>`_ as it restricts the
+portability of code.

clang-tools-extra/trunk/test/clang-tidy/safety-no-assembler.cpp

+// RUN: %check_clang_tidy %s safety-no-assembler %t
+
+__asm__(".symver foo, bar@v");
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not use inline assembler in safety-critical code [safety-no-assembler]
+
+static int s asm("spam");
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use inline assembler in safety-critical code [safety-no-assembler]
+
+void f() {
+  __asm("mov al, 2");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use inline assembler in safety-critical code [safety-no-assembler]
+}
+