ftrace: fix seq-clear flags when symbolizing kernel addresses

The logic that emits SEQ_INCREMENTAL_STATE_CLEARED had a
logical bug. That logic used to look at (max_index_at_start == 0)
to determine if we need to reset the interning state by emitting
a SEQ_INCREMENTAL_STATE_CLEARED flag.
This is the bug:
- We are at the start of a new batch and we are processing events
  for CPU0. Before this call, the ftrace controller did a
  FtraceMetadata::Clear(), so last_kernel_addr_index_written = 0
  and the interning map is cleared.
- There are N events with addresses (e.g. workqueue events) in
  the CPU0 buffer. We try to symbolize them and all of them fail
  the Lookup(). This event (the sym failure) is not too unlikely.
  Many ftrace events contain (useless) pointers to data structures.
- At the end of the loop, we set last_kernel_addr_index_written = N.
  However we have not written any symbol, so we never even started
  the set_interned_data() section.
- On the next round (e.g. on CPU1) we find some other symbol which
  passes the lookup. We create the interned_data section but we
  do NOT set SEQ_INCREMENTAL_STATE_CLEARED because at this point
  max_index_at_start = N, not 0.
- By doing so we fail to signal the restart of the intern numbering
  to trace processor, even though we actuall restart it (by virtue
  of having called FtraceMetadata::Clear() before CPU0.

Bug: 190075739
(cherry picked from commit c1ba22d9d500a3456c1f25854275fb378fc129d7)
Change-Id: Ic467018c6752c492a48065839f17b589c83f7d53
Merged-In: Ic467018c6752c492a48065839f17b589c83f7d53

1 files changed, 11 insertions, 1 deletions

diff --git a/src/traced/probes/ftrace/cpu_reader.cc b/src/traced/probes/ftrace/cpu_reader.cc
index facd4ceee..a6a6f6eae 100644
--- a/src/traced/probes/ftrace/cpu_reader.cc
+++ b/src/traced/probes/ftrace/cpu_reader.cc
@@ -323,6 +323,7 @@ bool CpuReader::ProcessPagesForDataSource(
       PERFETTO_DCHECK(max_index_at_start <= metadata->kernel_addrs.size());
       protos::pbzero::InternedData* interned_data = nullptr;
       auto* ksyms_map = symbolizer->GetOrCreateKernelSymbolMap();
+      bool wrote_at_least_one_symbol = false;
       for (const FtraceMetadata::KernelAddr& kaddr : metadata->kernel_addrs) {
         if (kaddr.index <= max_index_at_start)
           continue;
@@ -351,9 +352,18 @@ bool CpuReader::ProcessPagesForDataSource(
         auto* interned_sym = interned_data->add_kernel_symbols();
         interned_sym->set_iid(kaddr.index);
         interned_sym->set_str(sym_name);
+        wrote_at_least_one_symbol = true;
       }
+
       auto max_it_at_end = static_cast<uint32_t>(metadata->kernel_addrs.size());
-      metadata->last_kernel_addr_index_written = max_it_at_end;
+
+      // Rationale for the if (wrote_at_least_one_symbol) check: in rare cases,
+      // all symbols seen in a ProcessPagesForDataSource() call can fail the
+      // ksyms_map->Lookup(). If that happens we don't want to bump the
+      // last_kernel_addr_index_written watermark, as that would cause the next
+      // call to NOT emit the SEQ_INCREMENTAL_STATE_CLEARED.
+      if (wrote_at_least_one_symbol)
+        metadata->last_kernel_addr_index_written = max_it_at_end;
     }
 
     packet->Finalize();