From cba3b97531af194fd3c44ed3d4a2cb34ecfabc43 Mon Sep 17 00:00:00 2001 From: Lalit Maganti Date: Fri, 9 Oct 2026 14:02:32 -0700 Subject: [PATCH 1/2] tp: fix manifest offset_ns ignored for snapshot-less proto files A perfetto_manifest clock override adds an edge from the file's source clock (the named `clock`, or the file's private TraceFile clock when pinned) to the reference clock. #6399 dropped setting that clock as the file's default, so proto files without ClockSnapshots or timestamp_clock_id never went through the edge and offset_ns was silently ignored. Restore it, for both the pinned and named-clock forms. Also reject offset_ns placed inside sync_to instead of ignoring it. --- .../forwarding_trace_parser.cc | 22 ++- .../perfetto_manifest_reader.cc | 5 + .../diff_tests/parser/trace_manifest/tests.py | 126 ++++++++++++++++++ 3 files changed, 149 insertions(+), 4 deletions(-) diff --git a/src/trace_processor/forwarding_trace_parser.cc b/src/trace_processor/forwarding_trace_parser.cc index 6e33852b51b..2ba65aa2ddd 100644 --- a/src/trace_processor/forwarding_trace_parser.cc +++ b/src/trace_processor/forwarding_trace_parser.cc @@ -232,9 +232,23 @@ base::Status ForwardingTraceParser::Init(const TraceBlobView& blob) { // clock of its own, so flag it: the file is now single-clock / single-machine // and any ClockSnapshot or remote machine id on it is rejected. It still // converts through the default clock set up below, like any clockless format. - if (manifest_entry && manifest_entry->clock_override && - !manifest_entry->clock_override->source_clock) { - trace_context_->trace_state->has_clock_override = true; + // + // The override's edge is attached to a specific source clock (the manifest's + // `clock`, or for a pinned file its private TraceFile clock), so that clock + // must also be the file's default: otherwise events which don't name a clock + // (e.g. proto packets without timestamp_clock_id or ClockSnapshots) never + // reach the edge and the offset is silently ignored. Proto may still replace + // this later via primary_trace_clock. + bool force_default_clock = false; + if (manifest_entry && manifest_entry->clock_override) { + const auto& co = *manifest_entry->clock_override; + if (co.source_clock) { + trace_clock = ClockId::Machine(*co.source_clock); + } else { + trace_context_->trace_state->has_clock_override = true; + trace_clock = ClockId::TraceFile(trace_context_->trace_id().value); + } + force_default_clock = true; } // Set up the format's source clock. Proto manages its own default clock @@ -242,7 +256,7 @@ base::Status ForwardingTraceParser::Init(const TraceBlobView& blob) { // (sets_default_clock=false); every other format converts its events through // the default clock via ClockTracker::ConvertDefaultClockToTraceTime. if (trace_clock) { - if (desc->sets_default_clock) { + if (desc->sets_default_clock || force_default_clock) { clock_tracker->SetTraceDefaultClock(*trace_clock); } if (desc->claims_global_clock) { diff --git a/src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc b/src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc index 1850b47d88f..58562897776 100644 --- a/src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc +++ b/src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc @@ -136,6 +136,11 @@ base::StatusOr ParseClocks(const json::Dom& clocks) { result.ref_clock = ref_clock; } + if (sync_to.HasMember("offset_ns")) { + return base::ErrStatus( + "perfetto_manifest: clocks: offset_ns belongs in the clocks block, " + "not inside sync_to."); + } if (clocks.HasMember("offset_ns")) { if (!clocks["offset_ns"].IsIntegral()) { return base::ErrStatus( diff --git a/test/trace_processor/diff_tests/parser/trace_manifest/tests.py b/test/trace_processor/diff_tests/parser/trace_manifest/tests.py index 4da4dd2e5d5..b3f697c890e 100644 --- a/test/trace_processor/diff_tests/parser/trace_manifest/tests.py +++ b/test/trace_processor/diff_tests/parser/trace_manifest/tests.py @@ -187,6 +187,18 @@ def _proto_boot_snap(name, seq, uuid, boot, at): (boot, seq, uuid, seq, at, uuid, name, seq, at + 100000000, uuid)) +# Like _proto_boot_snap but with no ClockSnapshot and no timestamp_clock_id: +# the packets name no clock at all (e.g. traces written by external tools). +def _proto_no_snap(name, seq, uuid, at): + return TextProto( + 'packet { trusted_packet_sequence_id: %d track_descriptor { uuid: %d } }\n' + 'packet { trusted_packet_sequence_id: %d timestamp: %d\n' + ' track_event { type: TYPE_SLICE_BEGIN track_uuid: %d name: "%s" } }\n' + 'packet { trusted_packet_sequence_id: %d timestamp: %d\n' + ' track_event { type: TYPE_SLICE_END track_uuid: %d } }\n' % + (seq, uuid, seq, at, uuid, name, seq, at + 100000000, uuid)) + + # A perfetto_manifest entry attributing |path| to machine |name|. def _machine_file(path, name): return {'path': path, 'machine': {'name': name}} @@ -1215,6 +1227,120 @@ def test_sync_to_machine_offset(self): "server_slice",1100000500,"server" ''')) + # A pinned (no `clock`) proto file with no ClockSnapshot and no + # timestamp_clock_id must still have offset_ns applied: its events go through + # the file's private clock, which the manifest edge relates to the reference. + def test_sync_to_offset_pinned_proto_no_snapshot(self): + return DiffTestBlueprint( + trace=Zip({ + 'meta.json': + _meta({ + 'version': + 1, + 'files': [ + _machine_file('phone.pb', 'phone'), + { + 'path': 'server.pb', + 'machine': { + 'name': 'server' + }, + 'clocks': { + 'offset_ns': 500, + 'sync_to': { + 'file': 'phone.pb', + 'clock': 'BOOTTIME' + } + } + }, + ], + }), + 'phone.pb': + _proto_boot_snap('phone_slice', 1, 111, 1000000000, 1100000000), + 'server.pb': + _proto_no_snap('server_slice', 2, 222, 1100000000), + }), + query=_ALIGN_QUERY, + out=Csv(''' + "name","ts","machine" + "phone_slice",1100000000,"phone" + "server_slice",1100000500,"server" + ''')) + + # Same as above but naming the source clock: snapshot-less packets are in + # that clock, so the offset still applies. + def test_sync_to_offset_clock_proto_no_snapshot(self): + return DiffTestBlueprint( + trace=Zip({ + 'meta.json': + _meta({ + 'version': + 1, + 'files': [ + _machine_file('phone.pb', 'phone'), + { + 'path': 'server.pb', + 'machine': { + 'name': 'server' + }, + 'clocks': { + 'clock': 'BOOTTIME', + 'offset_ns': 500, + 'sync_to': { + 'file': 'phone.pb', + 'clock': 'BOOTTIME' + } + } + }, + ], + }), + 'phone.pb': + _proto_boot_snap('phone_slice', 1, 111, 1000000000, 1100000000), + 'server.pb': + _proto_no_snap('server_slice', 2, 222, 1100000000), + }), + query=_ALIGN_QUERY, + out=Csv(''' + "name","ts","machine" + "phone_slice",1100000000,"phone" + "server_slice",1100000500,"server" + ''')) + + # offset_ns belongs in `clocks`; placing it inside sync_to must not be + # silently ignored. + def test_error_offset_ns_inside_sync_to(self): + return DiffTestBlueprint( + trace=Zip({ + 'meta.json': + _meta({ + 'version': + 1, + 'files': [ + _machine_file('phone.pb', 'phone'), + { + 'path': 'server.pb', + 'machine': { + 'name': 'server' + }, + 'clocks': { + 'sync_to': { + 'file': 'phone.pb', + 'clock': 'BOOTTIME', + 'offset_ns': 500 + } + } + }, + ], + }), + 'phone.pb': + _proto_boot_snap('phone_slice', 1, 111, 1000000000, 1100000000), + 'server.pb': + _proto_no_snap('server_slice', 2, 222, 1100000000), + }), + query='SELECT 1;', + out=ExpectedError( + 'perfetto_manifest: clocks: offset_ns belongs in the clocks block, ' + 'not inside sync_to.')) + # sync_to.file naming an undeclared file is rejected. def test_error_sync_to_file_unknown(self): return DiffTestBlueprint( From d63733d1960195b692e438edece3de47e72f9255 Mon Sep 17 00:00:00 2001 From: Lalit Maganti Date: Fri, 9 Oct 2026 14:03:34 -0700 Subject: [PATCH 2/2] tp: allow TRACE_FILE as a clock name in manifest clocks blocks The file's private per-file timeline could only be selected by omitting `clock` / `sync_to.clock`. Accept "TRACE_FILE" as an explicit spelling of the same thing on both sides. Also accept it for trace_time.clock (with a required trace_time.file and no machine) to make that file's private timeline the trace time. --- docs/reference/perfetto-manifest.md | 6 +- .../perfetto_manifest_reader.cc | 62 ++++++---- .../diff_tests/parser/trace_manifest/tests.py | 109 ++++++++++++++++++ 3 files changed, 152 insertions(+), 25 deletions(-) diff --git a/docs/reference/perfetto-manifest.md b/docs/reference/perfetto-manifest.md index e4bfb66f1d0..dcffa3bf35b 100644 --- a/docs/reference/perfetto-manifest.md +++ b/docs/reference/perfetto-manifest.md @@ -269,11 +269,13 @@ Setting the same key twice overwrites: the last value wins. Wherever a clock name is expected, one of: `REALTIME`, `REALTIME_COARSE`, `MONOTONIC`, `MONOTONIC_COARSE`, -`MONOTONIC_RAW`, `BOOTTIME` +`MONOTONIC_RAW`, `BOOTTIME`, `TRACE_FILE` These correspond to the builtin clocks in [builtin_clock.proto](/protos/perfetto/common/builtin_clock.proto) and the -POSIX `clock_gettime` domains of the same names. +POSIX `clock_gettime` domains of the same names. `TRACE_FILE` is the file's +own per-file timeline (same as omitting `clock`); in `trace_time` it requires +`file`. ## {#sql} Effects on the SQL surface diff --git a/src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc b/src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc index 58562897776..724debe9d43 100644 --- a/src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc +++ b/src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc @@ -71,9 +71,13 @@ base::StatusOr ParseClockName(const json::Dom& value) { return BuiltinClock::BUILTIN_CLOCK_MONOTONIC_RAW; if (name == "BOOTTIME") return BuiltinClock::BUILTIN_CLOCK_BOOTTIME; + // The file's own private per-file timeline. + if (name == "TRACE_FILE") + return BuiltinClock::BUILTIN_CLOCK_TRACE_FILE; return base::ErrStatus( "perfetto_manifest: unknown clock name: %s. Use one of REALTIME, " - "REALTIME_COARSE, MONOTONIC, MONOTONIC_COARSE, MONOTONIC_RAW, BOOTTIME.", + "REALTIME_COARSE, MONOTONIC, MONOTONIC_COARSE, MONOTONIC_RAW, BOOTTIME, " + "TRACE_FILE.", name.c_str()); } @@ -102,7 +106,9 @@ base::StatusOr ParseClocks(const json::Dom& clocks) { } if (clocks.HasMember("clock")) { ASSIGN_OR_RETURN(uint32_t source_clock, ParseClockName(clocks["clock"])); - result.source_clock = source_clock; + // TRACE_FILE is the same as omitting the clock (pin the private clock). + if (source_clock != protos::pbzero::BUILTIN_CLOCK_TRACE_FILE) + result.source_clock = source_clock; } if (!clocks.HasMember("sync_to")) { @@ -133,7 +139,8 @@ base::StatusOr ParseClocks(const json::Dom& clocks) { } if (sync_to.HasMember("clock")) { ASSIGN_OR_RETURN(uint32_t ref_clock, ParseClockName(sync_to["clock"])); - result.ref_clock = ref_clock; + if (ref_clock != protos::pbzero::BUILTIN_CLOCK_TRACE_FILE) + result.ref_clock = ref_clock; } if (sync_to.HasMember("offset_ns")) { @@ -548,26 +555,6 @@ base::Status PerfettoManifestReader::ApplyManifest() { return *name_to_id.Find(*ref_machine); }; - // Claim the global trace time clock directly: the manifest is the first file, - // so its claim wins over later traces. trace_time.file (+ .machine) pins it - // to that file's (pre-allocated) machine. - if (state->trace_time_clock) { - ClockId trace_time = ClockId::Machine(*state->trace_time_clock); - if (state->trace_time_file || state->trace_time_machine) { - ASSIGN_OR_RETURN( - int64_t raw, - resolve_ref("trace_time: file", "trace_time: machine", - state->trace_time_file, state->trace_time_machine, - /*fallback=*/0)); - trace_time = ClockId::Machine(EnsureMachineRow(context_, raw), - *state->trace_time_clock); - } - context_->trace_time_state->TrySetClock(trace_time, file_id_); - context_->global_metadata_tracker->SetMetadata( - std::nullopt, std::nullopt, metadata::trace_time_clock_id, - Variadic::Integer(*state->trace_time_clock)); - } - // Resolves which machine declared by |entry| owns its source clock: the named // |source_machine| (which must be one this file itself declares), or - when // unset - the file's sole machine. A multi-machine file is several machines, @@ -620,6 +607,35 @@ base::Status PerfettoManifestReader::ApplyManifest() { path.c_str()); }; + // Claim the global trace time clock directly: the manifest is the first file, + // so its claim wins over later traces. trace_time.file (+ .machine) pins it + // to that file's (pre-allocated) machine. + if (state->trace_time_clock) { + ClockId trace_time = ClockId::Machine(*state->trace_time_clock); + if (state->trace_time_file || state->trace_time_machine) { + ASSIGN_OR_RETURN( + int64_t raw, + resolve_ref("trace_time: file", "trace_time: machine", + state->trace_time_file, state->trace_time_machine, + /*fallback=*/0)); + trace_time = ClockId::Machine(EnsureMachineRow(context_, raw), + *state->trace_time_clock); + } + // TRACE_FILE: trace time is |file|'s private per-file clock. + if (*state->trace_time_clock == protos::pbzero::BUILTIN_CLOCK_TRACE_FILE) { + if (!state->trace_time_file) { + return base::ErrStatus( + "perfetto_manifest: trace_time: clock TRACE_FILE requires file"); + } + ASSIGN_OR_RETURN(trace_time.trace_file_id, + find_trace_file_id(*state->trace_time_file)); + } + context_->trace_time_state->TrySetClock(trace_time, file_id_); + context_->global_metadata_tracker->SetMetadata( + std::nullopt, std::nullopt, metadata::trace_time_clock_id, + Variadic::Integer(*state->trace_time_clock)); + } + // Add every override's cross-machine edge to the global clock graph now, // before any file is parsed (a file may reference another parsed later), // recording it in the clock_snapshot table as ClockTracker would. The source diff --git a/test/trace_processor/diff_tests/parser/trace_manifest/tests.py b/test/trace_processor/diff_tests/parser/trace_manifest/tests.py index b3f697c890e..18226e79fc7 100644 --- a/test/trace_processor/diff_tests/parser/trace_manifest/tests.py +++ b/test/trace_processor/diff_tests/parser/trace_manifest/tests.py @@ -2013,6 +2013,115 @@ def test_sync_to_clock_omitted_clockless_ref(self): "b_slice",2000500 ''')) + # TRACE_FILE names the file's private per-file timeline explicitly, on both + # the source and the reference side; equivalent to omitting the clocks (see + # test_sync_to_clock_omitted_clockless_ref). + def test_trace_file_clock_explicit(self): + return DiffTestBlueprint( + trace=Zip({ + 'meta.json': + _meta({ + 'version': + 1, + 'files': [ + { + 'path': 'a.json' + }, + { + 'path': 'b.json', + 'clocks': { + 'clock': 'TRACE_FILE', + 'offset_ns': 500, + 'sync_to': { + 'file': 'a.json', + 'clock': 'TRACE_FILE' + } + } + }, + ], + }), + 'a.json': + _json_trace('a_slice', pid=10), + 'b.json': + _json_trace('b_slice', pid=11), + }), + query=''' + SELECT name, ts FROM slice + WHERE name IN ('a_slice', 'b_slice') + ORDER BY name; + ''', + out=Csv(''' + "name","ts" + "a_slice",2000000 + "b_slice",2000500 + ''')) + + # trace_time.clock TRACE_FILE makes the named file's private timeline trace + # time. b.json = a.json + 500, and b.json owns trace time, so b's slice keeps + # its identity ts and a's moves back by 500. + def test_trace_time_trace_file_clock(self): + return DiffTestBlueprint( + trace=Zip({ + 'meta.json': + _meta({ + 'version': + 1, + 'trace_time': { + 'clock': 'TRACE_FILE', + 'file': 'b.json' + }, + 'files': [ + { + 'path': 'a.json' + }, + { + 'path': 'b.json', + 'clocks': { + 'offset_ns': 500, + 'sync_to': { + 'file': 'a.json' + } + } + }, + ], + }), + 'a.json': + _json_trace('a_slice', pid=10), + 'b.json': + _json_trace('b_slice', pid=11), + }), + query=''' + SELECT name, ts FROM slice + WHERE name IN ('a_slice', 'b_slice') + ORDER BY name; + ''', + out=Csv(''' + "name","ts" + "a_slice",1999500 + "b_slice",2000000 + ''')) + + # A per-file clock needs to know which file. + def test_error_trace_time_trace_file_clock_no_file(self): + return DiffTestBlueprint( + trace=Zip({ + 'meta.json': + _meta({ + 'version': 1, + 'trace_time': { + 'clock': 'TRACE_FILE' + }, + 'files': [{ + 'path': 'a.json' + }], + }), + 'a.json': + _json_trace('a_slice', pid=10), + }), + query='SELECT 1;', + out=ExpectedError( + 'perfetto_manifest: trace_time: clock TRACE_FILE requires file')) + # A multi-machine source names which of its declared machines owns the related # clock: multi.pb's vm BOOTTIME = server.pb's BOOTTIME + 500. server owns trace # time, so vm's slice lands 500ns after server's.