Skip to content

Use thiserror for humility-arch-arm #676

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
mkeeter wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use thiserror for humility-arch-arm #676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions humility-arch-arm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ version = "0.1.0"
edition.workspace = true

[dependencies]
anyhow.workspace = true
capstone.workspace = true
num-traits.workspace = true
num-derive.workspace = true
num-traits.workspace = true
thiserror.workspace = true
17 changes: 12 additions & 5 deletions humility-arch-arm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use anyhow::{Result, bail};
use num_derive::{FromPrimitive, ToPrimitive};
use num_traits::ToPrimitive;
use std::collections::{BTreeMap, HashMap};
Expand Down Expand Up @@ -211,25 +210,33 @@ fn instr_operands(cs: &Capstone, instr: &capstone::Insn) -> Vec<ARMRegister> {
rval
}

#[derive(Debug, thiserror::Error)]
Copy link
Copy Markdown

@jamesmunns jamesmunns May 22, 2026

Choose a reason for hiding this comment

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

Oh, we might also want to hit some common derives here as well, PartialEq, Clone, and Hash are usually big ones.

Clone is just generally useful, PartialEq allows for easier asserting (incl in tests), Hash is also one of those "eventually someone wants to stick it in a map" kind of things.

pub enum InstructionError {
Copy link
Copy Markdown

@jamesmunns jamesmunns May 22, 2026

Choose a reason for hiding this comment

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

Do we want to make this #[non_exhaustive] to avoid semver breakage of humility-as-a-lib?

This makes things a little tedious in some places, but we might want to start caring about the impact of semver breaking changes if we expect to start having external consumers.

#[error("multiple source registers")]
MultipleSourceRegisters,
Copy link
Copy Markdown

@jamesmunns jamesmunns May 22, 2026

Choose a reason for hiding this comment

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

Might be worth including doc comments on the enum, and on each variant, at least answering "what does this error mean, and what should a user do about it if they get it". If I got these errors, I'd probably have to go ping you in chat to figure out what I was doing wrong.

If we expect users to start handling these errors (instead of just bailing), we should probably give them some useful feedback!

#[error("multiple target registers")]
MultipleTargetRegisters,
}

fn instr_source_target(
cs: &Capstone,
instr: &capstone::Insn,
) -> Result<(Option<ARMRegister>, Option<ARMRegister>)> {
) -> Result<(Option<ARMRegister>, Option<ARMRegister>), InstructionError> {
let detail = cs.insn_detail(instr).unwrap();

let mut source: Option<ARMRegister> = None;
let mut target: Option<ARMRegister> = None;

for op in detail.regs_read() {
if source.is_some() {
bail!("multiple source registers");
return Err(InstructionError::MultipleSourceRegisters);
}
source = Some((*op).into());
}

for op in detail.regs_write() {
if target.is_some() {
bail!("multiple target registers");
return Err(InstructionError::MultipleTargetRegisters);
}
target = Some((*op).into());
}
Expand All @@ -252,7 +259,7 @@ fn instr_source_target(
pub fn presyscall_pushes(
cs: &Capstone,
instrs: &[capstone::Insn],
) -> Result<Vec<ARMRegister>> {
) -> Result<Vec<ARMRegister>, InstructionError> {
const ARM_INSN_PUSH: u32 = arch::arm::ArmInsn::ARM_INS_PUSH as u32;
const ARM_INSN_MOV: u32 = arch::arm::ArmInsn::ARM_INS_MOV as u32;
const ARM_INSN_POP: u32 = arch::arm::ArmInsn::ARM_INS_POP as u32;
Expand Down
Loading