Use clang-tidy on ebpf code (#1180)

grafana · Sep 20, 2024 · 2d7c95c · 2d7c95c
1 parent cbca5fe
commit 2d7c95c
Show file tree

Hide file tree

Showing 47 changed files with 298 additions and 177 deletions.
diff --git a/.github/workflows/clang-tidy-check.yml b/.github/workflows/clang-tidy-check.yml
@@ -0,0 +1,36 @@
+name: Clang Tidy Check
+
+on:
+ push:
+ branches: [ 'main', 'release-*' ]
+ pull_request:
+ branches: [ 'main', 'release-*' ]
+
+jobs:
+ clang-tidy:
+ runs-on: ubuntu-latest
+
+ steps:
+ - name: Checkout code
+ uses: actions/checkout@v3
+ with:
+ lfs: true
+
+ - name: Install clang-tidy
+ run: |
+ sudo apt-get update
+ sudo apt-get install -y wget lsb-release software-properties-common gnupg
+ wget -qO- https://apt.llvm.org/llvm-snapshot.gpg.key | sudo tee /etc/apt/trusted.gpg.d/apt.llvm.org.asc
+ sudo add-apt-repository -y 'deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy main'
+ sudo apt-get install -y clang-tidy-20
+
+ - name: Check Clang Tidy
+ run: |
+ cd bpf
+ clang-tidy-20 *.c *.h | tee clang-tidy-output.txt
+
+ if [ $? -ne 0 ] || grep -q "warning:" clang-tidy-output.txt; then
+ echo "clang-tidy found warnings or errors."
+ exit 1
+ fi
+
diff --git a/bpf/.clang-tidy b/bpf/.clang-tidy
@@ -0,0 +1,25 @@
+Checks:
+ - '-*'
+ - 'readability-*'
+ - 'clang-diagnostic-error'
+ - 'misc-misplaced_const'
+ - 'misc-*'
+ - '-bugprone-*'
+ - 'bugprone-switch-missing-default-case'
+ - '-bugprone-multi-level-implicit-pointer-conversion' #TODO enable this
+ - '-bugprone-reserved-identifier'
+ - '-bugprone-easily-swappable-parameters'
+ - '-bugprone-narrowing-conversions' #TODO this needs to be checked/enabled
+ - '-misc-unused-parameters'
+ - '-misc-misplaced-const'
+ - '-misc-include-cleaner'
+ - '-readability-else-after-return'
+ - '-readability-identifier-length'
+ - '-readability-function-cognitive-complexity'
+ - '-readability-magic-numbers'
+
+WarningsAsErrors: ''
+HeaderFilterRegex: ''
+FormatStyle: none
+ExtraArgs: ['--target=bpf', '-Iheaders', '-D__TARGET_ARCH_x86', '-DBPF_DEBUG', '-DBPF_TRACEPARENT']
+
diff --git a/bpf/bpf_dbg.h b/bpf/bpf_dbg.h
@@ -12,6 +12,9 @@
 #ifndef BPF_DBG_H
 #define BPF_DBG_H
 
+#include "vmlinux.h"
+#include "bpf_core_read.h"
+
 #ifdef BPF_DEBUG
 
 typedef struct log_info {

diff --git a/bpf/debug_log.c b/bpf/debug_log.c
@@ -2,4 +2,6 @@
 #include "bpf_helpers.h"
 #include "bpf_dbg.h"
 
+#ifdef BPF_DEBUG
 const log_info_t *unused_100 __attribute__((unused));
+#endif
diff --git a/bpf/flow.h b/bpf/flow.h
@@ -19,6 +19,8 @@
 #ifndef __FLOW_H__
 #define __FLOW_H__
 
+#include "vmlinux.h"
+
 #define TC_ACT_OK 0
 #define TC_ACT_SHOT 2
 #define IP_MAX_LEN 16

diff --git a/bpf/flows_common.h b/bpf/flows_common.h
@@ -186,9 +186,11 @@ static inline u8 get_connection_initiator(flow_id *id, u16 flags) {
  flow_initiator = INITIATOR_SRC;
  }
  break;
+ default:
+ break;
  }
 
  return flow_initiator;
 }
 
