Upgrade to LLVM-17 linting

Fix broken CI due to actions-runner. Manually install llvm-17 and make it preferred. The upgrade improves static analysis, leading to the following fixes, uniformities: - misc-include-cleaner: Add missing headers, remove unused headers - Use `\n` instead of `std::endl` - Move includes to included impl files - Fixes to HTML.cc - Add explicit `default: break` for switches - Fix use-after-free
jerinphilip · Oct 27, 2023 · a4512de · a4512de
1 parent abfcd46
commit a4512de
Show file tree

Hide file tree

Showing 39 changed files with 271 additions and 56 deletions.
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
@@ -32,15 +32,26 @@ jobs:
           run: |
             sudo apt-get update
             sudo apt-get install -y build-essential cmake
-            sudo apt-get install -y clang-format clang-tidy
+            # Remove existing installation
+            sudo apt-get purge llvm clang
             python3 -m pip install black isort
             python3 -m pip install cmake-format
             sudo apt-get install -y shfmt
 
+        - name: Setup latest clang
+          run: |
+           sudo bash scripts/ci/update-alternatives-clang.sh 17 100
+
         - name: Run format-script
           run:
             bash scripts/ci/format-check.sh
 
+        - uses: actions/upload-artifact@v2
+          if: always()
+          with:
+            name: format
+            path: ${{ github.workspace }}/build/clang-tidy.*.yml
+
   build-test:
       name: "build-test"
       strategy:

diff --git a/app/main.cc b/app/main.cc
@@ -1,9 +1,16 @@
-#include <fstream>
+#include <cstdio>
+#include <cstdlib>
+#include <future>
 #include <iostream>
+#include <memory>
 #include <sstream>
+#include <string>
+#include <utility>
 
 #include "3rd-party/CLI11.hpp"
-#include "slimt/slimt.hh"
+#include "slimt/Frontend.hh"
+#include "slimt/Model.hh"
+#include "slimt/Response.hh"
 
 inline std::string read_from_stdin() {
   // Read a large input text blob from stdin

diff --git a/scripts/ci/format-check.sh b/scripts/ci/format-check.sh
@@ -19,10 +19,12 @@ function slimt-check-clang-tidy {
 
     # Gemmology, which is default on has to be turned off.
     -DWITH_GEMMOLOGY=OFF
+    -DEXPORT_CMAKE_FILE=OFF
   )
+
   cmake -B build -S . "${ARGS[@]}"
-  run-clang-tidy -p build -header-filter="$PWD/slimt" "$PWD/slimt/.*"
-  run-clang-tidy -p build -header-filter="$PWD/slimt" "$PWD/app/.*"
+  run-clang-tidy -export-fixes build/clang-tidy.slimt.yml -p build -header-filter="$PWD/slimt" "$PWD/slimt/.*"
+  run-clang-tidy -export-fixes build/clang-tidy.app.yml -p build -header-filter="$PWD/slimt" "$PWD/app/.*"
 
 }
 

