TracedValue: add dchecks that the scope is always correct.
Each time a value is being written into a TracedValue / TracedArray
/ TracedDictionary, DCHECK that it's an active one.
Active bit is tracked on each scope in the following way:
- When calling Write* for primitive types, active bit is passed back
to the parent.
- When calling Write* for complex types, active bit is passed to the
newly-created scope.
- When destroying the scope, active bit is passed back to the parent.
Also add a few death tests checking that the dchecks actually fail.
R=primiano@google.com,eseckler@google.com,skyostil@google.com
Change-Id: Iab5122a2317c2ea7ad646039ea6e40a6fe7195ba
diff --git a/Android.bp b/Android.bp
index 8c9f234..a8a334e 100644
--- a/Android.bp
+++ b/Android.bp
@@ -8218,6 +8218,7 @@
"src/tracing/debug_annotation.cc",
"src/tracing/event_context.cc",
"src/tracing/interceptor.cc",
+ "src/tracing/internal/checked_scope.cc",
"src/tracing/internal/interceptor_trace_writer.cc",
"src/tracing/internal/tracing_muxer_fake.cc",
"src/tracing/internal/tracing_muxer_impl.cc",
diff --git a/BUILD b/BUILD
index 3303844..256f5f5 100644
--- a/BUILD
+++ b/BUILD
@@ -513,6 +513,7 @@
"include/perfetto/tracing/event_context.h",
"include/perfetto/tracing/interceptor.h",
"include/perfetto/tracing/internal/basic_types.h",
+ "include/perfetto/tracing/internal/checked_scope.h",
"include/perfetto/tracing/internal/data_source_internal.h",
"include/perfetto/tracing/internal/in_process_tracing_backend.h",
"include/perfetto/tracing/internal/interceptor_trace_writer.h",
@@ -1571,6 +1572,7 @@
"src/tracing/debug_annotation.cc",
"src/tracing/event_context.cc",
"src/tracing/interceptor.cc",
+ "src/tracing/internal/checked_scope.cc",
"src/tracing/internal/interceptor_trace_writer.cc",
"src/tracing/internal/tracing_muxer_fake.cc",
"src/tracing/internal/tracing_muxer_fake.h",
diff --git a/include/perfetto/tracing/BUILD.gn b/include/perfetto/tracing/BUILD.gn
index 566636b..c48ea76 100644
--- a/include/perfetto/tracing/BUILD.gn
+++ b/include/perfetto/tracing/BUILD.gn
@@ -34,6 +34,7 @@
"event_context.h",
"interceptor.h",
"internal/basic_types.h",
+ "internal/checked_scope.h",
"internal/data_source_internal.h",
"internal/in_process_tracing_backend.h",
"internal/interceptor_trace_writer.h",
diff --git a/include/perfetto/tracing/internal/checked_scope.h b/include/perfetto/tracing/internal/checked_scope.h
new file mode 100644
index 0000000..53ef55c
--- /dev/null
+++ b/include/perfetto/tracing/internal/checked_scope.h
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef INCLUDE_PERFETTO_TRACING_INTERNAL_CHECKED_SCOPE_H_
+#define INCLUDE_PERFETTO_TRACING_INTERNAL_CHECKED_SCOPE_H_
+
+#include "perfetto/base/logging.h"
+
+namespace perfetto {
+namespace internal {
+
+#if PERFETTO_DCHECK_IS_ON()
+
+// Checker to ensure that despite multiple scopes being present, only the active
+// one is being accessed. Rules:
+// - Only an active scope can create inner scopes. When this happens, it stops
+// being active and the inner scope becomes active instead.
+// - Only an active scope can be destroyed. When this happens, its parent scope
+// becomes active.
+class CheckedScope {
+ public:
+ explicit CheckedScope(CheckedScope* parent_scope);
+ ~CheckedScope();
+ CheckedScope(CheckedScope&&);
+ CheckedScope& operator=(CheckedScope&&);
+ CheckedScope(const CheckedScope&) = delete;
+ CheckedScope& operator=(const CheckedScope&) = delete;
+
+ void Reset();
+
+ CheckedScope* parent_scope() const { return parent_scope_; }
+ bool is_active() const { return is_active_; }
+
+ private:
+ void set_is_active(bool is_active) { is_active_ = is_active; }
+
+ bool is_active_ = true;
+ CheckedScope* parent_scope_;
+
+ bool deleted_ = false;
+};
+
+#else
+
+// Dummy for cases when DCHECK is not enabled. Methods are marked constexpr to
+// ensure that the compiler can inline and optimise them away.
+class CheckedScope {
+ public:
+ inline explicit CheckedScope(CheckedScope*) {}
+ inline ~CheckedScope() {}
+
+ inline void Reset() {}
+
+ inline CheckedScope* parent_scope() const { return nullptr; }
+ inline bool is_active() const { return true; }
+};
+
+#endif // PERFETTO_DCHECK_IS_ON()
+
+} // namespace internal
+} // namespace perfetto
+
+#endif // INCLUDE_PERFETTO_TRACING_INTERNAL_CHECKED_SCOPE_H_
diff --git a/include/perfetto/tracing/traced_value.h b/include/perfetto/tracing/traced_value.h
index 9369640..37a243f 100644
--- a/include/perfetto/tracing/traced_value.h
+++ b/include/perfetto/tracing/traced_value.h
@@ -19,6 +19,7 @@
#include "perfetto/base/compiler.h"
#include "perfetto/base/export.h"
+#include "perfetto/tracing/internal/checked_scope.h"
#include "protos/perfetto/trace/track_event/debug_annotation.pbzero.h"
#include <type_traits>
@@ -149,11 +150,13 @@
friend class TracedArray;
friend class TracedDictionary;
- inline explicit TracedValue(protos::pbzero::DebugAnnotation* root_context)
- : root_context_(root_context) {}
+ inline explicit TracedValue(protos::pbzero::DebugAnnotation* root_context,
+ internal::CheckedScope* parent_scope)
+ : root_context_(root_context), checked_scope_(parent_scope) {}
inline explicit TracedValue(
- protos::pbzero::DebugAnnotation::NestedValue* nested_context)
- : nested_context_(nested_context) {}
+ protos::pbzero::DebugAnnotation::NestedValue* nested_context,
+ internal::CheckedScope* parent_scope)
+ : nested_context_(nested_context), checked_scope_(parent_scope) {}
// Temporary support for perfetto::DebugAnnotation C++ class before it's going
// to be replaced by TracedValue.
@@ -165,6 +168,8 @@
// this duplication.
protos::pbzero::DebugAnnotation* root_context_ = nullptr;
protos::pbzero::DebugAnnotation::NestedValue* nested_context_ = nullptr;
+
+ internal::CheckedScope checked_scope_;
};
class TracedArray {
@@ -173,7 +178,7 @@
TracedArray& operator=(const TracedArray&) = delete;
TracedArray& operator=(TracedArray&&) = delete;
TracedArray(TracedArray&&) = default;
- ~TracedArray() { value_->Finalize(); }
+ ~TracedArray() = default;
TracedValue AppendItem();
@@ -184,10 +189,13 @@
friend class TracedValue;
inline explicit TracedArray(
- protos::pbzero::DebugAnnotation::NestedValue* value)
- : value_(value) {}
+ protos::pbzero::DebugAnnotation::NestedValue* value,
+ internal::CheckedScope* parent_scope)
+ : value_(value), checked_scope_(parent_scope) {}
protos::pbzero::DebugAnnotation::NestedValue* value_;
+
+ internal::CheckedScope checked_scope_;
};
class TracedDictionary {
@@ -196,7 +204,7 @@
TracedDictionary& operator=(const TracedDictionary&) = delete;
TracedDictionary& operator=(TracedDictionary&&) = delete;
TracedDictionary(TracedDictionary&&) = default;
- ~TracedDictionary() {}
+ ~TracedDictionary() = default;
TracedValue AddItem(const char* key);
@@ -207,10 +215,13 @@
friend class TracedValue;
inline explicit TracedDictionary(
- protos::pbzero::DebugAnnotation::NestedValue* value)
- : value_(value) {}
+ protos::pbzero::DebugAnnotation::NestedValue* value,
+ internal::CheckedScope* parent_scope)
+ : value_(value), checked_scope_(parent_scope) {}
protos::pbzero::DebugAnnotation::NestedValue* value_;
+
+ internal::CheckedScope checked_scope_;
};
} // namespace perfetto
diff --git a/src/tracing/BUILD.gn b/src/tracing/BUILD.gn
index ea23cd8..f4b0b79 100644
--- a/src/tracing/BUILD.gn
+++ b/src/tracing/BUILD.gn
@@ -103,6 +103,7 @@
"debug_annotation.cc",
"event_context.cc",
"interceptor.cc",
+ "internal/checked_scope.cc",
"internal/interceptor_trace_writer.cc",
"internal/tracing_muxer_fake.cc",
"internal/tracing_muxer_fake.h",
diff --git a/src/tracing/internal/checked_scope.cc b/src/tracing/internal/checked_scope.cc
new file mode 100644
index 0000000..7de79e2
--- /dev/null
+++ b/src/tracing/internal/checked_scope.cc
@@ -0,0 +1,68 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "perfetto/tracing/internal/checked_scope.h"
+
+#include <utility>
+
+namespace perfetto {
+namespace internal {
+
+#if PERFETTO_DCHECK_IS_ON()
+CheckedScope::CheckedScope(CheckedScope* parent_scope)
+ : parent_scope_(parent_scope) {
+ if (parent_scope_) {
+ PERFETTO_DCHECK(parent_scope_->is_active());
+ parent_scope_->set_is_active(false);
+ }
+}
+
+CheckedScope::~CheckedScope() {
+ Reset();
+}
+
+void CheckedScope::Reset() {
+ if (!is_active_) {
+ // The only case when inactive scope could be destroyed is when Reset() was
+ // called explicitly or the contents of the object were moved away.
+ PERFETTO_DCHECK(deleted_);
+ return;
+ }
+ is_active_ = false;
+ deleted_ = true;
+ if (parent_scope_)
+ parent_scope_->set_is_active(true);
+}
+
+CheckedScope::CheckedScope(CheckedScope&& other) {
+ *this = std::move(other);
+}
+
+CheckedScope& CheckedScope::operator=(CheckedScope&& other) {
+ is_active_ = other.is_active_;
+ parent_scope_ = other.parent_scope_;
+ deleted_ = other.deleted_;
+
+ other.is_active_ = false;
+ other.parent_scope_ = nullptr;
+ other.deleted_ = true;
+
+ return *this;
+}
+#endif
+
+} // namespace internal
+} // namespace perfetto
diff --git a/src/tracing/traced_value.cc b/src/tracing/traced_value.cc
index c113388..0d0d64a 100644
--- a/src/tracing/traced_value.cc
+++ b/src/tracing/traced_value.cc
@@ -25,10 +25,11 @@
// static
TracedValue TracedValue::CreateForTest(
protos::pbzero::DebugAnnotation* context) {
- return TracedValue(context);
+ return TracedValue(context, nullptr);
}
void TracedValue::WriteInt64(int64_t value) && {
+ PERFETTO_DCHECK(checked_scope_.is_active());
if (nested_context_) {
nested_context_->set_int_value(value);
} else {
@@ -37,6 +38,7 @@
}
void TracedValue::WriteUInt64(uint64_t value) && {
+ PERFETTO_DCHECK(checked_scope_.is_active());
if (nested_context_) {
nested_context_->set_int_value(static_cast<int64_t>(value));
} else {
@@ -45,6 +47,7 @@
}
void TracedValue::WriteDouble(double value) && {
+ PERFETTO_DCHECK(checked_scope_.is_active());
if (nested_context_) {
nested_context_->set_double_value(value);
} else {
@@ -53,6 +56,7 @@
}
void TracedValue::WriteBoolean(bool value) && {
+ PERFETTO_DCHECK(checked_scope_.is_active());
if (nested_context_) {
nested_context_->set_bool_value(value);
} else {
@@ -61,6 +65,7 @@
}
void TracedValue::WriteString(const char* value) && {
+ PERFETTO_DCHECK(checked_scope_.is_active());
if (nested_context_) {
nested_context_->set_string_value(value);
} else {
@@ -69,6 +74,7 @@
}
void TracedValue::WriteString(const std::string& value) && {
+ PERFETTO_DCHECK(checked_scope_.is_active());
if (nested_context_) {
nested_context_->set_string_value(value);
} else {
@@ -77,6 +83,7 @@
}
void TracedValue::WritePointer(const void* value) && {
+ PERFETTO_DCHECK(checked_scope_.is_active());
if (nested_context_) {
nested_context_->set_int_value(reinterpret_cast<int64_t>(value));
} else {
@@ -85,59 +92,75 @@
}
TracedDictionary TracedValue::WriteDictionary() && {
+ // Note: this passes |checked_scope_.is_active_| bit to the parent to be
+ // picked up later by the new TracedDictionary.
+ PERFETTO_DCHECK(checked_scope_.is_active());
+ checked_scope_.Reset();
+
if (nested_context_) {
PERFETTO_DCHECK(!nested_context_->is_finalized());
nested_context_->set_nested_type(
protos::pbzero::DebugAnnotation_NestedValue_NestedType_DICT);
- return TracedDictionary(nested_context_);
+ return TracedDictionary(nested_context_, checked_scope_.parent_scope());
} else {
PERFETTO_DCHECK(!root_context_->is_finalized());
protos::pbzero::DebugAnnotation::NestedValue* value =
root_context_->set_nested_value();
value->set_nested_type(
protos::pbzero::DebugAnnotation_NestedValue_NestedType_DICT);
- return TracedDictionary(value);
+ return TracedDictionary(value, checked_scope_.parent_scope());
}
}
TracedArray TracedValue::WriteArray() && {
+ // Note: this passes |checked_scope_.is_active_| bit to the parent to be
+ // picked up later by the new TracedDictionary.
+ PERFETTO_DCHECK(checked_scope_.is_active());
+ checked_scope_.Reset();
+
if (nested_context_) {
PERFETTO_DCHECK(!nested_context_->is_finalized());
nested_context_->set_nested_type(
protos::pbzero::DebugAnnotation_NestedValue_NestedType_ARRAY);
- return TracedArray(nested_context_);
+ return TracedArray(nested_context_, checked_scope_.parent_scope());
} else {
PERFETTO_DCHECK(!root_context_->is_finalized());
protos::pbzero::DebugAnnotation::NestedValue* value =
root_context_->set_nested_value();
value->set_nested_type(
protos::pbzero::DebugAnnotation_NestedValue_NestedType_ARRAY);
- return TracedArray(value);
+ return TracedArray(value, checked_scope_.parent_scope());
}
}
TracedValue TracedArray::AppendItem() {
- return TracedValue(value_->add_array_values());
+ PERFETTO_DCHECK(checked_scope_.is_active());
+ return TracedValue(value_->add_array_values(), &checked_scope_);
}
TracedDictionary TracedArray::AppendDictionary() {
+ PERFETTO_DCHECK(checked_scope_.is_active());
return AppendItem().WriteDictionary();
}
TracedArray TracedArray::AppendArray() {
+ PERFETTO_DCHECK(checked_scope_.is_active());
return AppendItem().WriteArray();
}
TracedValue TracedDictionary::AddItem(const char* key) {
+ PERFETTO_DCHECK(checked_scope_.is_active());
value_->add_dict_keys(key);
- return TracedValue(value_->add_dict_values());
+ return TracedValue(value_->add_dict_values(), &checked_scope_);
}
TracedDictionary TracedDictionary::AddDictionary(const char* key) {
+ PERFETTO_DCHECK(checked_scope_.is_active());
return AddItem(key).WriteDictionary();
}
TracedArray TracedDictionary::AddArray(const char* key) {
+ PERFETTO_DCHECK(checked_scope_.is_active());
return AddItem(key).WriteArray();
}
diff --git a/src/tracing/traced_value_unittest.cc b/src/tracing/traced_value_unittest.cc
index 0fe358b..926e8ed 100644
--- a/src/tracing/traced_value_unittest.cc
+++ b/src/tracing/traced_value_unittest.cc
@@ -170,4 +170,49 @@
MessageToJSON(message.SerializeAsString()));
}
+#if PERFETTO_DCHECK_IS_ON()
+// This death test makes sense only when dchecks are enabled.
+TEST(TracedValueTest, FailOnIncorrectUsage) {
+ // A new call to AddItem is not allowed before the previous result is
+ // consumed.
+ EXPECT_DEATH(
+
+ {
+ protozero::HeapBuffered<protos::pbzero::DebugAnnotation> message;
+ auto dict = TracedValue::CreateForTest(message.get()).WriteDictionary();
+ auto scope1 = dict.AddItem("key1");
+ auto scope2 = dict.AddItem("key2");
+ std::move(scope1).WriteInt64(1);
+ std::move(scope2).WriteInt64(2);
+ },
+ "");
+
+ // A new call to AppendItem is not allowed before the previous result is
+ // consumed.
+ EXPECT_DEATH(
+ {
+ protozero::HeapBuffered<protos::pbzero::DebugAnnotation> message;
+ auto array = TracedValue::CreateForTest(message.get()).WriteArray();
+ auto scope1 = array.AppendItem();
+ auto scope2 = array.AppendItem();
+ std::move(scope1).WriteInt64(1);
+ std::move(scope2).WriteInt64(2);
+ },
+ "");
+
+ // Writing to parent scope is not allowed.
+ EXPECT_DEATH(
+ {
+ protozero::HeapBuffered<protos::pbzero::DebugAnnotation> message;
+ auto outer_dict =
+ TracedValue::CreateForTest(message.get()).WriteDictionary();
+ {
+ auto inner_dict = outer_dict.AddDictionary("inner");
+ outer_dict.AddItem("key").WriteString("value");
+ }
+ },
+ "");
+}
+#endif // PERFETTO_DCHECK_IS_ON()
+
} // namespace perfetto