Fix stack-sending logic in Go custom labels path (#180)

unwind_stop is supposed to be the centralized place to send stacks. It
also does some logic like recording error frames.

When the Go custom labels path was moved to a separate tail call, the
stack-sending call was moved there, instead of going through the main
path in unwind_stop. So for Go programs specifically, we are not
adding error frames (and in some cases just sending empty stacks).

Author: umanwizard

support/ebpf/go_labels.ebpf.c

@@ -173,16 +173,20 @@ static EBPF_INLINE int go_labels(struct pt_regs *ctx)
   u32 pid                  = record->trace.pid;
   GoLabelsOffsets *offsets = bpf_map_lookup_elem(&go_labels_procs, &pid);
   if (!offsets) {
-    DEBUG_PRINT("cl: no offsets, %d not recognized as a go binary", pid);
-    return -1;
-  }
-  DEBUG_PRINT(
-    "cl: go offsets found, %d recognized as a go binary: m_ptr: %lx",
-    pid,
-    (unsigned long)record->customLabelsState.go_m_ptr);
-  bool success = get_go_custom_labels(record, offsets);
-  if (!success) {
-    increment_metric(metricID_UnwindGoLabelsFailures);
+    DEBUG_PRINT("cl: no offsets, %d not recognized as a go binary. This should never happen!", pid);
+    // We shouldn't tail call
+    // into this program if there is no map element.
+    // Nevertheless, if that happens due to some bug,
+    // fall through rather than swallowing the trace.
+  } else {
+    DEBUG_PRINT(
+      "cl: go offsets found, %d recognized as a go binary: m_ptr: %lx",
+      pid,
+      (unsigned long)record->customLabelsState.go_m_ptr);
+    bool success = get_go_custom_labels(record, offsets);
+    if (!success) {
+      increment_metric(metricID_UnwindGoLabelsFailures);
+    }
   }
 
   send_trace(ctx, &record->trace);

support/ebpf/interpreter_dispatcher.ebpf.c

@@ -215,28 +215,24 @@ static EBPF_INLINE void *get_m_ptr(struct GoLabelsOffsets *offs, UNUSED UnwindSt
 
 static EBPF_INLINE void maybe_add_go_custom_labels_legacy(struct pt_regs *ctx, PerCPURecord *record)
 {
-  u32 pid = record->trace.pid;
+  u32 pid                        = record->trace.pid;
   // The Go label extraction code is too big to fit in this program, so we need to
-  // tail call it, in order to keep the hashing and clearing code in this program it
-  // will tail call back to us with this bool set.
-  if (!record->state.processed_go_labels) {
-    GoCustomLabelsOffsets *offsets = bpf_map_lookup_elem(&go_labels_procs, &pid);
-    if (!offsets) {
-      DEBUG_PRINT("cl: no offsets, %d not recognized as a go binary", pid);
-      return;
-    }
-
-    void *m_ptr_addr = get_m_ptr_legacy(offsets, &record->state);
-    if (!m_ptr_addr) {
-      return;
-    }
-    record->customLabelsState.go_m_ptr = m_ptr_addr;
+  // tail call it
+  GoCustomLabelsOffsets *offsets = bpf_map_lookup_elem(&go_labels_procs, &pid);
+  if (!offsets) {
+    DEBUG_PRINT("cl: no offsets, %d not recognized as a go binary", pid);
+    return;
+  }
 
-    DEBUG_PRINT("cl: trace is within a process with Go custom labels enabled");
-    increment_metric(metricID_UnwindGoCustomLabelsAttempts);
-    record->state.processed_go_labels = true;
-    tail_call(ctx, PROG_GO_LABELS);
+  void *m_ptr_addr = get_m_ptr_legacy(offsets, &record->state);
+  if (!m_ptr_addr) {
+    return;
   }
+  record->customLabelsState.go_m_ptr = m_ptr_addr;
+
+  DEBUG_PRINT("cl: trace is within a process with Go custom labels enabled");
+  increment_metric(metricID_UnwindGoCustomLabelsAttempts);
+  tail_call(ctx, PROG_GO_LABELS);
 }
 
 static EBPF_INLINE void maybe_add_go_custom_labels(struct pt_regs *ctx, PerCPURecord *record)
@@ -770,12 +766,8 @@ static EBPF_INLINE int unwind_stop(struct pt_regs *ctx)
   Trace *trace       = &record->trace;
   UnwindState *state = &record->state;
 
-  // Do Go first since we might tail call out and back again.
-  // Try legacy Go custom labels first, then new Go labels implementation
   // TODO: maybe instead of adding a per-language call here, we
   // should have "path to CLs" be a standard part of some per-pid map?
-  maybe_add_go_custom_labels_legacy(ctx, record);
-  maybe_add_go_custom_labels(ctx, record);
   maybe_add_native_custom_labels(record);
   if (!maybe_add_node_custom_labels(record))
     increment_metric(metricID_UnwindNodeCustomLabelsFailures);
@@ -833,6 +825,11 @@ static EBPF_INLINE int unwind_stop(struct pt_regs *ctx)
   }
   // TEMPORARY HACK END
 
+  // Try legacy Go custom labels first, then new Go labels implementation
+  // Must be last since it may not return (it will call send_trace).
+  maybe_add_go_custom_labels_legacy(ctx, record);
+  maybe_add_go_custom_labels(ctx, record);
+
   send_trace(ctx, trace);
 
   return 0;

support/ebpf/tracemgmt.h

@@ -230,7 +230,6 @@ static inline EBPF_INLINE PerCPURecord *get_pristine_per_cpu_record()
   record->state.return_address             = false;
   record->state.error_metric               = -1;
   record->state.unwind_error               = ERR_OK;
-  record->state.processed_go_labels        = false;
   record->perlUnwindState.stackinfo        = 0;
   record->perlUnwindState.cop              = 0;
   record->pythonUnwindState.py_frame       = 0;

support/ebpf/tracer.ebpf.amd64

@@ -703,9 +703,6 @@ typedef struct UnwindState {
   // Consider calling unwinder_mark_nonleaf_frame rather than setting this directly.
   bool return_address;
 
-  // Make sure we only do this once.
-  bool processed_go_labels;
-
 #if defined(__aarch64__)
   // On aarch64, whether to forbid LR-based unwinding.
   // LR unwinding is only allowed for leaf user-mode frames. Frames making a syscall