-#endif //__FLOW_HELPERS_H__
+#endif //__FLOW_HELPERS_H__
diff --git a/bpf/flows_sock.c b/bpf/flows_sock.c
@@ -150,7 +150,10 @@ static __always_inline bool read_sk_buff(struct __sk_buff *skb, flow_id *id, u16
  id->src_port = __bpf_htons(port);
  bpf_skb_load_bytes(skb, hdr_len + offsetof(struct __udphdr, dest), &port, sizeof(port));
  id->dst_port = __bpf_htons(port);
+ break;
  }
+ default:
+ return false;
  }
 
  // custom flags
@@ -165,7 +168,7 @@ static __always_inline bool read_sk_buff(struct __sk_buff *skb, flow_id *id, u16
  return true;
 }
 
-static __always_inline bool same_ip(u8 *ip1, u8 *ip2) {
+static __always_inline bool same_ip(const u8 *ip1, const u8 *ip2) {
  for (int i = 0; i < 16; i += 4) {
  if (*((u32 *)(ip1 + i)) != *((u32 *)(ip2 + i))) {
  return false;
@@ -301,4 +304,4 @@ const flow_metrics *unused_flow_metrics __attribute__((unused));
 const flow_id *unused_flow_id __attribute__((unused));
 const flow_record *unused_flow_record __attribute__((unused));
 
-char _license[] SEC("license") = "GPL";
+char _license[] SEC("license") = "GPL";
diff --git a/bpf/go_common.h b/bpf/go_common.h
@@ -308,7 +308,7 @@ static __always_inline u8 get_conn_info(void *conn_ptr, connection_info_t *info)
 static __always_inline void *unwrap_tls_conn_info(void *conn_ptr, void *tls_state) {
  if (conn_ptr && tls_state) {
  void *c_ptr = 0;
- bpf_probe_read(&c_ptr, sizeof(c_ptr), (void *)(conn_ptr)); // unwrap conn
+ bpf_probe_read(&c_ptr, sizeof(c_ptr), conn_ptr); // unwrap conn
 
  bpf_dbg_printk("unwrapped conn ptr %llx", c_ptr);
 

diff --git a/bpf/go_grpc.h b/bpf/go_grpc.h
@@ -359,7 +359,7 @@ static __always_inline void clientConnStart(
  go_offset_of(ot, (go_offset){.v = _value_context_val_ptr_pos}) +
  sizeof(void *)));
 
- invocation.flags = client_trace_parent(goroutine_addr, &invocation.tp, (void *)(val_ptr));
+ invocation.flags = client_trace_parent(goroutine_addr, &invocation.tp, val_ptr);
  } else {
  // it's OK sending empty tp for a client, the userspace id generator will make random trace_id, span_id
  bpf_dbg_printk("No ctx_ptr %llx", ctx_ptr);

diff --git a/bpf/go_sql.h b/bpf/go_sql.h
@@ -16,6 +16,16 @@
 // This implementation was inspired by https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/ca1afccea6ec520d18238c3865024a9f5b9c17fe/internal/pkg/instrumentors/bpf/database/sql/bpf/probe.bpf.c
 // and has been modified since.
 
+#ifndef GO_SQL_H
+#define GO_SQL_H
+
+#include "vmlinux.h"
+
+#include "bpf_helpers.h"
+#include "http_types.h"
+#include "go_common.h"
+#include "ringbuf.h"
+
 typedef struct sql_func_invocation {
  u64 start_monotime_ns;
  u64 sql_param;
@@ -109,4 +119,6 @@ int uprobe_queryReturn(struct pt_regs *ctx) {
  bpf_dbg_printk("can't reserve space in the ringbuffer");
  }
  return 0;
-}
+}
+
+#endif
diff --git a/bpf/go_traceparent.h b/bpf/go_traceparent.h
@@ -66,7 +66,7 @@ struct {
 } golang_mapbucket_storage_map SEC(".maps");
 
 // assumes s2 is all lowercase
-static __always_inline int bpf_memicmp(char *s1, char *s2, s32 size) {
+static __always_inline int bpf_memicmp(const char *s1, const char *s2, s32 size) {
  for (int i = 0; i < size; i++) {
  if (s1[i] != s2[i] && s1[i] != (s2[i] - 32)) // compare with each uppercase character
  {