From ae133cdbd960f8be4339a1d990239dd1954e5148 Mon Sep 17 00:00:00 2001 From: jbassett Date: Mon, 15 Jun 2026 14:10:10 -0700 Subject: [PATCH 1/4] Inline usages of deprecated StringIdForMigration PiperOrigin-RevId: 932659383 --- src/perf_data_converter.cc | 48 +++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/perf_data_converter.cc b/src/perf_data_converter.cc index c124ff1..ee39550 100644 --- a/src/perf_data_converter.cc +++ b/src/perf_data_converter.cc @@ -536,11 +536,11 @@ ProfileBuilder* PerfDataConverter::GetOrCreateBuilder( } auto sample_type = profile->add_sample_type(); sample_type->set_type(UTF8StringId(event_name + "sample", builder)); - sample_type->set_unit(builder->StringIdForMigration("count")); + sample_type->set_unit(builder->StringId("count")); sample_type = profile->add_sample_type(); last_index = UTF8StringId(event_name + "event", builder); sample_type->set_type(last_index); - sample_type->set_unit(builder->StringIdForMigration("count")); + sample_type->set_unit(builder->StringId("count")); } DCHECK_NE(last_index, 0); profile->set_default_sample_type(last_index); @@ -660,22 +660,22 @@ void PerfDataConverter::AddOrUpdateSample( // Emit any requested labels. if (IncludePidLabels() && context.sample.has_pid()) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(PidLabelKey)); + label->set_key(builder->StringId(PidLabelKey)); label->set_num(static_cast(context.sample.pid())); } if (IncludeTidLabels() && context.sample.has_tid()) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(TidLabelKey)); + label->set_key(builder->StringId(TidLabelKey)); label->set_num(static_cast(context.sample.tid())); } if (IncludeCommLabels() && sample_key.comm != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(CommLabelKey)); + label->set_key(builder->StringId(CommLabelKey)); label->set_str(sample_key.comm); } if (IncludeTimestampNsLabels() && context.sample.has_sample_time_ns()) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(TimestampNsLabelKey)); + label->set_key(builder->StringId(TimestampNsLabelKey)); int64_t timestamp_ns_as_int64 = static_cast(context.sample.sample_time_ns()); label->set_num(timestamp_ns_as_int64); @@ -683,77 +683,77 @@ void PerfDataConverter::AddOrUpdateSample( if (IncludeExecutionModeLabels() && sample_key.exec_mode != quipper::AddressContext::kUnknown) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(ExecutionModeLabelKey)); + label->set_key(builder->StringId(ExecutionModeLabelKey)); label->set_str(builder->StringId(ExecModeString(sample_key.exec_mode))); } if (IncludeThreadTypeLabels() && sample_key.thread_type != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(ThreadTypeLabelKey)); + label->set_key(builder->StringId(ThreadTypeLabelKey)); label->set_str(sample_key.thread_type); } if (IncludeThreadCommLabels() && sample_key.thread_comm != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(ThreadCommLabelKey)); + label->set_key(builder->StringId(ThreadCommLabelKey)); label->set_str(sample_key.thread_comm); } if (IncludeCgroupLabels() && sample_key.cgroup != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(CgroupLabelKey)); + label->set_key(builder->StringId(CgroupLabelKey)); label->set_str(sample_key.cgroup); } if (IncludeCodePageSizeLabels() && sample_key.code_page_size != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(CodePageSizeLabelKey)); + label->set_key(builder->StringId(CodePageSizeLabelKey)); label->set_num(sample_key.code_page_size); } if (IncludeDataPageSizeLabels() && sample_key.data_page_size != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(DataPageSizeLabelKey)); + label->set_key(builder->StringId(DataPageSizeLabelKey)); label->set_num(sample_key.data_page_size); } if (IncludeCpuLabels() && context.sample.has_cpu()) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(CpuLabelKey)); + label->set_key(builder->StringId(CpuLabelKey)); label->set_num(static_cast(context.sample.cpu())); - label->set_num_unit(builder->StringIdForMigration("cpu")); + label->set_num_unit(builder->StringId("cpu")); } if (IncludeCacheLatencyLabel() && sample_key.cache_latency != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(CacheLatencyLabelKey)); + label->set_key(builder->StringId(CacheLatencyLabelKey)); label->set_num(sample_key.cache_latency); - label->set_num_unit(builder->StringIdForMigration("cycles")); + label->set_num_unit(builder->StringId("cycles")); } if (IncludeDataSrcLabels()) { if (sample_key.data_src != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(DataSrcLabelKey)); + label->set_key(builder->StringId(DataSrcLabelKey)); label->set_str(sample_key.data_src); } if (sample_key.snoop_status != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(SnoopStatusLabelKey)); + label->set_key(builder->StringId(SnoopStatusLabelKey)); label->set_str(sample_key.snoop_status); } } if (IncludeTotalLatencyLabels() && sample_key.total_latency != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(TotalLatencyLabelKey)); + label->set_key(builder->StringId(TotalLatencyLabelKey)); label->set_num(sample_key.total_latency); - label->set_num_unit(builder->StringIdForMigration("cycles")); + label->set_num_unit(builder->StringId("cycles")); } if (IncludeIssueLatencyLabels() && sample_key.issue_latency != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(IssueLatencyLabelKey)); + label->set_key(builder->StringId(IssueLatencyLabelKey)); label->set_num(sample_key.issue_latency); - label->set_num_unit(builder->StringIdForMigration("cycles")); + label->set_num_unit(builder->StringId("cycles")); } if (IncludeTranslationLatencyLabels() && sample_key.translation_latency != 0) { auto* label = sample->add_label(); - label->set_key(builder->StringIdForMigration(TranslationLatencyLabelKey)); + label->set_key(builder->StringId(TranslationLatencyLabelKey)); label->set_num(sample_key.translation_latency); - label->set_num_unit(builder->StringIdForMigration("cycles")); + label->set_num_unit(builder->StringId("cycles")); } // Two values per collected event: the first is sample counts, the second is From 943186d08202f2dc7c888b74473c82482f1cebd0 Mon Sep 17 00:00:00 2001 From: jbassett Date: Thu, 18 Jun 2026 11:22:34 -0700 Subject: [PATCH 2/4] Delete deprecated StringIdForMigration from Builder. PiperOrigin-RevId: 934459293 --- src/builder.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/builder.h b/src/builder.h index 27d5a06..1d7c477 100644 --- a/src/builder.h +++ b/src/builder.h @@ -49,10 +49,6 @@ class Builder { // Adds a string to the profile string table if not already present. // Returns a unique integer id for this string. int64_t StringId(absl::string_view str); - [[deprecated("Use StringId instead.")]] ABSL_REFACTOR_INLINE int64_t - StringIdForMigration(absl::string_view str) { - return StringId(str); - } // Adds a function with these attributes to the profile function // table, if not already present. Returns a unique integer id for From fe196a757f750afe0b83e25fc3d9de0030f46cc5 Mon Sep 17 00:00:00 2001 From: gmx Date: Wed, 29 Jul 2026 15:38:25 -0700 Subject: [PATCH 3/4] Modify ARM SPE decoder to support segmented/non-contiguous string_view buffers. ARM SPE traces are parsed using `ArmSpeDecoder`. Currently, it takes a single contiguous `std::string_view` as input, which assumes the entire trace buffer is contiguous in memory. This change modifies `ArmSpeDecoder` to take a non-contiguous buffer (pointer to multiple `std::string_view` segments and size) as input by using C++17 array-reference template deduction constructors. A helper class `SegmentedBuffer` is implemented to abstract multiple non-contiguous `string_view` buffers as a single contiguous stream. It provides indexing (`operator[]`), safe multicycle chunk copies (`ReadBytes`), and pointer resolution (`GetPointer`) for alignment calculation. With these changes, `ArmSpeDecoder` handles non-contiguous inputs seamlessly across boundary limits of individual segments. This is backwards compatible with single `string_view` traces, which are implicitly wrapped. The decoder uses view-type containers that don't own the input memory and the decoder doesn't make a copy of the data to avoid heap allocations and make it compatible with interrupt handlers. PiperOrigin-RevId: 956124896 --- src/quipper/BUILD | 1 - src/quipper/arm_spe_decoder.cc | 25 +++++-- src/quipper/arm_spe_decoder.h | 101 +++++++++++++++++++++++++++- src/quipper/arm_spe_decoder_test.cc | 52 ++++++++++++++ 4 files changed, 171 insertions(+), 8 deletions(-) diff --git a/src/quipper/BUILD b/src/quipper/BUILD index 3cdde04..cfc5924 100644 --- a/src/quipper/BUILD +++ b/src/quipper/BUILD @@ -514,7 +514,6 @@ cc_test( ":compat_gunit", ":test_runner", ":test_utils", - "@com_google_googletest//:gtest_main", ], ) diff --git a/src/quipper/arm_spe_decoder.cc b/src/quipper/arm_spe_decoder.cc index 9fa1cc2..19852c0 100644 --- a/src/quipper/arm_spe_decoder.cc +++ b/src/quipper/arm_spe_decoder.cc @@ -140,8 +140,9 @@ bool ArmSpeDecoder::NextRecord(struct Record* ret_record) { return false; } - p.size = - alignment - (((uintptr_t)(buf_.data() + buf_i_)) & (alignment - 1)); + p.size = alignment - + ((reinterpret_cast(buf_.GetPointer(buf_i_))) & + (alignment - 1)); continue; } } @@ -394,26 +395,38 @@ bool ArmSpeDecoder::SetPayloadAndSize(struct Packet* p) { switch (payload_size) { case 1: { uint8_t payload; - memcpy(&payload, buf_.data() + pos, sizeof(payload)); + if (!buf_.ReadBytes(pos, sizeof(payload), &payload)) { + LOG(ERROR) << "Error reading payload of size 1"; + return false; + } // No need to swap when there is only one byte. p->payload = payload; break; } case 2: { uint16_t payload; - memcpy(&payload, buf_.data() + pos, sizeof(payload)); + if (!buf_.ReadBytes(pos, sizeof(payload), &payload)) { + LOG(ERROR) << "Error reading payload of size 2"; + return false; + } p->payload = MaybeSwap(payload, is_cross_endian_); break; } case 4: { uint32_t payload; - memcpy(&payload, buf_.data() + pos, sizeof(payload)); + if (!buf_.ReadBytes(pos, sizeof(payload), &payload)) { + LOG(ERROR) << "Error reading payload of size 4"; + return false; + } p->payload = MaybeSwap(payload, is_cross_endian_); break; } case 8: { uint64_t payload; - memcpy(&payload, buf_.data() + pos, sizeof(payload)); + if (!buf_.ReadBytes(pos, sizeof(payload), &payload)) { + LOG(ERROR) << "Error reading payload of size 8"; + return false; + } p->payload = MaybeSwap(payload, is_cross_endian_); break; } diff --git a/src/quipper/arm_spe_decoder.h b/src/quipper/arm_spe_decoder.h index e8a2af6..69b122e 100644 --- a/src/quipper/arm_spe_decoder.h +++ b/src/quipper/arm_spe_decoder.h @@ -3,11 +3,99 @@ #include #include +#include #include #include namespace quipper { +// A helper class to treat multiple string_views as a contiguous buffer. +// The class is not copyable or movable to avoid accidental use of the default +// copy/move implementations that may lead to dangling pointers. These operators +// can be explicitly defined later if the SegmentedBuffer needs to be copied or +// moved. +class SegmentedBuffer { + public: + SegmentedBuffer() : buffers_(nullptr), num_buffers_(0), total_size_(0) {} + explicit SegmentedBuffer(std::string_view buf) + : single_buf_(buf), + buffers_(&single_buf_), + num_buffers_(1), + total_size_(buf.size()) {} + + template + explicit SegmentedBuffer(const std::string_view (&buffers)[N]) + : buffers_(buffers), num_buffers_(N), total_size_(0) { + for (size_t i = 0; i < N; ++i) { + total_size_ += buffers[i].size(); + } + } + + SegmentedBuffer(const SegmentedBuffer&) = delete; + SegmentedBuffer& operator=(const SegmentedBuffer&) = delete; + SegmentedBuffer(SegmentedBuffer&&) = delete; + SegmentedBuffer& operator=(SegmentedBuffer&&) = delete; + + size_t size() const { return total_size_; } + + uint8_t operator[](size_t global_offset) const { + size_t offset = global_offset; + for (size_t i = 0; i < num_buffers_; ++i) { + const auto& buf = buffers_[i]; + if (offset < buf.size()) { + return static_cast(buf[offset]); + } + offset -= buf.size(); + } + return 0; + } + + const char* GetPointer(size_t global_offset) const { + size_t offset = global_offset; + for (size_t i = 0; i < num_buffers_; ++i) { + const auto& buf = buffers_[i]; + if (offset < buf.size()) { + return buf.data() + offset; + } + offset -= buf.size(); + } + return nullptr; + } + + bool ReadBytes(size_t global_offset, size_t num_bytes, void* dest) const { + if (global_offset + num_bytes > total_size_) { + return false; + } + char* d = static_cast(dest); + size_t bytes_to_read = num_bytes; + size_t offset = global_offset; + for (size_t i = 0; i < num_buffers_; ++i) { + const auto& buf = buffers_[i]; + if (bytes_to_read == 0) break; + if (offset < buf.size()) { + size_t chunk_size = (bytes_to_read < buf.size() - offset) + ? bytes_to_read + : (buf.size() - offset); + std::memcpy(d, buf.data() + offset, chunk_size); + d += chunk_size; + bytes_to_read -= chunk_size; + offset = 0; + } else { + offset -= buf.size(); + } + } + return bytes_to_read == 0; + } + + private: + // Store the single string_view buffer separately to avoid heap allocations + // from creating a container locally. + std::string_view single_buf_; + const std::string_view* buffers_; + size_t num_buffers_; + size_t total_size_; +}; + // Decode SPE records from the given binary trace buffer. The decoder is // implemented according to the Arm Architecture Reference Manual for A-profile // architecture (ARM DDI 0487I.a ID081822). @@ -114,8 +202,19 @@ class ArmSpeDecoder { std::optional source; }; + // Constructors take 'view' overlays over the input data and the decoder + // doesn't own any of the memory during its lifetime to avoid heap + // allocations. The caller must ensure the lifetime of the buffers exceeds + // the lifetime of the decoder. ArmSpeDecoder(std::string_view buf, bool is_cross_endian); + template + ArmSpeDecoder(const std::string_view (&bufs)[N], bool is_cross_endian) + : buf_(bufs), + buf_i_(0), + seen_pk_idx_mask_(0), + is_cross_endian_(is_cross_endian) {} + // Sets fields of the given record to the next record parsed from the // previously given SPE trace. It will return false if it encounters invalid // data or reaching the end of the trace. @@ -153,7 +252,7 @@ class ArmSpeDecoder { uint8_t GetCorrectHeader(const ArmSpeDecoder::Packet& p); // The SPE trace buffer. Note that this class doesn't own the buffer. - std::string_view buf_; + SegmentedBuffer buf_; // Current position of the SPE trace. size_t buf_i_; diff --git a/src/quipper/arm_spe_decoder_test.cc b/src/quipper/arm_spe_decoder_test.cc index 3a66493..103bfcb 100644 --- a/src/quipper/arm_spe_decoder_test.cc +++ b/src/quipper/arm_spe_decoder_test.cc @@ -1,5 +1,6 @@ #include "arm_spe_decoder.h" +#include #include #include @@ -172,4 +173,55 @@ TEST(ArmSpeDecoderTest, ReturnFalseUponInvalidData) { EXPECT_FALSE(decoder.NextRecord(&tmp)); } +TEST(ArmSpeDecoderTest, CorrectlyParseSegmentedTraceAllSplitPoints) { + std::string trace = GenerateBinaryTrace(SampleSPEPackets); + // Loop over every possible split point to verify that non-contiguous buffers + // split at any position parse exactly the same as a single contiguous buffer. + // Use substr to generate separate parts that are not contiguous in memory. + for (size_t split_point = 1; split_point < trace.size(); ++split_point) { + std::string part1 = trace.substr(0, split_point); + std::string part2 = trace.substr(split_point, trace.size() - split_point); + + const std::string_view parts[] = {part1, part2}; + ArmSpeDecoder decoder(parts, false); + ArmSpeDecoder::Record record[2]; + ArmSpeDecoder::Record tmp; + + EXPECT_TRUE(decoder.NextRecord(&record[0])) + << "Failed at split point " << split_point; + EXPECT_TRUE(decoder.NextRecord(&record[1])) + << "Failed at split point " << split_point; + EXPECT_FALSE(decoder.NextRecord(&tmp)) + << "Failed at split point " << split_point; + + EXPECT_TRUE(CheckEqual( + record[0], + ArmSpeDecoder::Record{ + .event = {.retired = true, .l1d_access = true, .tlb_access = true}, + .op = {.is_ldst = true, .ldst = {.ld = true, .gp_reg = true}}, + .total_lat = 12, + .issue_lat = 4, + .translation_lat = 1, + .ip = {.addr = 0xffffba66eda1c2d0, .el = 2, .ns = 1}, + .virt = {.addr = 0xffff0e3703096b28}, + .timestamp = 44731163950, + .context = {.id = 0x5f80, .el2 = true}, + .source = {0}, + })); + + EXPECT_TRUE(CheckEqual( + record[1], + ArmSpeDecoder::Record{ + .event = {.retired = true, .cond_not_taken = true}, + .op = {.is_br_eret = true, .br_eret = {.br_cond = true}}, + .total_lat = 17, + .issue_lat = 16, + .ip = {.addr = 0xffffba66edefb0e0, .el = 2, .ns = 1}, + .tgt_br_ip = {.addr = 0xffffba66edefb0e4, .el = 2, .ns = 1}, + .timestamp = 44731164045, + .context = {.id = 0xe, .el2 = true}, + })); + } +} + } // namespace quipper From f7ceac8c981ad511ee3d2a0bcca977a811a1a2cf Mon Sep 17 00:00:00 2001 From: gmx Date: Thu, 6 Aug 2026 16:20:15 -0700 Subject: [PATCH 4/4] Split ByteSwap and MaybeSwap from binary_data_utils into byte_swap_utils. PiperOrigin-RevId: 960560835 --- src/quipper/BUILD | 30 ++++++++++++++++-- src/quipper/BUILD.gn | 2 ++ src/quipper/arm_spe_decoder.cc | 2 +- src/quipper/binary_data_utils.cc | 38 ---------------------- src/quipper/binary_data_utils.h | 13 -------- src/quipper/byte_swap_utils.cc | 49 +++++++++++++++++++++++++++++ src/quipper/byte_swap_utils.h | 27 ++++++++++++++++ src/quipper/byte_swap_utils_test.cc | 41 ++++++++++++++++++++++++ src/quipper/data_reader.cc | 2 -- src/quipper/data_reader.h | 4 ++- src/quipper/dso.cc | 1 + src/quipper/dso.h | 2 ++ src/quipper/perf_reader.cc | 1 + src/quipper/test_perf_data.h | 2 +- 14 files changed, 155 insertions(+), 59 deletions(-) create mode 100644 src/quipper/byte_swap_utils.cc create mode 100644 src/quipper/byte_swap_utils.h create mode 100644 src/quipper/byte_swap_utils_test.cc diff --git a/src/quipper/BUILD b/src/quipper/BUILD index cfc5924..98be117 100644 --- a/src/quipper/BUILD +++ b/src/quipper/BUILD @@ -118,8 +118,10 @@ cc_library( hdrs = ["dso.h"], visibility = ["//src:__subpackages__"], deps = [ + ":binary_data_utils", ":data_reader", ":file_reader", + ":kernel", ":base", ], linkopts = ["-lelf"], @@ -210,6 +212,7 @@ cc_library( ":binary_data_utils", ":buffer_reader", ":buffer_writer", + ":byte_swap_utils", ":compat", ":file_reader", ":file_utils", @@ -218,6 +221,7 @@ cc_library( ":perf_data_utils", ":perf_serializer", ":sample_info_reader", + ":string_utils", ":base", ], ) @@ -261,6 +265,16 @@ cc_library( ], ) +cc_library( + name = "byte_swap_utils", + srcs = ["byte_swap_utils.cc"], + hdrs = ["byte_swap_utils.h"], + visibility = ["//visibility:public"], + deps = [ + ":base", + ], +) + cc_library( name = "binary_data_utils", srcs = ["binary_data_utils.cc"], @@ -309,7 +323,7 @@ cc_library( srcs = ["data_reader.cc"], hdrs = ["data_reader.h"], deps = [ - ":binary_data_utils", + ":byte_swap_utils", ":base", ], ) @@ -437,7 +451,7 @@ cc_library( ], visibility = ["//src:__subpackages__"], deps = [ - ":binary_data_utils", + ":byte_swap_utils", ":compat", ":compat_gunit", ":file_reader", @@ -502,7 +516,7 @@ cc_library( hdrs = ["arm_spe_decoder.h"], visibility = ["//visibility:public"], deps = [ - ":binary_data_utils", + ":byte_swap_utils", ], ) @@ -571,6 +585,16 @@ cc_test( ], ) +cc_test( + name = "byte_swap_utils_test", + srcs = ["byte_swap_utils_test.cc"], + deps = [ + ":byte_swap_utils", + ":compat_gunit", + ":test_runner", + ], +) + cc_test( name = "binary_data_utils_test", srcs = ["binary_data_utils_test.cc"], diff --git a/src/quipper/BUILD.gn b/src/quipper/BUILD.gn index 5108143..47bf4ab 100644 --- a/src/quipper/BUILD.gn +++ b/src/quipper/BUILD.gn @@ -41,6 +41,7 @@ static_library("common") { "binary_data_utils.cc", "buffer_reader.cc", "buffer_writer.cc", + "byte_swap_utils.cc", "compat/log_level.cc", "data_reader.cc", "data_writer.cc", @@ -180,6 +181,7 @@ if (use.test) { "binary_data_utils_test.cc", "buffer_reader_test.cc", "buffer_writer_test.cc", + "byte_swap_utils_test.cc", "dso_test.cc", "file_reader_test.cc", "perf_buildid_test.cc", diff --git a/src/quipper/arm_spe_decoder.cc b/src/quipper/arm_spe_decoder.cc index 19852c0..bae382f 100644 --- a/src/quipper/arm_spe_decoder.cc +++ b/src/quipper/arm_spe_decoder.cc @@ -6,7 +6,7 @@ #include #include "base/logging.h" -#include "binary_data_utils.h" +#include "byte_swap_utils.h" namespace quipper { diff --git a/src/quipper/binary_data_utils.cc b/src/quipper/binary_data_utils.cc index aa2b981..a7755e3 100644 --- a/src/quipper/binary_data_utils.cc +++ b/src/quipper/binary_data_utils.cc @@ -14,8 +14,6 @@ #include #include -#include "base/logging.h" - namespace { // Number of hex digits in a byte. @@ -84,40 +82,4 @@ bool HexStringToRawData(const std::string& str, u8* array, size_t length) { return true; } -template -void ByteSwap(T* input) { - switch (sizeof(T)) { - case sizeof(uint8_t): - LOG(WARNING) << "Attempting to byte swap on a single byte."; - break; - case sizeof(uint16_t): - *input = bswap_16(*input); - break; - case sizeof(uint32_t): - *input = bswap_32(*input); - break; - case sizeof(uint64_t): - *input = bswap_64(*input); - break; - default: - LOG(FATAL) << "Invalid size for byte swap: " << sizeof(T) << " bytes"; - break; - } -} - -template void ByteSwap(signed char*); -template void ByteSwap(unsigned char*); -template void ByteSwap(int*); -template void ByteSwap(unsigned int*); -// For portability we make explicit specialization on all integer types -// instead of using exact-width types. - -template void ByteSwap(short*); -template void ByteSwap(unsigned short*); -template void ByteSwap(long*); -template void ByteSwap(unsigned long*); -template void ByteSwap(long long*); -template void ByteSwap(unsigned long long*); - - } // namespace quipper diff --git a/src/quipper/binary_data_utils.h b/src/quipper/binary_data_utils.h index 8a0520a..6e235b9 100644 --- a/src/quipper/binary_data_utils.h +++ b/src/quipper/binary_data_utils.h @@ -5,7 +5,6 @@ #ifndef CHROMIUMOS_WIDE_PROFILING_BINARY_DATA_UTILS_H_ #define CHROMIUMOS_WIDE_PROFILING_BINARY_DATA_UTILS_H_ -#include #include #include @@ -17,18 +16,6 @@ namespace quipper { -// Swaps the byte order of 16-bit, 32-bit, and 64-bit unsigned integers. -template -void ByteSwap(T* input); - -// Swaps byte order of |value| if the |swap| flag is set. This function is -// trivial but it avoids filling code with "if (swap) { ... } " statements. -template -T MaybeSwap(T value, bool swap) { - if (swap) ByteSwap(&value); - return value; -} - // Returns the number of bits in a numerical value. template size_t GetNumBits(const T& value) { diff --git a/src/quipper/byte_swap_utils.cc b/src/quipper/byte_swap_utils.cc new file mode 100644 index 0000000..6e70df9 --- /dev/null +++ b/src/quipper/byte_swap_utils.cc @@ -0,0 +1,49 @@ +// Copyright 2026 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "byte_swap_utils.h" + +#include + +#include "base/logging.h" + +namespace quipper { + +template +void ByteSwap(T* input) { + switch (sizeof(T)) { + case sizeof(uint8_t): + LOG(WARNING) << "Attempting to byte swap on a single byte."; + break; + case sizeof(uint16_t): + *input = bswap_16(*input); + break; + case sizeof(uint32_t): + *input = bswap_32(*input); + break; + case sizeof(uint64_t): + *input = bswap_64(*input); + break; + default: + LOG(FATAL) << "Invalid size for byte swap: " << sizeof(T) << " bytes"; + break; + } +} + +template void ByteSwap(signed char*); +template void ByteSwap(unsigned char*); +template void ByteSwap(int*); +template void ByteSwap(unsigned int*); +// For portability we make explicit specialization on all integer types +// instead of using exact-width types. + +template void ByteSwap(short*); +template void ByteSwap(unsigned short*); +template void ByteSwap(long*); +template void ByteSwap(unsigned long*); +template void ByteSwap(long long*); +template void ByteSwap(unsigned long long*); + + +} // namespace quipper diff --git a/src/quipper/byte_swap_utils.h b/src/quipper/byte_swap_utils.h new file mode 100644 index 0000000..516810e --- /dev/null +++ b/src/quipper/byte_swap_utils.h @@ -0,0 +1,27 @@ +// Copyright 2026 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROMIUMOS_WIDE_PROFILING_BYTE_SWAP_UTILS_H_ +#define CHROMIUMOS_WIDE_PROFILING_BYTE_SWAP_UTILS_H_ + +#include +#include + +namespace quipper { + +// Swaps the byte order of 16-bit, 32-bit, and 64-bit unsigned integers. +template +void ByteSwap(T* input); + +// Swaps byte order of |value| if the |swap| flag is set. This function is +// trivial but it avoids filling code with "if (swap) { ... } " statements. +template +T MaybeSwap(T value, bool swap) { + if (swap) ByteSwap(&value); + return value; +} + +} // namespace quipper + +#endif // CHROMIUMOS_WIDE_PROFILING_BYTE_SWAP_UTILS_H_ diff --git a/src/quipper/byte_swap_utils_test.cc b/src/quipper/byte_swap_utils_test.cc new file mode 100644 index 0000000..e698b92 --- /dev/null +++ b/src/quipper/byte_swap_utils_test.cc @@ -0,0 +1,41 @@ +// Copyright 2026 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "byte_swap_utils.h" + +#include + +#include "compat/test.h" + +namespace quipper { + +TEST(ByteSwapUtilsTest, ByteSwap16) { + uint16_t val = 0x1234; + ByteSwap(&val); + EXPECT_EQ(val, 0x3412); +} + +TEST(ByteSwapUtilsTest, ByteSwap32) { + uint32_t val = 0x12345678; + ByteSwap(&val); + EXPECT_EQ(val, 0x78563412); +} + +TEST(ByteSwapUtilsTest, ByteSwap64) { + uint64_t val = 0x123456789abcdef0ULL; + ByteSwap(&val); + EXPECT_EQ(val, 0xf0debc9a78563412ULL); +} + +TEST(ByteSwapUtilsTest, MaybeSwap) { + uint32_t val = 0x12345678; + + // With swap = false, value should remain the same. + EXPECT_EQ(MaybeSwap(val, false), 0x12345678); + + // With swap = true, value should be swapped. + EXPECT_EQ(MaybeSwap(val, true), 0x78563412); +} + +} // namespace quipper diff --git a/src/quipper/data_reader.cc b/src/quipper/data_reader.cc index c712539..faac5a4 100644 --- a/src/quipper/data_reader.cc +++ b/src/quipper/data_reader.cc @@ -8,8 +8,6 @@ #include "base/logging.h" -#include "binary_data_utils.h" - namespace quipper { bool DataReader::ReadDataString(const size_t size, std::string* dest) { diff --git a/src/quipper/data_reader.h b/src/quipper/data_reader.h index 5b63a60..3de542d 100644 --- a/src/quipper/data_reader.h +++ b/src/quipper/data_reader.h @@ -8,7 +8,9 @@ #include #include -#include "binary_data_utils.h" +#include + +#include "byte_swap_utils.h" namespace quipper { diff --git a/src/quipper/dso.cc b/src/quipper/dso.cc index eb0a4fb..d74e243 100644 --- a/src/quipper/dso.cc +++ b/src/quipper/dso.cc @@ -17,6 +17,7 @@ #include #include "base/logging.h" +#include "binary_data_utils.h" #include "file_reader.h" namespace quipper { diff --git a/src/quipper/dso.h b/src/quipper/dso.h index a67e119..c377e91 100644 --- a/src/quipper/dso.h +++ b/src/quipper/dso.h @@ -7,10 +7,12 @@ #include +#include #include #include #include "data_reader.h" +#include "kernel/perf_internals.h" namespace quipper { diff --git a/src/quipper/perf_reader.cc b/src/quipper/perf_reader.cc index c245ea8..7afaa5e 100644 --- a/src/quipper/perf_reader.cc +++ b/src/quipper/perf_reader.cc @@ -25,6 +25,7 @@ #include "binary_data_utils.h" #include "buffer_reader.h" #include "buffer_writer.h" +#include "byte_swap_utils.h" #include "compat/proto.h" #include "file_reader.h" #include "file_utils.h" diff --git a/src/quipper/test_perf_data.h b/src/quipper/test_perf_data.h index 1f0a0fb..3099c87 100644 --- a/src/quipper/test_perf_data.h +++ b/src/quipper/test_perf_data.h @@ -15,7 +15,7 @@ #include #include -#include "binary_data_utils.h" +#include "byte_swap_utils.h" #include "kernel/perf_event.h" #include "kernel/perf_internals.h"