Skip to content

tp: fix manifest offset_ns ignored for snapshot-less proto files #7876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LalitMaganti merged 1 commit into from
Oct 12, 2026
Merged

tp: fix manifest offset_ns ignored for snapshot-less proto files #7876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions src/trace_processor/forwarding_trace_parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,17 +232,31 @@ 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
// (primary_trace_clock / ClockSnapshot) so it does not set the default clock
// (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) {
Expand Down
5 changes: 5 additions & 0 deletions src/trace_processor/plugins/perfetto_manifest/perfetto_manifest_reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ base::StatusOr<ClockOverride> 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(
Expand Down
126 changes: 126 additions & 0 deletions test/trace_processor/diff_tests/parser/trace_manifest/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Expand Down Expand Up @@ -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(
Expand Down
Loading