On Thu, Sep 17, 2026 at 10:39:13AM +1000, Kugan Vivekanandarajah wrote:
> + size_t processed_samples = 0;
> +
> + /* MMAPs can appear after samples in the ring buffer; bind mappings first.
> */
> + if (!for_each_mapped_perf_data_record (
> + [&] (const perf_event_header &record_header,
> + const uint8_t *payload_ptr,
> + size_t payload_size,
> + size_t)
> + {
> + switch (record_header.type)
> + {
> + case perf_record_type::MMAP:
> + parse_mmap_record (payload_ptr, payload_size,
> + record_header.misc);
> + break;
> + case perf_record_type::MMAP2:
> + parse_mmap2_record (payload_ptr, payload_size,
> + record_header.misc);
> + break;
So the problem with the two step walk is that you don't handle the case
when a library gets unloaded and something else loaded in the same
place.
I realize this might be hard to handle with your other data structures
that assume an unique PC mapping, but it would be good to at least detect
and warn about it.
> + case perf_record_type::COMM:
> + note_process_lifetime_record (
> + record_header, payload_ptr, payload_size);
> + break;
> + default:
> + break;
It would probably be useful to handle FINISHED_ROUND at least,
I think those can happen with aio writes etc.
> +bool
> +perf_branch_stack_reader::parse_sample_record (const uint8_t *record_data,
> + size_t record_size)
> +{
> + if (use_fixed_layout_branch_stack_parse_)
> + return parse_sample_record_fixed_branch_stack_layout (
> + record_data, record_size);
> +
> + const uint8_t *ptr = record_data;
> + const uint8_t *const end = record_data + record_size;
> +
> + uint64_t ip = 0;
> + bool have_ip = false;
> + uint32_t pid = 0;
> + uint64_t sample_period = 0;
> + uint64_t sample_time = 0;
> +
> + if (!perf_event_sample_util::walk_sample_before_branch (
> + ptr, end, sample_layout.sample_type, sample_layout.read_format, &ip,
> + &have_ip, &pid, nullptr, &sample_period, &sample_time))
> + {
> + fprintf (stderr,
> + _("PERF: WARNING: Truncated or invalid sample (prefix)\n"));
> + return false;
> + }
Probably need some kind of rate limit on the warning here, also for the
others. Otherwise it could be a very long flood of messages for a bad
file.
> + if (sample_layout.fixed_sample_period
> + && sample_period != sample_layout.sample_period)
> + {
> + fprintf (stderr,
> + _("PERF: WARNING: sample period does not match the fixed "
> + "event period\n"));
> + return false;
> + }
Perf can actually reduce the period any time when it is throttling
because of PMI overload. Better just handle it silently.
> + ptr, end, sample_layout.sample_type, sample_layout.sample_regs_user,
> + sample_layout.sample_regs_intr)
> + || ptr != end)
> + {
> + fprintf (stderr,
> + _("PERF: WARNING: Truncated or invalid sample suffix\n"));
> + return false;
> + }
> +
> + const std::vector<simple_mmap> *const pid_mmaps = target_mmaps_for_pid
> (pid);
> + if (pid_mmaps == nullptr)
> + return false;
> + uint64_t dwarf_ip = 0;
> + const bool ip_is_target
> + = have_ip && ip != 0
> + && try_translate_runtime_with_mmaps (*pid_mmaps, ip, &dwarf_ip);
> + bool recorded_sample = record_sample_ip (
> + dwarf_ip, ip_is_target, sample_time, accumulate_sample_ips_,
> + address_frequencies);
> + if (use_branch_stack_for_correlation_
> + && (sample_layout.sample_type & perf_sample_type::BRANCH_STACK) != 0
> + && nr > 0
> + && accumulate_branch_stack_sample (
> + entries,
> + nr,
> + *this,
> + sample_time,
I forgot this earlier, but it would be good if you supported not having
a time stamp to shrink the files.
> + const uint8_t *record_data, size_t record_size)
> +{
> + if (record_size < BRANCH_STACK_FIXED_PREFIX_BYTES + sizeof (uint64_t))
> + return false;
What's the extra 8 bytes?
-Andi
