Open
Conversation
Author
|
hey @vanhauser-thc, we a ready for a review here, pls |
There was a problem hiding this comment.
Pull request overview
This PR updates the CoreSight (ETMv4) trace decoder to support additional packet formats and 32-bit ARM binaries, and adds an optional instruction-flow export for Lighthouse-compatible coverage exploration.
Changes:
- Added decoding support for additional address packet types (IS1 variants, 32-bit address+context, exact match address packets) and updated address-register tracking.
- Added optional instruction-flow collection and export to
coverage.txtinmodule+offsetformat (gated byINSN_SAVE). - Added ARM32 (A32) disassembly mode selection (gated by
ARM32) and updated branch handling.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/processor.cpp | Enables instruction-flow saving via env var; passes binary/module name into MemoryImage. |
| src/process.cpp | Adds global instruction-flow buffer + export; expands packet/state handling to new address packet types. |
| src/libcsdec.cpp | Extends C API init to pass image paths into MemoryImage; enables instruction-flow saving via env var. |
| src/disassembler.cpp | Adds ARM32 mode support; logs instruction flow during disassembly; adjusts branch metadata. |
| src/decoder.cpp | Adds decoding for new address packet headers/types; implements exact-match address packets; tracks multiple address registers. |
| src/common.cpp | Extends MemoryImage to store a binary/module name. |
| include/common.hpp | Adds binary_name to MemoryImage and updates constructors. |
| include/decoder.hpp | Adds new packet types and address-register storage/APIs to Decoder. |
| include/libcsdec.h | Extends libcsdec_memory_image to include a path field used as module name. |
| HOWTO.md | Documents INSN_SAVE=1 for exporting coverage.txt. |
Comments suppressed due to low confidence (2)
src/process.cpp:555
PathProcessexception handling only transitions onETM4_PKT_I_ADDR_L_64IS{0,1}. If the trace emits a short address, exact-match address, 32-bit long address, or an address-with-context packet in the exception sequence, the state machine will remain stuck inEXCEPTION_ADDR1/2. Consider broadening these checks to accept the same set of supported address packet types as the mainProcessdecoder (including the new 32-bit/context/exact-match packets).
case DecodeState::EXCEPTION_ADDR1: {
if (packet.type == PacketType::ETM4_PKT_I_ADDR_L_64IS0 || packet.type == PacketType::ETM4_PKT_I_ADDR_L_64IS1) {
this->decoder.state = DecodeState::EXCEPTION_ADDR2;
}
break;
}
case DecodeState::EXCEPTION_ADDR2: {
if (packet.type == PacketType::ETM4_PKT_I_ADDR_L_64IS0 || packet.type == PacketType::ETM4_PKT_I_ADDR_L_64IS1) {
this->decoder.state = DecodeState::TRACE;
}
break;
}
src/process.cpp:31
insn_flowis a global that accumulates entries across decoding sessions, butProcess::reset()doesn't clear it. If the same process instance is reused (e.g., via repeatedlibcsdec_reset_edgecalls),coverage.txtwill contain stale data from previous runs and memory usage will grow unbounded. Clearinsn_flowduring reset (or at the start/end ofrun/final).
std::vector<std::pair<std::string, uint64_t>> insn_flow;
bool need_save_insn_flow = false;
void Process::reset(std::vector<MemoryMap> &&memory_maps,
const std::uint8_t target_trace_id) {
this->data.bitmap.reset();
this->deformatter.reset(target_trace_id);
this->decoder.reset();
this->state.reset(std::move(memory_maps));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+18
to
20
| extern std::vector<std::pair<std::string, uint64_t>> insn_flow; | ||
| extern bool need_save_insn_flow; | ||
| cs_insn *disassembleNextBranchInsn(const csh *handle, |
Comment on lines
161
to
+170
| void Decoder::reset() { | ||
| this->trace_data = std::vector<std::uint8_t>(); | ||
| this->trace_data_offset = 0; | ||
| this->state = DecodeState::START; | ||
| } | ||
|
|
||
| void Decoder::update_address_regs(uint64_t address){ | ||
| std::rotate(this->address_regs.rbegin(), this->address_regs.rbegin() + 1, this->address_regs.rend()); | ||
| this->address_regs[0] = address; | ||
| } |
Comment on lines
59
to
65
| std::unique_ptr<Process> process = std::make_unique<Process>( | ||
| std::move(memory_images), | ||
| Bitmap(reinterpret_cast<std::uint8_t *>(bitmap_addr), | ||
| static_cast<std::size_t>(bitmap_size)), | ||
| Cache()); | ||
|
|
||
| need_save_insn_flow = std::getenv("INSN_SAVE"); | ||
| // Release ownership and pass it to the C API side. |
Comment on lines
92
to
96
| for (int id = 0; id < binary_file_num; ++id) { | ||
| const std::string path = argv[4 + id * 3]; | ||
| std::vector<std::uint8_t> data = readBinaryFile(path); | ||
|
|
||
| memory_images.emplace_back(MemoryImage(std::move(data), (std::size_t)id)); | ||
| memory_images.emplace_back(MemoryImage(std::move(data), (std::size_t)id, basename(argv[4 + id * 3]))); | ||
| } |
Comment on lines
+518
to
+521
| // Header is correct, but packet size is incomplete. | ||
| if (rest_data_size < 10) { | ||
| return Packet{PacketType::PKT_INCOMPLETE, rest_data_size, 0, 0, 0}; | ||
| } |
Comment on lines
24
to
28
| struct libcsdec_memory_image { | ||
| void *data; /**< Binary data of the memory image. */ | ||
| char path[PATH_MAX]; | ||
| size_t size; /**< Size of the memory image. */ | ||
| }; |
Comment on lines
47
to
52
| usage(argv[0]); | ||
| std::exit(EXIT_FAILURE); | ||
| } | ||
|
|
||
| need_save_insn_flow = std::getenv("INSN_SAVE"); | ||
| const std::string trace_data_filename = argv[1]; | ||
| const std::uint8_t trace_id = std::stol(argv[2], nullptr, 16); |
Comment on lines
+43
to
+48
| std::ios_base::fmtflags f(std::cout.flags()); | ||
| for (auto el : insn_flow) { | ||
| CoverageFile << el.first << "+" << std::hex << el.second << std::endl; | ||
| } | ||
| std::cout.flags(f); | ||
| CoverageFile.close(); |
Comment on lines
+642
to
+657
| Packet Decoder::decodeExactMatchAddressPacket(){ | ||
| std::uint8_t header = this->trace_data[this->trace_data_offset]; | ||
| Packet packet; | ||
| // The QE field indicates the queue entry that contains the exact match | ||
| switch(header & 0b11){ | ||
| case 0b00: | ||
| packet = {PacketType::ETM4_ADDR_MATCH, 1, 0, 0, address_regs[0]}; | ||
| break; | ||
| case 0b01: | ||
| packet = {PacketType::ETM4_ADDR_MATCH, 1, 0, 0, address_regs[1]}; | ||
| break; | ||
| case 0b10: | ||
| packet = {PacketType::ETM4_ADDR_MATCH, 1, 0, 0, address_regs[2]}; | ||
| break; | ||
| } | ||
| return packet; |
Comment on lines
+158
to
+159
| //(type == BranchType::DIRECT_BRANCH) ? offset + insn->size : 0; | ||
Member
|
as I am clueless about this software I activated copilot :-) |
Author
|
Fixed the address packet size check (Address with Context 32-bit IS0/IS1 Long). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added/Fixed:
Exact Match Address packetand the decoder now is able to update the three address registers when decoding the address-related packets;Module + Offset (modoff)format to a file. This format is compatible with the lighthouse for code coverage exploration;