Merge "tp: fix tracking when process reuse happens before ftrace starts" into main
diff --git a/bazel/proto_gen.bzl b/bazel/proto_gen.bzl
index c5fd127..e237ebc 100644
--- a/bazel/proto_gen.bzl
+++ b/bazel/proto_gen.bzl
@@ -46,13 +46,17 @@
strip_base_path = ctx.label.package + "/"
elif ctx.label.workspace_root:
# This path is hit when proto targets are built as @perfetto//:xxx
- # instead of //:xxx. This happens in embedder builds. In this case,
- # workspace_root == "external/perfetto" and we need to rebase the paths
- # passed to protoc.
+ # instead of //:xxx. This happens in embedder builds.
proto_path = ctx.label.workspace_root
- out_dir += "/" + ctx.label.workspace_root
+
+ # We could be using the sibling repository layout, in which case we do nothing.
+ if not ctx.label.workspace_root.startswith("../"):
+ # workspace_root == "external/perfetto" and we need to rebase the paths
+ # passed to protoc.
+ out_dir += "/" + ctx.label.workspace_root
strip_base_path = ctx.label.workspace_root + "/"
+
out_files = []
suffix = ctx.attr.suffix
for src in proto_src:
diff --git a/protos/perfetto/trace/android/BUILD.gn b/protos/perfetto/trace/android/BUILD.gn
index 900a64b..90234da 100644
--- a/protos/perfetto/trace/android/BUILD.gn
+++ b/protos/perfetto/trace/android/BUILD.gn
@@ -15,7 +15,10 @@
import("../../../../gn/proto_library.gni")
perfetto_proto_library("@TYPE@") {
- deps = [ "../../common:@TYPE@", ":winscope_regular_@TYPE@" ]
+ deps = [
+ ":winscope_regular_@TYPE@",
+ "../../common:@TYPE@",
+ ]
sources = [
"android_game_intervention_list.proto",
@@ -44,8 +47,8 @@
# Winscope messages added to TracePacket directly
perfetto_proto_library("winscope_regular_@TYPE@") {
deps = [
- "../../common:@TYPE@",
":winscope_common_@TYPE@",
+ "../../common:@TYPE@",
]
sources = [
"protolog.proto",
@@ -64,19 +67,19 @@
]
public_deps = [ ":winscope_common_@TYPE@" ]
sources = [
- "inputmethodeditor.proto",
"graphics/pixelformat.proto",
+ "inputmethodeditor.proto",
"inputmethodservice/inputmethodservice.proto",
"inputmethodservice/softinputwindow.proto",
"server/inputmethod/inputmethodmanagerservice.proto",
"typedef.proto",
- "view/inputmethod/editorinfo.proto",
- "view/inputmethod/inputconnection.proto",
- "view/inputmethod/inputmethodmanager.proto",
"view/display.proto",
"view/displaycutout.proto",
"view/imefocuscontroller.proto",
"view/imeinsetssourceconsumer.proto",
+ "view/inputmethod/editorinfo.proto",
+ "view/inputmethod/inputconnection.proto",
+ "view/inputmethod/inputmethodmanager.proto",
"view/insetsanimationcontrolimpl.proto",
"view/insetscontroller.proto",
"view/insetssource.proto",
@@ -95,8 +98,8 @@
proto_generators = [ "descriptor" ]
generate_descriptor = "winscope.descriptor"
deps = [
- ":winscope_regular_source_set",
":winscope_extensions_source_set",
+ ":winscope_regular_source_set",
]
sources = [ "winscope.proto" ]
import_dirs = [ "${perfetto_protobuf_src_dir}" ]
diff --git a/src/trace_redaction/main.cc b/src/trace_redaction/main.cc
index 296f12f..78bf3d0 100644
--- a/src/trace_redaction/main.cc
+++ b/src/trace_redaction/main.cc
@@ -84,10 +84,18 @@
auto* redact_ftrace_events = redactor.emplace_transform<RedactFtraceEvent>();
redact_ftrace_events
- ->emplace_back<RedactTaskNewTask::kFieldId, RedactTaskNewTask>();
- redact_ftrace_events
->emplace_back<RemoveProcessFreeComm::kFieldId, RemoveProcessFreeComm>();
+ // By default, the comm value is cleared. However, when thread merging is
+ // enabled (kTaskNewtaskFieldNumber + ThreadMergeDropField), the event is
+ // dropped, meaning that this primitive was effectivly a no-op. This primitive
+ // remains so that removing thread merging won't leak thread names via new
+ // task events.
+ auto* redact_new_task =
+ redact_ftrace_events
+ ->emplace_back<RedactTaskNewTask::kFieldId, RedactTaskNewTask>();
+ redact_new_task->emplace_back<ClearComms>();
+
// This set of transformations will change pids. This will break the
// connections between pids and the timeline (the synth threads are not in the
// timeline). If a transformation uses the timeline, it must be before this
diff --git a/src/trace_redaction/redact_ftrace_event.h b/src/trace_redaction/redact_ftrace_event.h
index 9de6cc6..68aff05 100644
--- a/src/trace_redaction/redact_ftrace_event.h
+++ b/src/trace_redaction/redact_ftrace_event.h
@@ -57,8 +57,12 @@
// Add a new redaction. T must extend FtraceEventRedaction. This relies on the
// honor system; no more than one redaction can be mapped to a field.
template <uint32_t field_id, class T>
- void emplace_back() {
- redactions_.Insert(field_id, std::make_unique<T>());
+ T* emplace_back() {
+ auto ptr = std::make_unique<T>();
+ T* raw_ptr = ptr.get();
+
+ redactions_.Insert(field_id, std::move(ptr));
+ return raw_ptr;
}
private:
diff --git a/src/trace_redaction/redact_task_newtask.cc b/src/trace_redaction/redact_task_newtask.cc
index fb0cfe8..7a0fbeb 100644
--- a/src/trace_redaction/redact_task_newtask.cc
+++ b/src/trace_redaction/redact_task_newtask.cc
@@ -16,28 +16,15 @@
#include "src/trace_redaction/redact_task_newtask.h"
-#include "src/trace_redaction/proto_util.h"
-
#include "protos/perfetto/trace/ftrace/ftrace_event.pbzero.h"
#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.pbzero.h"
#include "protos/perfetto/trace/ftrace/task.pbzero.h"
+#include "src/trace_processor/util/status_macros.h"
namespace perfetto::trace_redaction {
namespace {
-// TODO(vaage): Merge with RedactComm in redact_sched_switch.cc.
-protozero::ConstChars RedactComm(const Context& context,
- uint64_t ts,
- int32_t pid,
- protozero::ConstChars comm) {
- if (context.timeline->PidConnectsToUid(ts, pid, *context.package_uid)) {
- return comm;
- }
-
- return {};
-}
-
} // namespace
// Redact sched switch trace events in an ftrace event bundle:
//
@@ -56,9 +43,11 @@
// equal to "event.task_newtask.pid" (a thread cannot start itself).
base::Status RedactTaskNewTask::Redact(
const Context& context,
- const protos::pbzero::FtraceEventBundle::Decoder&,
+ const protos::pbzero::FtraceEventBundle::Decoder& bundle,
protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const {
+ PERFETTO_DCHECK(transform_);
+
if (!context.package_uid.has_value()) {
return base::ErrStatus("RedactTaskNewTask: missing package uid");
}
@@ -68,45 +57,41 @@
}
// The timestamp is needed to do the timeline look-up. If the packet has no
- // timestamp, don't add the sched switch event. This is the safest option.
- auto timestamp =
+ // timestamp, don't add the sched switch event.
+ auto timestamp_field =
event.FindField(protos::pbzero::FtraceEvent::kTimestampFieldNumber);
- if (!timestamp.valid()) {
- return base::OkStatus();
- }
-
- auto new_task =
+ auto new_task_field =
event.FindField(protos::pbzero::FtraceEvent::kTaskNewtaskFieldNumber);
- if (!new_task.valid()) {
+
+ if (!timestamp_field.valid() || !new_task_field.valid()) {
return base::ErrStatus(
- "RedactTaskNewTask: was used for unsupported field type");
+ "RedactTaskNewTask: missing required FtraceEvent field.");
}
- protozero::ProtoDecoder new_task_decoder(new_task.as_bytes());
+ protos::pbzero::TaskNewtaskFtraceEvent::Decoder new_task(
+ new_task_field.as_bytes());
- auto pid = new_task_decoder.FindField(
- protos::pbzero::TaskNewtaskFtraceEvent::kPidFieldNumber);
-
- if (!pid.valid()) {
- return base::OkStatus();
+ // There are only four fields in a new task event. Since two of them can
+ // change, it is easier to work with them directly.
+ if (!new_task.has_pid() || !new_task.has_comm() ||
+ !new_task.has_clone_flags() || !new_task.has_oom_score_adj()) {
+ return base::ErrStatus(
+ "RedactTaskNewTask: missing required TaskNewtaskFtraceEvent field.");
}
- // Avoid making the message until we know that we have prev and next pids.
+ auto pid = new_task.pid();
+ auto comm = new_task.comm().ToStdString();
+
+ auto cpu = static_cast<int32_t>(bundle.cpu());
+
+ RETURN_IF_ERROR(transform_->Transform(context, timestamp_field.as_uint64(),
+ cpu, &pid, &comm));
+
auto* new_task_message = event_message->set_task_newtask();
-
- for (auto field = new_task_decoder.ReadField(); field.valid();
- field = new_task_decoder.ReadField()) {
- // Perfetto view (ui.perfetto.dev) crashes if the comm value is missing.
- // To work around this, the comm value is replaced with an empty string.
- // This appears to work.
- if (field.id() ==
- protos::pbzero::TaskNewtaskFtraceEvent::kCommFieldNumber) {
- new_task_message->set_comm(RedactComm(context, timestamp.as_uint64(),
- pid.as_int32(), field.as_string()));
- } else {
- proto_util::AppendField(field, new_task_message);
- }
- }
+ new_task_message->set_pid(pid);
+ new_task_message->set_comm(comm);
+ new_task_message->set_clone_flags(new_task.clone_flags());
+ new_task_message->set_oom_score_adj(new_task.oom_score_adj());
return base::OkStatus();
}
diff --git a/src/trace_redaction/redact_task_newtask.h b/src/trace_redaction/redact_task_newtask.h
index 51c1124..5b589fc 100644
--- a/src/trace_redaction/redact_task_newtask.h
+++ b/src/trace_redaction/redact_task_newtask.h
@@ -18,6 +18,7 @@
#define SRC_TRACE_REDACTION_REDACT_TASK_NEWTASK_H_
#include "src/trace_redaction/redact_ftrace_event.h"
+#include "src/trace_redaction/redact_sched_switch.h"
#include "src/trace_redaction/trace_redaction_framework.h"
namespace perfetto::trace_redaction {
@@ -34,6 +35,14 @@
const protos::pbzero::FtraceEventBundle::Decoder& bundle,
protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const override;
+
+ template <class Transform>
+ void emplace_back() {
+ transform_ = std::make_unique<Transform>();
+ }
+
+ public:
+ std::unique_ptr<SchedSwitchTransform> transform_;
};
} // namespace perfetto::trace_redaction
diff --git a/src/trace_redaction/redact_task_newtask_unittest.cc b/src/trace_redaction/redact_task_newtask_unittest.cc
index 33886f6..11b1405 100644
--- a/src/trace_redaction/redact_task_newtask_unittest.cc
+++ b/src/trace_redaction/redact_task_newtask_unittest.cc
@@ -16,19 +16,21 @@
#include "src/trace_redaction/redact_task_newtask.h"
#include "perfetto/protozero/scattered_heap_buffer.h"
-#include "protos/perfetto/trace/ftrace/task.gen.h"
+#include "src/base/test/status_matchers.h"
#include "test/gtest_and_gmock.h"
#include "protos/perfetto/trace/ftrace/ftrace_event.gen.h"
#include "protos/perfetto/trace/ftrace/ftrace_event.pbzero.h"
#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.gen.h"
-#include "protos/perfetto/trace/ftrace/sched.gen.h"
+#include "protos/perfetto/trace/ftrace/task.gen.h"
#include "protos/perfetto/trace/trace.gen.h"
#include "protos/perfetto/trace/trace_packet.gen.h"
namespace perfetto::trace_redaction {
namespace {
+constexpr uint64_t kCpu = 1;
+
constexpr uint64_t kUidA = 1;
constexpr uint64_t kUidB = 2;
@@ -44,19 +46,23 @@
class RedactTaskNewTaskTest : public testing::Test {
protected:
void SetUp() override {
- auto* event = bundle_.add_event();
+ bundle_.set_cpu(kCpu);
+ auto* event = bundle_.add_event();
event->set_timestamp(123456789);
event->set_pid(kPidA);
auto* new_task = event->mutable_task_newtask();
+ new_task->set_clone_flags(0);
new_task->set_comm(std::string(kCommA));
+ new_task->set_oom_score_adj(0);
new_task->set_pid(kPidA);
}
base::Status Redact(const Context& context,
protos::pbzero::FtraceEvent* event_message) {
RedactTaskNewTask redact;
+ redact.emplace_back<ClearComms>();
auto bundle_str = bundle_.SerializeAsString();
protos::pbzero::FtraceEventBundle::Decoder bundle_decoder(bundle_str);
@@ -93,8 +99,6 @@
};
TEST_F(RedactTaskNewTaskTest, RejectMissingPackageUid) {
- RedactTaskNewTask redact;
-
Context context;
context.timeline = std::make_unique<ProcessThreadTimeline>();
@@ -106,8 +110,6 @@
}
TEST_F(RedactTaskNewTaskTest, RejectMissingTimeline) {
- RedactTaskNewTask redact;
-
Context context;
context.package_uid = kUidA;
@@ -119,8 +121,6 @@
}
TEST_F(RedactTaskNewTaskTest, PidInPackageKeepsComm) {
- RedactTaskNewTask redact;
-
// Because Uid A is the target, when Pid A starts (new task event), it should
// keep its comm value.
Context context;
@@ -130,8 +130,7 @@
protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
- auto result = Redact(context, event_message.get());
- ASSERT_TRUE(result.ok());
+ ASSERT_OK(Redact(context, event_message.get()));
protos::gen::FtraceEvent redacted_event;
redacted_event.ParseFromString(event_message.SerializeAsString());
@@ -142,8 +141,6 @@
}
TEST_F(RedactTaskNewTaskTest, PidOutsidePackageLosesComm) {
- RedactTaskNewTask redact;
-
// Because Uid B is the target, when Pid A starts (new task event), it should
// lose its comm value.
Context context;
@@ -153,8 +150,7 @@
protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
- auto result = Redact(context, event_message.get());
- ASSERT_TRUE(result.ok());
+ ASSERT_OK(Redact(context, event_message.get()));
protos::gen::FtraceEvent redacted_event;
redacted_event.ParseFromString(event_message.SerializeAsString());
diff --git a/src/traced/probes/ftrace/cpu_reader.cc b/src/traced/probes/ftrace/cpu_reader.cc
index 66b9b1b..4045978 100644
--- a/src/traced/probes/ftrace/cpu_reader.cc
+++ b/src/traced/probes/ftrace/cpu_reader.cc
@@ -869,6 +869,9 @@
case kDevId64ToUint64:
ReadDevId<uint64_t>(field_start, field_id, message, metadata);
return true;
+ case kFtraceSymAddr32ToUint64:
+ ReadSymbolAddr<uint32_t>(field_start, field_id, message, metadata);
+ return true;
case kFtraceSymAddr64ToUint64:
ReadSymbolAddr<uint64_t>(field_start, field_id, message, metadata);
return true;
diff --git a/src/traced/probes/ftrace/event_info_constants.cc b/src/traced/probes/ftrace/event_info_constants.cc
index 37f9c77..4a0d08f 100644
--- a/src/traced/probes/ftrace/event_info_constants.cc
+++ b/src/traced/probes/ftrace/event_info_constants.cc
@@ -106,6 +106,8 @@
*out = kBoolToUint64;
} else if (ftrace == kFtraceDataLoc && proto == ProtoSchemaType::kString) {
*out = kDataLocToString;
+ } else if (ftrace == kFtraceSymAddr32 && proto == ProtoSchemaType::kUint64) {
+ *out = kFtraceSymAddr32ToUint64;
} else if (ftrace == kFtraceSymAddr64 && proto == ProtoSchemaType::kUint64) {
*out = kFtraceSymAddr64ToUint64;
} else {
diff --git a/src/traced/probes/ftrace/event_info_constants.h b/src/traced/probes/ftrace/event_info_constants.h
index 0283f12..cce144f 100644
--- a/src/traced/probes/ftrace/event_info_constants.h
+++ b/src/traced/probes/ftrace/event_info_constants.h
@@ -48,6 +48,7 @@
kFtraceDevId32,
kFtraceDevId64,
kFtraceDataLoc,
+ kFtraceSymAddr32,
kFtraceSymAddr64,
};
@@ -84,6 +85,7 @@
kDevId32ToUint64,
kDevId64ToUint64,
kDataLocToString,
+ kFtraceSymAddr32ToUint64,
kFtraceSymAddr64ToUint64,
};
@@ -127,6 +129,7 @@
return "devid64";
case kFtraceDataLoc:
return "__data_loc";
+ case kFtraceSymAddr32:
case kFtraceSymAddr64:
return "void*";
case kInvalidFtraceFieldType:
diff --git a/src/traced/probes/ftrace/proto_translation_table.cc b/src/traced/probes/ftrace/proto_translation_table.cc
index 9dcde14..7e9092e 100644
--- a/src/traced/probes/ftrace/proto_translation_table.cc
+++ b/src/traced/probes/ftrace/proto_translation_table.cc
@@ -239,6 +239,7 @@
case kFtraceUint64:
case kFtraceInode32:
case kFtraceInode64:
+ case kFtraceSymAddr32:
case kFtraceSymAddr64:
*proto_type = ProtoSchemaType::kUint64;
*proto_field_id = GenericFtraceEvent::Field::kUintValueFieldNumber;
@@ -310,13 +311,16 @@
return true;
}
- // Kernel addresses that need symbolization via kallsyms. Only 64-bit kernels
- // are supported for now. 32-bit kernels seems to be going away.
- if ((base::StartsWith(type_and_name, "void*") ||
- base::StartsWith(type_and_name, "void *")) &&
- size == 8) {
- *out = kFtraceSymAddr64;
- return true;
+ // Kernel addresses that need symbolization via kallsyms.
+ if (base::StartsWith(type_and_name, "void*") ||
+ base::StartsWith(type_and_name, "void *")) {
+ if (size == 4) {
+ *out = kFtraceSymAddr32;
+ return true;
+ } else if (size == 8) {
+ *out = kFtraceSymAddr64;
+ return true;
+ }
}
// Variable length strings: "char foo" + size: 0 (as in 'print').