diff --git a/scripts/ci/update-alternatives-clang.sh b/scripts/ci/update-alternatives-clang.sh
@@ -0,0 +1,69 @@
+# !/bin/bash
+#
+# Taken from comment inside the following gist
+# https://gist.github.com/junkdog/70231d6953592cd6f27def59fe19e50d?permalink_comment_id=4336074#gistcomment-4336074
+# add clang ${version} to Ubuntu
+
+set -eo pipefail
+
+update_alternatives() {
+  local version=${1}
+  local priority=${2}
+  local master=${3}
+  local slaves=${4}
+  local path=${5}
+  local cmdln
+
+  cmdln="--verbose --install ${path}${master} ${master} ${path}${master}-${version} ${priority}"
+  for slave in ${slaves}; do
+    cmdln="${cmdln} --slave ${path}${slave} ${slave} ${path}${slave}-${version}"
+  done
+  sudo update-alternatives ${cmdln}
+  sudo update-alternatives --set ${master} ${path}${master}-${version}
+}
+
+update_alternatives_master() {
+  local version=${1}
+  local priority=${2}
+  local packages=${3}
+  local path=${4}
+  local cmdln
+  for package in ${packages}; do
+    cmdln="--verbose --install ${path}${package} ${package} ${path}${package}-${version} ${priority}"
+    sudo update-alternatives ${cmdln}
+    sudo update-alternatives --set ${package} ${path}${package}-${version}
+  done
+}
+
+if [[ ${#} -ne 2 ]]; then
+  echo usage: "${0}" clang_version priority
+  exit 1
+fi
+
+version=${1}
+priority=${2}
+path="/usr/bin/"
+
+sudo apt update
+
+# download and launch the setup script
+wget https://apt.llvm.org/llvm.sh
+# sudo bash llvm.sh ${version}
+sudo bash llvm.sh ${version} all
+
+master="clang"
+slaves="asan_symbolize bugpoint clang-cpp clangd count dsymutil FileCheck ld64.lld ld.lld llc lld lldb lldb-argdumper lldb-instr lldb-server lldb-vscode lld-link lli lli-child-target not obj2yaml opt sanstats split-file UnicodeNameMappingGenerator verify-uselis
+torder wasm-ld yaml2obj yaml-bench"
+update_alternatives "${version}" "${priority}" "${master}" "${slaves}" "${path}"
+
+packages="clang++ clang-tidy run-clang-tidy"
+update_alternatives_master "${version}" "${priority}" "${packages}" "${path}"
+
+# configure with update-alternatives
+master="llvm-config"
+slaves="llvm-addr2line llvm-ar llvm-as llvm-bcanalyzer llvm-bitcode-strip llvm-cat llvm-cfi-verify llvm-cov llvm-c-test llvm-cvtres llvm-cxxdump llvm-cxxfilt llvm-cxxmap llvm-debuginfo-analyzer llvm-debuginfod llvm-debuginfod-find llvm-diff llvm-dis llvm-dlltool llvm-d
+warfdump llvm-dwarfutil llvm-dwp llvm-exegesis llvm-extract llvm-gsymutil llvm-ifs llvm-install-name-tool llvm-jitlink llvm-jitlink-executor llvm-lib llvm-libtool-darwin llvm-link llvm-lipo llvm-lto llvm-lto2 llvm-mc llvm-mca llvm-ml llvm-modextract llvm-mt llvm-nm llv
+m-objcopy llvm-objdump llvm-opt-report llvm-otool llvm-pdbutil llvm-PerfectShuffle llvm-profdata llvm-profgen llvm-ranlib llvm-rc llvm-readelf llvm-readobj llvm-reduce llvm-remark-size-diff llvm-remarkutil llvm-rtdyld llvm-sim llvm-size llvm-split llvm-stress llvm-stri
+ngs llvm-strip llvm-symbolizer llvm-tapi-diff llvm-tblgen llvm-tli-checker llvm-undname llvm-windres llvm-xray"
+
+update_alternatives "${version}" "${priority}" "${master}" "${slaves}" "${path}"
diff --git a/slimt/Aligned.cc b/slimt/Aligned.cc
@@ -1,6 +1,8 @@
 #include "slimt/Aligned.hh"
 
 #include <cassert>
+#include <cstddef>
+#include <cstdlib>
 
 namespace slimt {
 

diff --git a/slimt/Annotation.cc b/slimt/Annotation.cc
@@ -3,7 +3,9 @@
 #include <cassert>
 #include <cstddef>
 #include <string>
+#include <string_view>
 #include <utility>
+#include <vector>
 
 namespace slimt {
 

diff --git a/slimt/Batcher.cc b/slimt/Batcher.cc
@@ -2,12 +2,16 @@
 
 #include <algorithm>
 #include <cassert>
+#include <cstddef>
+#include <functional>
+#include <memory>
+#include <tuple>
 #include <utility>
 
 #include "slimt/Macros.hh"
 #include "slimt/Model.hh"
 #include "slimt/Request.hh"
-#include "slimt/Utils.hh"
+#include "slimt/Types.hh"
 
 namespace slimt {
 

diff --git a/slimt/Batcher.hh b/slimt/Batcher.hh
@@ -6,6 +6,7 @@
 #include <memory>
 #include <mutex>
 #include <set>
+#include <tuple>
 #include <unordered_map>
 #include <unordered_set>
 #include <utility>

diff --git a/slimt/Frontend.cc b/slimt/Frontend.cc
@@ -1,10 +1,15 @@
 #include "slimt/Frontend.hh"
 
-#include <algorithm>
+#include <cassert>
+#include <cstddef>
 #include <cstdint>
 #include <future>
 #include <memory>
+#include <optional>
+#include <string>
+#include <thread>
 #include <utility>
+#include <vector>
 
 #include "slimt/Annotation.hh"
 #include "slimt/Batcher.hh"
@@ -14,10 +19,10 @@
 #include "slimt/Request.hh"
 #include "slimt/Response.hh"
 #include "slimt/ResponseBuilder.hh"
-#include "slimt/Tensor.hh"
-#include "slimt/TensorOps.hh"
 #include "slimt/TextProcessor.hh"
+#include "slimt/Types.hh"
 #include "slimt/Utils.hh"
+#include "slimt/Vocabulary.hh"
 
 namespace slimt {
 
@@ -247,16 +252,15 @@ std::future<Response> Async::pivot(const Ptr<Model> &first,
   }
 
   // This is callback chaining or CPS due to async.
-  Promise promise;
-  auto future = promise.get_future();
+  auto promise = std::make_shared<Promise>();
+  auto future = promise->get_future();
 
-  auto continuation = [this, &promise, second,
-                       html](Response &&source_to_pivot) {
+  auto continuation = [this, promise, second, html](Response &&partial) {
     // https://stackoverflow.com/a/65606554/4565794
     // Move semantics only work on mutable lambdas, and can only be done once.
     // It's only once in our case, so issok.
-    auto joining_continuation = [source_to_pivot = std::move(source_to_pivot),
-                                 &promise,
+    AnnotatedText intermediate = partial.target;
+    auto joining_continuation = [source_to_pivot = std::move(partial), &promise,
                                  html](Response &&pivot_to_target) mutable {
       // We have both Responses at this callback, source_to_pivot is moved in,
       // second half will be available when complete.
@@ -267,11 +271,11 @@ std::future<Response> Async::pivot(const Ptr<Model> &first,
       if (html) {
         html->restore(response);
       }
-      promise.set_value(std::move(response));
+      promise->set_value(std::move(response));
     };
 
     const TextProcessor &processor = second->processor();
-    auto [annotated, segments] = processor.process(source_to_pivot.target);
+    auto [annotated, segments] = processor.process(intermediate);
 
     auto request =
         make_request(id_, second, cache_, std::move(annotated),

diff --git a/slimt/Frontend.hh b/slimt/Frontend.hh
@@ -3,6 +3,7 @@
 #include <future>
 #include <optional>
 #include <string>
+#include <thread>
 #include <vector>
 
 #include "slimt/Batcher.hh"
@@ -14,6 +15,8 @@
 namespace slimt {
 
 class Model;
+struct Options;
+struct Response;
 
 struct SLIMT_EXPORT Config {
   // NOLINTBEGIN

diff --git a/slimt/HTML.cc b/slimt/HTML.cc
@@ -3,10 +3,16 @@
 #include <algorithm>
 #include <cassert>
 #include <cctype>
-#include <compare>
+#include <cstddef>
 #include <iterator>
+#include <ostream>
 #include <sstream>
+#include <string>
+#include <string_view>
+#include <utility>
+#include <vector>
 
+#include "slimt/Annotation.hh"
 #include "slimt/Macros.hh"
 #include "slimt/Response.hh"
 #include "slimt/Types.hh"
@@ -386,7 +392,8 @@ HTML::HTML(std::string &source, Options &&options)
   bool add_word_break = false;  // whether to add a word break next text segment
 
   // Starting point: an empty span with no open tags.
-  spans_.push_back(Span{0, 0, {}});
+  Span start{.begin = 0, .end = 0, .tags = {}};
+  spans_.push_back(std::move(start));
 
   bool stop = false;
   while (!stop) {

diff --git a/slimt/Input.cc b/slimt/Input.cc
@@ -2,6 +2,9 @@
 
 #include <algorithm>
 #include <cassert>
+#include <cstddef>
+#include <cstdint>
+#include <vector>
 
 #include "slimt/Tensor.hh"
 

diff --git a/slimt/Io.cc b/slimt/Io.cc
@@ -3,17 +3,21 @@
 #include <fcntl.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <unistd.h>
 
 #include <cassert>
 #include <cstdint>
 #include <cstdlib>
 #include <iostream>
 #include <stdexcept>
+#include <string>
 #include <utility>
 #include <vector>
 
+#include "slimt/Aligned.hh"
 #include "slimt/QMM.hh"
 #include "slimt/Tensor.hh"
+#include "slimt/Types.hh"
 
 namespace slimt::io {
 

diff --git a/slimt/Io.hh b/slimt/Io.hh
@@ -12,6 +12,7 @@
 
 #include "slimt/Aligned.hh"
 #include "slimt/Tensor.hh"
+#include "slimt/Types.hh"
 
 namespace slimt {
 

diff --git a/slimt/Macros.hh b/slimt/Macros.hh
@@ -3,19 +3,19 @@
 
 #define SLIMT_BREAK std::raise(SIGTRAP)
 
-#define SLIMT_TRACE(x)                                              \
-  do {                                                              \
-    std::cerr << __FILE__ << ":" << __LINE__;                       \
-    std::cerr << " " << __FUNCTION__ << " ";                        \
-    std::cerr << #x << ": " << std::scientific << (x) << std::endl; \
+#define SLIMT_TRACE(x)                                         \
+  do {                                                         \
+    std::cerr << __FILE__ << ":" << __LINE__;                  \
+    std::cerr << " " << __FUNCTION__ << " ";                   \
+    std::cerr << #x << ": " << std::scientific << (x) << '\n'; \
   } while (0)
 
-#define SLIMT_TRACE_BLOCK(x)                                        \
-  do {                                                              \
-    std::cerr << __FILE__ << ":" << __LINE__;                       \
-    std::cerr << " " << __FUNCTION__ << " \n\n";                    \
-    std::cerr << #x << ": " << std::scientific << (x) << std::endl; \
-    std::cerr << "\n\n";                                            \
+#define SLIMT_TRACE_BLOCK(x)                                   \
+  do {                                                         \
+    std::cerr << __FILE__ << ":" << __LINE__;                  \
+    std::cerr << " " << __FUNCTION__ << " \n\n";               \
+    std::cerr << #x << ": " << std::scientific << (x) << '\n'; \
+    std::cerr << "\n\n";                                       \
   } while (0);
 
 #define SLIMT_TRACE2(x, y) \

diff --git a/slimt/Model.cc b/slimt/Model.cc
@@ -1,14 +1,21 @@
 #include "slimt/Model.hh"
 
-#include <cmath>
+#include <algorithm>
+#include <cstddef>
 #include <cstdint>
-#include <iostream>
-#include <unordered_map>
+#include <memory>
+#include <string>
 #include <utility>
+#include <vector>
 
+#include "slimt/Aligned.hh"
 #include "slimt/Input.hh"
-#include "slimt/Modules.hh"
+#include "slimt/Io.hh"
+#include "slimt/Shortlist.hh"
+#include "slimt/Tensor.hh"
 #include "slimt/TensorOps.hh"
+#include "slimt/Transformer.hh"
+#include "slimt/Types.hh"
 #include "slimt/Vocabulary.hh"
 
 namespace slimt {