build: add support for Clang-Tidy tool. (proxy-wasm#263)

Signed-off-by: Piotr Sikora <[email protected]>

Author: PiotrSikora

Diff for: .bazelrc

@@ -40,6 +40,11 @@ build:clang-tsan --copt -fsanitize=thread
 build:clang-tsan --linkopt -fsanitize=thread
 build:clang-tsan --linkopt -fuse-ld=lld
 
+# Use Clang-Tidy tool.
+build:clang-tidy --aspects @bazel_clang_tidy//clang_tidy:clang_tidy.bzl%clang_tidy_aspect
+build:clang-tidy --@bazel_clang_tidy//:clang_tidy_config=@proxy_wasm_cpp_host//:clang_tidy_config
+build:clang-tidy --output_groups=report
+
 # Use GCC compiler.
 build:gcc --action_env=BAZEL_COMPILER=gcc
 build:gcc --action_env=CC=gcc

Diff for: .clang-tidy

@@ -0,0 +1,23 @@
+Checks:            clang-*,
+                   -clang-analyzer-optin.portability.UnixAPI,
+                   -clang-analyzer-unix.Malloc,
+                   -clang-diagnostic-pragma-once-outside-header,
+                   cppcoreguidelines-pro-type-member-init,
+                   cppcoreguidelines-pro-type-static-cast-downcast,
+                   misc-*,
+                   -misc-non-private-member-variables-in-classes,
+                   modernize-*,
+                   -modernize-avoid-c-arrays,
+                   -modernize-use-trailing-return-type,
+                   llvm-include-order,
+                   performance-*,
+                   -performance-no-int-to-ptr,
+                   portability-*,
+                   readability-*,
+                   -readability-convert-member-functions-to-static,
+                   -readability-function-cognitive-complexity,
+                   -readability-magic-numbers,
+                   -readability-make-member-function-const,
+                   -readability-simplify-boolean-expr,
+
+WarningsAsErrors:  '*'

Diff for: .github/workflows/format.yml

@@ -28,6 +28,9 @@ on:
     - 'envoy-release/**'
     - 'istio-release/**'
 
+  schedule:
+    - cron: '0 0 * * *'
+
 jobs:
 
   addlicense:
@@ -105,3 +108,61 @@ jobs:
       run: |
         find . -name "*.h" -o -name "*.cc" -o -name "*.proto" | grep -v ".pb." | xargs -n1 clang-format-12 -i
         git diff --exit-code
+
+  clang_tidy:
+    name: check format with clang-tidy
+
+    runs-on: ubuntu-20.04
+
+    steps:
+    - uses: actions/checkout@v2
+
+    - name: Install dependencies (Linux)
+      run: sudo apt-get install -y clang-tidy-12
+
+    - name: Bazel cache
+      uses: PiotrSikora/[email protected]
+      with:
+        path: |
+          ~/.cache/bazel
+        key: clang_tidy-${{ hashFiles('WORKSPACE', '.bazelrc', '.bazelversion', 'bazel/dependencies.bzl', 'bazel/repositories.bzl', 'bazel/cargo/wasmsign/crates.bzl') }}
+
+    - name: Bazel build
+      run: >
+        bazel build
+        --config clang-tidy
+        --define engine=multi
+        --copt=-DPROXY_WASM_VERIFY_WITH_ED25519_PUBKEY=\"$(xxd -p -c 256 test/test_data/signature_key1.pub | cut -b9-)\"
+        //...
+
+    - name: Skip Bazel cache update
+      if: ${{ github.ref != 'refs/heads/master' }}
+      run: echo "CACHE_SKIP_SAVE=true" >> $GITHUB_ENV
+
+    - name: Cleanup Bazel cache
+      if: ${{ github.ref == 'refs/heads/master' }}
+      run: |
+        export OUTPUT=$(${{ matrix.run_under }} bazel info output_base)
+        echo "===== BEFORE ====="
+        du -s ${OUTPUT}/external/* $(dirname ${OUTPUT})/* | sort -rn | head -20
+        # BoringSSL's test data (90 MiB).
+        rm -rf ${OUTPUT}/external/boringssl/crypto_test_data.cc
+        rm -rf ${OUTPUT}/external/boringssl/src/crypto/*/test/
+        rm -rf ${OUTPUT}/external/boringssl/src/third_party/wycheproof_testvectors/
+        # LLVM's tests (500 MiB).
+        rm -rf ${OUTPUT}/external/llvm*/test/
+        # V8's tests (100 MiB).
+        if [ -d "${OUTPUT}/external/v8/test/torque" ]; then
+          mv ${OUTPUT}/external/v8/test/torque ${OUTPUT}/external/v8/test_torque
+          rm -rf ${OUTPUT}/external/v8/test/*
+          mv ${OUTPUT}/external/v8/test_torque ${OUTPUT}/external/v8/test/torque
+        fi
+        # Unnecessary CMake tools (65 MiB).
+        rm -rf ${OUTPUT}/external/cmake-*/bin/{ccmake,cmake-gui,cpack,ctest}
+        # Distfiles for Rust toolchains (350 MiB).
+        rm -rf ${OUTPUT}/external/rust_*/*.tar.gz
+        # Bazel's repository cache (650-800 MiB) and install base (155 MiB).
+        rm -rf ${OUTPUT}/../cache
+        rm -rf ${OUTPUT}/../install
+        echo "===== AFTER ====="
+        du -s ${OUTPUT}/external/* $(dirname ${OUTPUT})/* | sort -rn | head -20

Diff for: BUILD

@@ -14,6 +14,11 @@ package(default_visibility = ["//visibility:public"])
 
 exports_files(["LICENSE"])
 
+filegroup(
+    name = "clang_tidy_config",
+    data = [".clang-tidy"],
+)
+
 cc_library(
     name = "wasm_vm_headers",
     hdrs = [
@@ -191,7 +196,7 @@ cc_library(
     ],
     hdrs = ["include/proxy-wasm/wavm.h"],
     copts = [
-        '-DWAVM_API=""',
+        "-DWAVM_API=",
         "-Wno-non-virtual-dtor",
         "-Wno-old-style-cast",
     ],

Diff for: bazel/external/bazel_clang_tidy.patch

@@ -0,0 +1,27 @@
+# 1. Treat .h files as C++ headers.
+# 2. Hardcode clang-tidy-12.
+
+diff --git a/clang_tidy/clang_tidy.bzl b/clang_tidy/clang_tidy.bzl
+index 3a5ed07..5db5c6c 100644
+--- a/clang_tidy/clang_tidy.bzl
++++ b/clang_tidy/clang_tidy.bzl
+@@ -16,6 +16,9 @@ def _run_tidy(ctx, exe, flags, compilation_context, infile, discriminator):
+     # add source to check
+     args.add(infile.path)
+ 
++    # treat .h files as C++ headers
++    args.add("--extra-arg-before=-xc++")
++
+     # start args passed to the compiler
+     args.add("--")
+ 
+diff --git a/clang_tidy/run_clang_tidy.sh b/clang_tidy/run_clang_tidy.sh
+index 582bab1..b9ebb94 100755
+--- a/clang_tidy/run_clang_tidy.sh
++++ b/clang_tidy/run_clang_tidy.sh
+@@ -11,4 +11,4 @@ shift
+ touch $OUTPUT
+ truncate -s 0 $OUTPUT
+ 
+-clang-tidy "$@"
++clang-tidy-12 "$@"

Diff for: bazel/repositories.bzl

@@ -29,6 +29,16 @@ def proxy_wasm_cpp_host_repositories():
         sha256 = "c6966ec828da198c5d9adbaa94c05e3a1c7f21bd012a0b29ba8ddbccb2c93b0d",
     )
 
+    maybe(
+        http_archive,
+        name = "bazel_clang_tidy",
+        sha256 = "6ed23cbff9423a30ef10becf57210a26d54fe198a211f4037d931c06f843c023",
+        strip_prefix = "bazel_clang_tidy-c2fe98cfec0430e78bff4169e9ca0a43123e4c99",
+        url = "https://github.com/erenon/bazel_clang_tidy/archive/c2fe98cfec0430e78bff4169e9ca0a43123e4c99.tar.gz",
+        patches = ["@proxy_wasm_cpp_host//bazel/external:bazel_clang_tidy.patch"],
+        patch_args = ["-p1"],
+    )
+
     maybe(
         http_archive,
         name = "bazel-zig-cc",

Diff for: include/proxy-wasm/bytecode_util.h

@@ -69,7 +69,7 @@ class BytecodeUtil {
   static bool getStrippedSource(std::string_view bytecode, std::string &ret);
 
 private:
-  static bool parseVarint(const char *&begin, const char *end, uint32_t &ret);
+  static bool parseVarint(const char *&pos, const char *end, uint32_t &ret);
 };
 
 } // namespace proxy_wasm

Diff for: include/proxy-wasm/context.h

@@ -143,11 +143,11 @@ class ContextBase : public RootInterface,
                     public SharedQueueInterface,
                     public GeneralInterface {
 public:
-  ContextBase();                                                   // Testing.
-  ContextBase(WasmBase *wasm);                                     // Vm Context.
-  ContextBase(WasmBase *wasm, std::shared_ptr<PluginBase> plugin); // Root Context.
+  ContextBase();                                                          // Testing.
+  ContextBase(WasmBase *wasm);                                            // Vm Context.
+  ContextBase(WasmBase *wasm, const std::shared_ptr<PluginBase> &plugin); // Root Context.
   ContextBase(WasmBase *wasm, uint32_t parent_context_id,
-              std::shared_ptr<PluginHandleBase> plugin_handle); // Stream context.
+              const std::shared_ptr<PluginHandleBase> &plugin_handle); // Stream context.
   virtual ~ContextBase();
 
   WasmBase *wasm() const { return wasm_; }
@@ -196,11 +196,11 @@ class ContextBase : public RootInterface,
 
   // HTTP
   FilterHeadersStatus onRequestHeaders(uint32_t headers, bool end_of_stream) override;
-  FilterDataStatus onRequestBody(uint32_t body_buffer_length, bool end_of_stream) override;
+  FilterDataStatus onRequestBody(uint32_t body_length, bool end_of_stream) override;
   FilterTrailersStatus onRequestTrailers(uint32_t trailers) override;
   FilterMetadataStatus onRequestMetadata(uint32_t elements) override;
   FilterHeadersStatus onResponseHeaders(uint32_t headers, bool end_of_stream) override;
-  FilterDataStatus onResponseBody(uint32_t body_buffer_length, bool end_of_stream) override;
+  FilterDataStatus onResponseBody(uint32_t body_length, bool end_of_stream) override;
   FilterTrailersStatus onResponseTrailers(uint32_t trailers) override;
   FilterMetadataStatus onResponseMetadata(uint32_t elements) override;
 
@@ -341,7 +341,7 @@ class ContextBase : public RootInterface,
   WasmResult registerSharedQueue(std::string_view queue_name,
                                  SharedQueueDequeueToken *token_ptr) override;
   WasmResult lookupSharedQueue(std::string_view vm_id, std::string_view queue_name,
-                               SharedQueueEnqueueToken *token) override;
+                               SharedQueueEnqueueToken *token_ptr) override;
   WasmResult dequeueSharedQueue(uint32_t token, std::string *data) override;
   WasmResult enqueueSharedQueue(uint32_t token, std::string_view value) override;
 

Diff for: include/proxy-wasm/context_interface.h

@@ -206,7 +206,7 @@ struct HttpInterface {
   virtual FilterHeadersStatus onRequestHeaders(uint32_t headers, bool end_of_stream) = 0;
 
   // Call on a stream context to indicate that body data has arrived.
-  virtual FilterDataStatus onRequestBody(uint32_t body_buffer_length, bool end_of_stream) = 0;
+  virtual FilterDataStatus onRequestBody(uint32_t body_length, bool end_of_stream) = 0;
 
   // Call on a stream context to indicate that the request trailers have arrived.
   virtual FilterTrailersStatus onRequestTrailers(uint32_t trailers) = 0;
@@ -218,7 +218,7 @@ struct HttpInterface {
   virtual FilterHeadersStatus onResponseHeaders(uint32_t trailers, bool end_of_stream) = 0;
 
   // Call on a stream context to indicate that body data has arrived.
-  virtual FilterDataStatus onResponseBody(uint32_t body_buffer_length, bool end_of_stream) = 0;
+  virtual FilterDataStatus onResponseBody(uint32_t body_length, bool end_of_stream) = 0;
 
   // Call on a stream context to indicate that the request trailers have arrived.
   virtual FilterTrailersStatus onResponseTrailers(uint32_t trailers) = 0;

Diff for: include/proxy-wasm/exports.h

@@ -57,7 +57,7 @@ struct RegisterForeignFunction {
    * @param function_name is the key for this foreign function.
    * @param f is the function instance.
    */
-  RegisterForeignFunction(std::string function_name, WasmForeignFunction f);
+  RegisterForeignFunction(const std::string &function_name, WasmForeignFunction f);
 };
 
 namespace exports {
@@ -94,8 +94,8 @@ template <typename Pairs> void marshalPairs(const Pairs &result, char *buffer) {
 
 // ABI functions exported from host to wasm.
 
-Word get_configuration(Word address, Word size);
-Word get_status(Word status_code, Word address, Word size);
+Word get_configuration(Word value_ptr_ptr, Word value_size_ptr);
+Word get_status(Word code_ptr, Word value_ptr_ptr, Word value_size_ptr);
 Word log(Word level, Word address, Word size);
 Word get_log_level(Word result_level_uint32_ptr);
 Word get_property(Word path_ptr, Word path_size, Word value_ptr_ptr, Word value_size_ptr);
@@ -134,7 +134,7 @@ Word get_response_body_buffer_bytes(Word start, Word length, Word ptr_ptr, Word
 Word http_call(Word uri_ptr, Word uri_size, Word header_pairs_ptr, Word header_pairs_size,
                Word body_ptr, Word body_size, Word trailer_pairs_ptr, Word trailer_pairs_size,
                Word timeout_milliseconds, Word token_ptr);
-Word define_metric(Word metric_type, Word name_ptr, Word name_size, Word result_ptr);
+Word define_metric(Word metric_type, Word name_ptr, Word name_size, Word metric_id_ptr);
 Word increment_metric(Word metric_id, int64_t offset);
 Word record_metric(Word metric_id, uint64_t value);
 Word get_metric(Word metric_id, Word result_uint64_ptr);

Diff for: include/proxy-wasm/null_plugin.h

@@ -71,7 +71,7 @@ class NullPlugin : public NullVmPlugin {
   void onForeignFunction(uint64_t root_context_id, uint64_t foreign_function_id,
                          uint64_t data_size);
 
-  void onCreate(uint64_t context_id, uint64_t root_context_id);
+  void onCreate(uint64_t context_id, uint64_t parent_context_id);
 
   uint64_t onNewConnection(uint64_t context_id);
   uint64_t onDownstreamData(uint64_t context_id, uint64_t data_length, uint64_t end_of_stream);

Diff for: include/proxy-wasm/null_vm.h

@@ -35,8 +35,8 @@ struct NullVm : public WasmVm {
   std::string_view getEngineName() override { return "null"; }
   Cloneable cloneable() override { return Cloneable::InstantiatedModule; };
   std::unique_ptr<WasmVm> clone() override;
-  bool load(std::string_view bytecode, std::string_view precompiled,
-            std::unordered_map<uint32_t, std::string> function_names) override;
+  bool load(std::string_view plugin_name, std::string_view precompiled,
+            const std::unordered_map<uint32_t, std::string> &function_names) override;
   bool link(std::string_view debug_name) override;
   uint64_t getMemorySize() override;
   std::optional<std::string_view> getMemory(uint64_t pointer, uint64_t size) override;

Diff for: include/proxy-wasm/vm_id_handle.h

@@ -31,6 +31,6 @@ class VmIdHandle {
 };
 
 std::shared_ptr<VmIdHandle> getVmIdHandle(std::string_view vm_id);
-void registerVmIdHandleCallback(std::function<void(std::string_view vm_id)> f);
+void registerVmIdHandleCallback(const std::function<void(std::string_view vm_id)> &f);
 
 } // namespace proxy_wasm

Diff for: include/proxy-wasm/wasm.h

@@ -52,15 +52,15 @@ class WasmBase : public std::enable_shared_from_this<WasmBase> {
            std::string_view vm_configuration, std::string_view vm_key,
            std::unordered_map<std::string, std::string> envs,
            AllowedCapabilitiesMap allowed_capabilities);
-  WasmBase(const std::shared_ptr<WasmHandleBase> &other, WasmVmFactory factory);
+  WasmBase(const std::shared_ptr<WasmHandleBase> &base_wasm_handle, const WasmVmFactory &factory);
   virtual ~WasmBase();
 
   bool load(const std::string &code, bool allow_precompiled = false);
   bool initialize();
   void startVm(ContextBase *root_context);
   bool configure(ContextBase *root_context, std::shared_ptr<PluginBase> plugin);
   // Returns the root ContextBase or nullptr if onStart returns false.
-  ContextBase *start(std::shared_ptr<PluginBase> plugin);
+  ContextBase *start(const std::shared_ptr<PluginBase> &plugin);
 
   std::string_view vm_id() const { return vm_id_; }
   std::string_view vm_key() const { return vm_key_; }
@@ -340,11 +340,13 @@ using WasmHandleCloneFactory =
     std::function<std::shared_ptr<WasmHandleBase>(std::shared_ptr<WasmHandleBase> wasm)>;
 
 // Returns nullptr on failure (i.e. initialization of the VM fails).
-std::shared_ptr<WasmHandleBase>
-createWasm(std::string vm_key, std::string code, std::shared_ptr<PluginBase> plugin,
-           WasmHandleFactory factory, WasmHandleCloneFactory clone_factory, bool allow_precompiled);
-// Get an existing ThreadLocal VM matching 'vm_id' or nullptr if there isn't one.
-std::shared_ptr<WasmHandleBase> getThreadLocalWasm(std::string_view vm_id);
+std::shared_ptr<WasmHandleBase> createWasm(const std::string &vm_key, const std::string &code,
+                                           const std::shared_ptr<PluginBase> &plugin,
+                                           const WasmHandleFactory &factory,
+                                           const WasmHandleCloneFactory &clone_factory,
+                                           bool allow_precompiled);
+// Get an existing ThreadLocal VM matching 'vm_key' or nullptr if there isn't one.
+std::shared_ptr<WasmHandleBase> getThreadLocalWasm(std::string_view vm_key);
 
 class PluginHandleBase : public std::enable_shared_from_this<PluginHandleBase> {
 public:
@@ -371,8 +373,8 @@ using PluginHandleFactory = std::function<std::shared_ptr<PluginHandleBase>(
 // Get an existing ThreadLocal VM matching 'vm_id' or create one using 'base_wavm' by cloning or by
 // using it it as a template.
 std::shared_ptr<PluginHandleBase> getOrCreateThreadLocalPlugin(
-    std::shared_ptr<WasmHandleBase> base_wasm, std::shared_ptr<PluginBase> plugin,
-    WasmHandleCloneFactory clone_factory, PluginHandleFactory plugin_factory);
+    const std::shared_ptr<WasmHandleBase> &base_handle, const std::shared_ptr<PluginBase> &plugin,
+    const WasmHandleCloneFactory &clone_factory, const PluginHandleFactory &plugin_factory);
 
 // Clear Base Wasm cache and the thread-local Wasm sandbox cache for the calling thread.
 void clearWasmCachesForTesting();
@@ -403,7 +405,7 @@ inline uint64_t WasmBase::copyString(std::string_view s) {
   if (s.empty()) {
     return 0; // nullptr
   }
-  uint64_t pointer;
+  uint64_t pointer = 0;
   uint8_t *m = static_cast<uint8_t *>(allocMemory((s.size() + 1), &pointer));
   memcpy(m, s.data(), s.size());
   m[s.size()] = 0;

Diff for: include/proxy-wasm/wasm_vm.h

@@ -214,7 +214,7 @@ class WasmVm {
    * @return whether or not the load was successful.
    */
   virtual bool load(std::string_view bytecode, std::string_view precompiled,
-                    const std::unordered_map<uint32_t, std::string> function_names) = 0;
+                    const std::unordered_map<uint32_t, std::string> &function_names) = 0;
 
   /**
    * Link the WASM code to the host-provided functions, e.g. the ABI. Prior to linking, the module