Skip to content

Updated nyxstone.cpp #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
irishdev00 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Updated nyxstone.cpp #72

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d749a0d
Update nyxstone.cpp
irishdev00 Aug 21, 2024
98e7544
Refactor LLVM disassembly function for efficiency and readability
irishdev00 Nov 16, 2024
9ad7e8a
Merge branch 'emproof-com:main' into main
irishdev00 Nov 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 23 additions & 26 deletions src/nyxstone.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,21 +381,18 @@ tl::expected<void, std::string> Nyxstone::assemble_impl(const std::string& assem
tl::expected<void, std::string> Nyxstone::disassemble_impl(const std::vector<uint8_t>& bytes, uint64_t address,
size_t count, std::string* disassembly, std::vector<Instruction>* instructions) const
{
if (disassembly == nullptr && instructions == nullptr) {
if ((disassembly == nullptr && instructions == nullptr) || bytes.empty()) {
return {};
}

if (disassembly != nullptr) {
disassembly->clear();
}

if (instructions != nullptr) {
instructions->clear();
}

if (bytes.empty()) {
return {};
}

// Equip context with info objects and custom error handling
llvm::SmallString<128> error_msg;
llvm::MCContext context(
Expand All @@ -417,47 +414,47 @@ tl::expected<void, std::string> Nyxstone::disassemble_impl(const std::vector<uin
return tl::unexpected("Invalid architecture / LLVM target triple");
}

// Disassemble
const llvm::ArrayRef<u8> data(bytes.data(), bytes.size());
uint64_t pos = 0;
uint64_t insn_count = 0;
Comment on lines -422 to -423
Copy link
Collaborator

@stuxnot stuxnot Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave these two in seperate lines. We try to use this style of variable initialization as the single line initialization can easily lead to forgetting to assign a value to a variable.

while (true) {
// Disassemble instructions
llvm::ArrayRef<u8> data(bytes);
uint64_t pos = 0, insn_count = 0;
const bool disassemble_all = (count == 0);

// We exit either if we reached the end of the provided bytes, or if we have disassembled as many instructions
// as the user has requested
while (pos < data.size() && (disassemble_all || insn_count < count)) {
Copy link
Collaborator

@stuxnot stuxnot Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You re-added the conditions that you removed earlier from the end of this loop, i.e. lines 462-464 and 467-469. These conditions are redundant because of the conditions in the while loop. Please remove them.

// Decompose one instruction
llvm::MCInst insn;
uint64_t insn_size = 0;
auto res = disassembler->getInstruction(insn, insn_size, data.slice(pos), address + pos, llvm::nulls());
if (res == llvm::MCDisassembler::Fail || res == llvm::MCDisassembler::SoftFail || !error_msg.empty()) {

if (disassembler->getInstruction(insn, insn_size, data.slice(pos), address + pos, llvm::nulls()) != llvm::MCDisassembler::Success
|| !error_msg.empty()) {
std::stringstream error_stream;
error_stream << "Could not disassemble at position " << pos << " / address " << std::hex << address + pos;
error_stream << "Could not disassemble at position " << pos
<< " / address " << std::hex << address + pos;
if (!error_msg.empty()) {
error_stream << "(= " << error_msg.c_str() << " )";
error_stream << " (= " << error_msg.c_str() << ")";
Copy link
Collaborator

@stuxnot stuxnot Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason to touch the whitespaces in the error string (at least in this PR), especially since we are planning to rewrite the error handling anyway. Please revert this change.

}
return tl::unexpected(error_stream.str());
}

// Generate instruction disassembly text
std::string insn_str;
llvm::raw_string_ostream str_stream(insn_str);
instruction_printer->printInst(&insn,
/* Address */ address + pos,
/* Annot */ "", *subtarget_info, str_stream);

// left trim
instruction_printer->printInst(&insn, /* Address */ address + pos, /* Annot */ "", *subtarget_info, str_stream);
// Left trim
insn_str.erase(0, insn_str.find_first_not_of(" \t\n\r"));
// convert tabulators to spaces
// Convert tabulators to spaces
std::replace(insn_str.begin(), insn_str.end(), '\t', ' ');

// Add instruction to results
if (disassembly != nullptr) {
*disassembly += insn_str + "\n";
}

if (instructions != nullptr) {
Nyxstone::Instruction new_insn;
new_insn.address = address + pos;
new_insn.assembly = insn_str;
new_insn.bytes.reserve(insn_size);
std::copy(data.begin() + pos, data.begin() + pos + insn_size, std::back_inserter(new_insn.bytes));
instructions->push_back(new_insn);
Instruction new_insn{address + pos, insn_str, {}};
new_insn.bytes.assign(data.begin() + pos, data.begin() + pos + insn_size);
instructions->push_back(std::move(new_insn));
Comment on lines +455 to +457
Copy link
Collaborator

@stuxnot stuxnot Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are touching this code anyway, in my opinion doing

            auto insn_bytes = data.slice(pos, insn_size);
            Instruction new_insn { address + pos, insn_str, std::move(insn_bytes) };
            instructions->push_back(std::move(new_insn));

is a bit clearer.

}

// Abort after n instructions if requested
Expand Down