Whamcloud - gitweb
LU-16961 clang: plugins and build system integration
authorTimothy Day <timday@amazon.com>
Mon, 11 Mar 2024 17:38:04 +0000 (10:38 -0700)
committerAndreas Dilger <adilger@whamcloud.com>
Sat, 16 Mar 2024 08:16:57 +0000 (08:16 +0000)
Clang has a plugin system. Compiler extensions can be created
by making a shared library and loading it via the "-fplugin"
options. This makes it simple to implement custom warnings
and static analyzers.

This patch adds a plugin to detect functions that should have
been made static. This plugin has been run over the majority
of the Lustre tree and patches have been submitted for all
warnings. The plugin did not return any false positives in
my testing.

It also add the "--enable-compiler-plugins" configure option,
which automatically builds and sets up the in-tree C compiler
plugins. The option force-enables the plugin regardless of
which compiler is in use. This behavior could be changed if
there is ever a need to support GCC specific plugins.

Also, add the configure checks needed to support building C++
in the Lustre tree. Clang and GCC plugins (and the compilers
themselves) are written in C++.

The license for the plugin mirrors that of the LLVM project
itself. This leaves the door open for contributing this
plugin upstream in the future. This isn't being upstreamed
now because it lacks any significant user community. Hence,
the plugin does not appear to meet the requirements for
upstreaming based on https://clang.llvm.org/get_involved.html.

Lustre-change: https://review.whamcloud.com/51659
Lustre-commit: d684885098c40fee2951feb410bec739717ac9bc

Test-Parameters: trivial
Signed-off-by: Timothy Day <timday@amazon.com>
Change-Id: I747ed91b53e765cc58e91a3eb9ec6c12b9908a96
Reviewed-on: https://review.whamcloud.com/c/ex/lustre-release/+/54350
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
autoMakefile.am
cc-plugins/.gitignore [new file with mode: 0644]
cc-plugins/FindStatic.cpp [new file with mode: 0644]
cc-plugins/Makefile.am [new file with mode: 0644]
config/lustre-build.m4
config/lustre-compiler-plugins.m4 [new file with mode: 0644]
config/lustre-toolchain.m4

index 467a43b..0e517ac 100644 (file)
@@ -88,7 +88,15 @@ endif # LINUX
 
 endif # MODULES
 
-all: undef.h
+all: plugins
+
+# compiler plugins should be built as soon as possible,
+# so they can be available to the rest of the build
+# system
+plugins: undef.h
+if CC_PLUGINS
+       $(MAKE) all -C cc-plugins
+endif # CC_PLUGINS
 
 undef.h: config.h.in
        grep -v config.h.in config.h.in > $@
diff --git a/cc-plugins/.gitignore b/cc-plugins/.gitignore
new file mode 100644 (file)
index 0000000..10a7e8d
--- /dev/null
@@ -0,0 +1 @@
+/Makefile.in
diff --git a/cc-plugins/FindStatic.cpp b/cc-plugins/FindStatic.cpp
new file mode 100644 (file)
index 0000000..46b0a5d
--- /dev/null
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+//
+// This file is part of Lustre, http://www.lustre.org/
+//
+// cc-plugins/FindStatic.cpp
+//
+// Clang plugin to find functions which could be made
+// static.
+//
+// Author: Timothy Day <timday@amazon.com>
+//
+
+#include <clang/AST/AST.h>
+#include <clang/AST/ASTConsumer.h>
+#include <clang/AST/RecursiveASTVisitor.h>
+#include <clang/Frontend/CompilerInstance.h>
+#include <clang/Frontend/FrontendPluginRegistry.h>
+#include <clang/Sema/Sema.h>
+#include <llvm/Support/raw_ostream.h>
+
+using namespace clang;
+
+namespace {
+
+static std::unordered_map<std::string, int> functionMap;
+
+bool isWarnable(FunctionDecl *MethodDecl) {
+  auto isValidDef =
+      MethodDecl->isThisDeclarationADefinition() && MethodDecl->hasBody();
+  auto isNotDeclared =
+      (functionMap.count(MethodDecl->getNameAsString()) == 0);
+  auto isNotStatic = !MethodDecl->isStatic();
+  auto couldBeStatic =
+      MethodDecl->getASTContext().getSourceManager().isInMainFile(
+          MethodDecl->getLocation()) &&
+      !MethodDecl->isMain();
+
+  return isValidDef && isNotDeclared && isNotStatic && couldBeStatic;
+}
+
+class FindStaticDeclVisitor : public RecursiveASTVisitor<FindStaticDeclVisitor> {
+  DiagnosticsEngine &Diags;
+  unsigned WarningFoundStatic;
+
+public:
+  FindStaticDeclVisitor(DiagnosticsEngine &Diags) : Diags(Diags) {
+    WarningFoundStatic = Diags.getCustomDiagID(
+        DiagnosticsEngine::Warning, "Should this function be static?");
+  }
+
+  bool VisitFunctionDecl(FunctionDecl *MethodDecl) {
+    if (isWarnable(MethodDecl)) {
+      Diags.Report(MethodDecl->getLocation(), WarningFoundStatic);
+    }
+
+    return true;
+  }
+};
+
+class ScanDeclVisitor : public RecursiveASTVisitor<ScanDeclVisitor> {
+public:
+  bool VisitFunctionDecl(FunctionDecl *MethodDecl) {
+    if (!isWarnable(MethodDecl)) {
+      functionMap.insert({MethodDecl->getNameAsString(), 1});
+    }
+
+    return true;
+  }
+};
+
+class PrintFunctionsConsumer : public ASTConsumer {
+public:
+  void HandleTranslationUnit(ASTContext &Context) override {
+    ScanDeclVisitor Scanner;
+    Scanner.TraverseDecl(Context.getTranslationUnitDecl());
+
+    FindStaticDeclVisitor Visitor(Context.getDiagnostics());
+    Visitor.TraverseDecl(Context.getTranslationUnitDecl());
+  }
+};
+
+class FindStaticAction : public PluginASTAction {
+  std::set<std::string> ParsedTemplates;
+
+protected:
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 llvm::StringRef) override {
+    return std::make_unique<PrintFunctionsConsumer>();
+  }
+
+  /// Consume plugin arguments; the plugin has no args, so always
+  /// succeed.
+  ///
+  /// \returns true when the parsing succeeds
+  bool ParseArgs(const CompilerInstance &CI,
+                 const std::vector<std::string> &args) override {
+    return true;
+  }
+
+  /// Determines when to run action, in this case, automatically
+  /// after the main AST action.
+  ///
+  /// \returns when the action should run
+  PluginASTAction::ActionType getActionType() override {
+    return AddAfterMainAction;
+  }
+};
+
+} // namespace
+
+static FrontendPluginRegistry::Add<FindStaticAction>
+    X("find-static", "find potential static functions");
diff --git a/cc-plugins/Makefile.am b/cc-plugins/Makefile.am
new file mode 100644 (file)
index 0000000..dd9f405
--- /dev/null
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+#
+# This file is part of Lustre, http://www.lustre.org/
+#
+# cc-plugins/Makefile.am
+#
+# Automake file for compiler plugins.
+#
+
+if CC_PLUGINS
+lib_LTLIBRARIES = libfindstatic.la
+libfindstatic_la_SOURCES = FindStatic.cpp
+endif # CC_PLUGINS
index 6963ad1..a6f6cae 100644 (file)
@@ -694,10 +694,12 @@ AS_IF([test -n "$SNMP_DIST_SUBDIR"], [LS_CONFIGURE])
 LB_CONDITIONALS
 LB_CONFIG_HEADERS
 
