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(