Merge "Roll libprocinfo and libunwindstack."
diff --git a/src/perfetto_cmd/perfetto_cmd.cc b/src/perfetto_cmd/perfetto_cmd.cc
index 2873179..f7b670c 100644
--- a/src/perfetto_cmd/perfetto_cmd.cc
+++ b/src/perfetto_cmd/perfetto_cmd.cc
@@ -607,6 +607,15 @@
// In detached mode we must pass the file descriptor to the service and
// let that one write the trace. We cannot use the IPC readback code path
// because the client process is about to exit soon after detaching.
+ // We could support detach && !write_into_file, but that would make the
+ // cmdline logic more complex. The feasible configurations are:
+ // 1. Using write_into_file and passing the file path on the --detach call.
+ // 2. Using pure ring-buffer mode, setting write_into_file = false and
+ // passing the output file path to the --attach call.
+ // This is too complicated and harder to reason about, so we support only 1.
+ // Traceur gets around this by always setting write_into_file and specifying
+ // write_into_file_period = 1week (which effectively means: write into the
+ // file only at the end of the trace) to achieve ring buffer traces.
PERFETTO_ELOG(
"TraceConfig's write_into_file must be true when using --detach");
return 1;
diff --git a/src/tracing/core/tracing_service_impl.cc b/src/tracing/core/tracing_service_impl.cc
index 588da8e..26da3a6 100644
--- a/src/tracing/core/tracing_service_impl.cc
+++ b/src/tracing/core/tracing_service_impl.cc
@@ -1844,7 +1844,7 @@
// If the consumer enabled tracing and asked to save the contents into the
// passed file makes little sense to also try to read the buffers over IPC,
// as that would just steal data from the periodic draining task.
- PERFETTO_DFATAL("Consumer trying to read from write_into_file session.");
+ PERFETTO_ELOG("Consumer trying to read from write_into_file session.");
return false;
}
@@ -1852,7 +1852,9 @@
packets.reserve(1024); // Just an educated guess to avoid trivial expansions.
// If a bugreport request happened and the trace was stolen for that, give
- // an empty trace with a clear signal to the consumer.
+ // an empty trace with a clear signal to the consumer. This deals only with
+ // the case of readback-from-IPC. A similar code-path deals with the
+ // write_into_file case in MaybeSaveTraceForBugreport().
if (tracing_session->seized_for_bugreport && consumer) {
if (!tracing_session->config.builtin_data_sources()
.disable_service_events()) {
@@ -2978,14 +2980,21 @@
auto& session = session_id_and_session.second;
const int32_t score = session.config.bugreport_score();
// Exclude sessions with 0 (or below) score. By default tracing sessions
- // should NOT be eligible to be attached to bugreports. Also don't try to
- // steal long traces with write_into_file as their content is already
- // partially flushed onto a file and we can't easily recover it (also that
- // could lead to enormous traces).
- if (score <= 0 || session.state != TracingSession::STARTED ||
- session.write_into_file) {
+ // should NOT be eligible to be attached to bugreports.
+ if (score <= 0 || session.state != TracingSession::STARTED)
continue;
- }
+
+ // Also don't try to steal long traces with write_into_file if their content
+ // has been already partially written into a file, as we would get partial
+ // traces on both sides. We can't just copy the original file into the
+ // bugreport because the file could be too big (GBs) for bugreports.
+ // The only case where it's legit to steal traces with write_into_file, is
+ // when the consumer specified a very large write_period_ms (e.g. 24h),
+ // meaning that this is effectively a ring-buffer trace. Traceur (the
+ // Android System Tracing app), which uses --detach, does this to have a
+ // consistent invocation path for long-traces and ring-buffer-mode traces.
+ if (session.write_into_file && session.bytes_written_into_file > 0)
+ continue;
// If we are already in the process of finalizing another trace for
// bugreport, don't even start another one, as they would try to write onto
@@ -3006,6 +3015,27 @@
auto br_fd = CreateTraceFile(GetBugreportTmpPath(), /*overwrite=*/true);
if (!br_fd)
return false;
+
+ if (max_session->write_into_file) {
+ auto fd = *max_session->write_into_file;
+ // If we are stealing a write_into_file session, add a marker that explains
+ // why the trace has been stolen rather than creating an empty file. This is
+ // only for write_into_file traces. A similar code path deals with the case
+ // of reading-back a seized trace from IPC in ReadBuffers().
+ if (!max_session->config.builtin_data_sources().disable_service_events()) {
+ std::vector<TracePacket> packets;
+ EmitSeizedForBugreportLifecycleEvent(&packets);
+ for (auto& packet : packets) {
+ char* preamble;
+ size_t preamble_size = 0;
+ std::tie(preamble, preamble_size) = packet.GetProtoPreamble();
+ base::WriteAll(fd, preamble, preamble_size);
+ for (const Slice& slice : packet.slices()) {
+ base::WriteAll(fd, slice.start, slice.size);
+ }
+ } // for (packets)
+ } // if (!disable_service_events())
+ } // if (max_session->write_into_file)
max_session->write_into_file = std::move(br_fd);
max_session->on_disable_callback_for_bugreport = std::move(callback);
max_session->seized_for_bugreport = true;
diff --git a/test/end_to_end_integrationtest.cc b/test/end_to_end_integrationtest.cc
index 53c8369..e9617c4 100644
--- a/test/end_to_end_integrationtest.cc
+++ b/test/end_to_end_integrationtest.cc
@@ -253,6 +253,33 @@
TestHelper test_helper_{&task_runner_};
};
+// For the SaveForBugreport* tests.
+void SetTraceConfigForBugreportTest(TraceConfig* trace_config) {
+ trace_config->add_buffers()->set_size_kb(4096);
+ trace_config->set_duration_ms(60000); // Will never hit this.
+ trace_config->set_bugreport_score(10);
+ auto* ds_config = trace_config->add_data_sources()->mutable_config();
+ ds_config->set_name("android.perfetto.FakeProducer");
+ ds_config->mutable_for_testing()->set_message_count(3);
+ ds_config->mutable_for_testing()->set_message_size(10);
+ ds_config->mutable_for_testing()->set_send_batch_on_register(true);
+}
+
+// For the SaveForBugreport* tests.
+static void VerifyBugreportTraceContents() {
+ // Read the trace written in the fixed location (/data/misc/perfetto-traces/
+ // on Android, /tmp/ on Linux/Mac) and make sure it has the right contents.
+ std::string trace_str;
+ base::ReadFile(kBugreportTracePath, &trace_str);
+ ASSERT_FALSE(trace_str.empty());
+ protos::gen::Trace trace;
+ ASSERT_TRUE(trace.ParseFromString(trace_str));
+ int test_packets = 0;
+ for (const auto& p : trace.packet())
+ test_packets += p.has_for_testing() ? 1 : 0;
+ ASSERT_EQ(test_packets, 3);
+}
+
} // namespace
// If we're building on Android and starting the daemons ourselves,
@@ -852,14 +879,7 @@
helper.WaitForConsumerConnect();
TraceConfig trace_config;
- trace_config.add_buffers()->set_size_kb(4096);
- trace_config.set_duration_ms(10000);
- trace_config.set_bugreport_score(10);
- auto* ds_config = trace_config.add_data_sources()->mutable_config();
- ds_config->set_name("android.perfetto.FakeProducer");
- ds_config->mutable_for_testing()->set_message_count(3);
- ds_config->mutable_for_testing()->set_message_size(10);
- ds_config->mutable_for_testing()->set_send_batch_on_register(true);
+ SetTraceConfigForBugreportTest(&trace_config);
helper.StartTracing(trace_config);
helper.WaitForProducerEnabled();
@@ -867,17 +887,7 @@
EXPECT_TRUE(helper.SaveTraceForBugreportAndWait());
helper.WaitForTracingDisabled();
- // Read the trace written in the fixed location (/data/misc/perfetto-traces/
- // on Android, /tmp/ on Linux/Mac) and make sure it has the right contents.
- std::string trace_str;
- base::ReadFile(kBugreportTracePath, &trace_str);
- ASSERT_FALSE(trace_str.empty());
- protos::gen::Trace trace;
- ASSERT_TRUE(trace.ParseFromString(trace_str));
- int test_packets = 0;
- for (const auto& p : trace.packet())
- test_packets += p.has_for_testing() ? 1 : 0;
- ASSERT_EQ(test_packets, 3);
+ VerifyBugreportTraceContents();
// Now read the trace returned to the consumer via ReadBuffers. This should
// be always empty because --save-for-bugreport takes it over and makes the
@@ -894,6 +904,43 @@
}
}
+// Tests that the SaveForBugreport logic works also for traces with
+// write_into_file = true (with a passed file descriptor).
+TEST_F(PerfettoTest, SaveForBugreport_WriteIntoFile) {
+ base::TestTaskRunner task_runner;
+
+ TestHelper helper(&task_runner);
+ helper.StartServiceIfRequired();
+ helper.ConnectFakeProducer();
+ helper.ConnectConsumer();
+ helper.WaitForConsumerConnect();
+
+ TraceConfig trace_config;
+ SetTraceConfigForBugreportTest(&trace_config);
+ trace_config.set_file_write_period_ms(60000); // Will never hit this.
+ trace_config.set_write_into_file(true);
+
+ auto pipe_pair = base::Pipe::Create();
+ helper.StartTracing(trace_config, std::move(pipe_pair.wr));
+ helper.WaitForProducerEnabled();
+
+ EXPECT_TRUE(helper.SaveTraceForBugreportAndWait());
+ helper.WaitForTracingDisabled();
+
+ VerifyBugreportTraceContents();
+
+ // Now read the original file descriptor passed in.
+ std::string trace_bytes;
+ ASSERT_TRUE(base::ReadPlatformHandle(*pipe_pair.rd, &trace_bytes));
+ protos::gen::Trace trace;
+ ASSERT_TRUE(trace.ParseFromString(trace_bytes));
+ ASSERT_EQ(trace.packet().size(), 1u);
+ for (const auto& p : trace.packet()) {
+ ASSERT_TRUE(p.has_service_event());
+ ASSERT_TRUE(p.service_event().seized_for_bugreport());
+ }
+}
+
// Disable cmdline tests on sanitizets because they use fork() and that messes
// up leak / races detections, which has been fixed only recently (see
// https://github.com/google/sanitizers/issues/836 ).