+LPLUG_CONFIGURE
 LIBCFS_CONFIG_FILES
 LB_CONFIG_FILES
 LN_CONFIG_FILES
 LC_CONFIG_FILES
+LPLUG_CONFIG_FILES
 AS_IF([test -n "$SNMP_DIST_SUBDIR"], [LS_CONFIG_FILES])
 
 AC_SUBST(ac_configure_args)
diff --git a/config/lustre-compiler-plugins.m4 b/config/lustre-compiler-plugins.m4
new file mode 100644 (file)
index 0000000..6f58dbd
--- /dev/null
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: GPL-2.0
+
+#
+# This file is part of Lustre, http://www.lustre.org/
+#
+# config/lustre-compiler-plugins.m4
+#
+# Configure compliler plugin settings
+#
+
+#
+# LPLUG_ENABLE
+#
+# Simple flag to enable compiler plugins.
+#
+AC_DEFUN([LPLUG_ENABLE], [
+AC_ARG_ENABLE([compiler-plugins],
+    AS_HELP_STRING([--enable-compiler-plugins], [Enable compiler plugins]))
+
+AS_IF([test "x$enable_compiler_plugins" == "xyes"], [
+CFLAGS="$CFLAGS -fplugin=$(pwd)/cc-plugins/.libs/libfindstatic.so"
+], [])
+AM_CONDITIONAL([CC_PLUGINS], [test x$enable_compiler_plugins = xyes])
+]) # LPLUG_ENABLE
+
+#
+# LPLUG_CONFIGURE
+#
+# main configure steps
+#
+AC_DEFUN([LPLUG_CONFIGURE], [
+LPLUG_ENABLE
+]) # LPLUG_CONFIGURE
+
+#
+# LPLUG_CONFIG_FILES
+#
+# files that should be generated with AC_OUTPUT
+#
+AC_DEFUN([LPLUG_CONFIG_FILES], [
+AS_IF([test "x$enable_compiler_plugins" == "xyes"], [
+AC_CONFIG_FILES([
+cc-plugins/Makefile
+])
+], [])
+]) # LPLUG_CONFIG_FILES
index d8c85d8..571df1d 100644 (file)
@@ -31,6 +31,7 @@ fi
 HOSTCC="$LLVM_PREFIX"clang"$LLVM_SUFFIX"
 HOSTCXX="$LLVM_PREFIX"clang++"$LLVM_SUFFIX"
 CC="$LLVM_PREFIX"clang"$LLVM_SUFFIX"
+CXX="$LLVM_PREFIX"clang++"$LLVM_SUFFIX"
 LD="$LLVM_PREFIX"ld.lld"$LLVM_SUFFIX"
 AR="$LLVM_PREFIX"llvm-ar"$LLVM_SUFFIX"
 NM="$LLVM_PREFIX"llvm-nm"$LLVM_SUFFIX"
@@ -179,6 +180,7 @@ AC_DEFUN([LTC_CC_NO_STRINGOP_OVERFLOW], [
 AC_DEFUN([LTC_TOOLCHAIN_CONFIGURE], [
 AC_REQUIRE([LTC_LLVM_TOOLCHAIN])
 AC_REQUIRE([AC_PROG_CC])
+AC_REQUIRE([AC_PROG_CXX])
 
 AM_PROG_AS
 AC_CHECK_TOOLS(AR, ar)
@@ -213,6 +215,7 @@ EXTRA_KCFLAGS: $EXTRA_KCFLAGS
 
 LD:            $LD
 
+CXX:           $CXX
 CPPFLAGS:      $CPPFLAGS
 
 Type 'make' to build Lustre.