Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Implement --no-reset mode for live attach to a running device #414

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p

## [Unreleased]

- [#414] Add `--no-reset` to attach to/detach from live target
- [#410] Simplify canary
- [#405] Also enable merge queue for changelog enforcer
- [#404] Switch from bors to merge queue
Expand Down
2 changes: 1 addition & 1 deletion src/backtrace/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ impl Outcome {
Outcome::StackOverflow => log::error!("the program has overflowed its stack"),
Outcome::HardFault => log::error!("the program panicked"),
Outcome::Ok => log::info!("device halted without error"),
Outcome::CtrlC => log::info!("device halted by user"),
Outcome::CtrlC => log::info!("interrupted by user"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Outcome::CtrlC => log::info!("interrupted by user"),
Outcome::CtrlC => log::info!("device interrupted by user"),

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to the current message because it's the probe that was interrupted, not the device -- the device keeps running if you do --no-reset.

Copy link
Member

@Urhengulas Urhengulas Jun 23, 2023

Choose a reason for hiding this comment

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

Maybe we can actually have two different messages then?

Suggested change
Outcome::CtrlC => log::info!("interrupted by user"),
Outcome::CtrlC if opts.no_reset == true => log::info!("interrupted by user; program keeps running"),
Outcome::CtrlC if opts.no_reset == false => log::info!("device halted by by user"),

I am not sure if opts is in scope here.

Copy link
Member

Choose a reason for hiding this comment

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

It's not, but we can just add it as an argument:

fn log(&self, no_reset: bool)

}
}
}
Expand Down
13 changes: 12 additions & 1 deletion src/backtrace/unwind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,12 @@ pub fn target(core: &mut Core, elf: &Elf, target_info: &TargetInfo) -> Output {
// the stack was not corrupted.
let reset_fn_range = elf.reset_fn_range();
output.corrupted = !(reset_fn_range.contains(&pc) || reset_fn_range.is_empty());
if output.corrupted {
log::debug!(
"Frame (PC {pc:#010x}) is not within reset vector ({reset_fn_range:#010x?}) \
and links to itself, the stack is corrupted",
);
}
break;
}

Expand All @@ -125,6 +131,7 @@ pub fn target(core: &mut Core, elf: &Elf, target_info: &TargetInfo) -> Output {
{
stacked
} else {
log::debug!("Unreadable stacked exception registers, the stack is corrupted");
output.corrupted = true;
break;
};
Expand All @@ -143,9 +150,13 @@ pub fn target(core: &mut Core, elf: &Elf, target_info: &TargetInfo) -> Output {
break;
}

// if the SP is above the start of the stack, the stack is corrupt
match registers.get(registers::SP) {
Ok(advanced_sp) if advanced_sp > target_info.stack_start => {
log::debug!(
"SP ({advanced_sp:#010x}) above start of the stack ({:#010x}), \
the stack is corrupted",
target_info.stack_start
);
output.corrupted = true;
break;
}
Expand Down
4 changes: 4 additions & 0 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ pub struct Opts {
#[arg(long, env = "PROBE_RUN_PROBE")]
pub probe: Option<String>,

/// Whether to reset the target when attaching and detaching. Implies `--no-flash`.
#[arg(long)]
pub no_reset: bool,

/// Whether to shorten paths (e.g. to crates.io dependencies) in backtraces and defmt logs
#[arg(long)]
pub shorten_paths: bool,
Expand Down
4 changes: 3 additions & 1 deletion src/cortexm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,10 @@ pub fn subroutine_eq(addr1: u32, addr2: u32) -> bool {
/// The contents of the vector table
#[derive(Debug)]
pub struct VectorTable {
// entry 0
// entry 0: Initial SP
pub initial_stack_pointer: u32,
// entry 1: Reset vector
pub reset: u32,
Copy link
Member

Choose a reason for hiding this comment

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

There was some discussion if we can extract the reset vector from the elf or not and how to make this as stable as possible. Please see #383 (comment). Do you think we can always™️ extract the reset vector the way you implemented it?

Copy link
Author

Choose a reason for hiding this comment

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

From the linked discussion:

It could be that the Flash memory at 0x2000_0000 is aliased at address 0x0000_0000 but I have not checked.

I think that's a typo: the Flash memory is (on STM32s for example; in principe it could be anywhere the instruction fetches are legal) at 0x0800_0000 and the SRAM is (always) at 0x2000_0000.

However, that aside, I looked it up in the ARMv7-M ARM and it seems that I misremembered some details and a much more reliable way would be to read the IVT address from the VTOR register on attach and then use that. I can implement that later.

Screenshot_20230622_171704

Copy link
Member

Choose a reason for hiding this comment

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

I can implement that later.

That would be nice. Maybe in a follow up PR.

In any case I we will need to test the current PR with the rp pico, because that one seems to behave a bit differently than many other microcontrollers because of the bootloader.

Copy link
Member

Choose a reason for hiding this comment

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

I can do the testing. Just need to set it up.

// entry 3: HardFault handler
pub hard_fault: u32,
}
20 changes: 4 additions & 16 deletions src/elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,7 @@ pub struct Elf<'file> {
}

impl<'file> Elf<'file> {
pub fn parse(
elf_bytes: &'file [u8],
elf_path: &'file Path,
reset_fn_address: u32,
) -> Result<Self, anyhow::Error> {
pub fn parse(elf_bytes: &'file [u8], elf_path: &'file Path) -> Result<Self, anyhow::Error> {
let elf = ObjectFile::parse(elf_bytes)?;

let live_functions = extract_live_functions(&elf)?;
Expand All @@ -42,7 +38,7 @@ impl<'file> Elf<'file> {

let debug_frame = extract_debug_frame(&elf)?;

let symbols = extract_symbols(&elf, reset_fn_address)?;
let symbols = extract_symbols(&elf, vector_table.reset)?;

Ok(Self {
elf,
Expand All @@ -56,10 +52,6 @@ impl<'file> Elf<'file> {
})
}

pub fn main_fn_address(&self) -> u32 {
self.symbols.main_fn_address
}

pub fn program_uses_heap(&self) -> bool {
self.symbols.program_uses_heap
}
Expand Down Expand Up @@ -149,11 +141,12 @@ fn extract_vector_table(elf: &ObjectFile) -> anyhow::Result<cortexm::VectorTable
.chunks_exact(4)
.map(|chunk| u32::from_le_bytes(chunk.try_into().unwrap()));

if let (Some(initial_stack_pointer), Some(_reset), Some(_third), Some(hard_fault)) =
if let (Some(initial_stack_pointer), Some(reset), Some(_third), Some(hard_fault)) =
(words.next(), words.next(), words.next(), words.next())
{
Ok(cortexm::VectorTable {
initial_stack_pointer,
reset,
hard_fault,
})
} else {
Expand All @@ -179,14 +172,12 @@ fn extract_debug_frame<'file>(elf: &ObjectFile<'file>) -> anyhow::Result<DebugFr
}

struct Symbols {
main_fn_address: u32,
program_uses_heap: bool,
reset_fn_range: Range<u32>,
rtt_buffer_address: Option<u32>,
}

fn extract_symbols(elf: &ObjectFile, reset_fn_address: u32) -> anyhow::Result<Symbols> {
let mut main_fn_address = None;
let mut program_uses_heap = false;
let mut reset_symbols = Vec::new();
let mut rtt_buffer_address = None;
Expand All @@ -199,7 +190,6 @@ fn extract_symbols(elf: &ObjectFile, reset_fn_address: u32) -> anyhow::Result<Sy

let address = symbol.address().try_into().expect("expected 32-bit ELF");
match name {
"main" => main_fn_address = Some(cortexm::clear_thumb_bit(address)),
"_SEGGER_RTT" => rtt_buffer_address = Some(address),
"__rust_alloc" | "__rg_alloc" | "__rdl_alloc" | "malloc" if !program_uses_heap => {
log::debug!("symbol `{}` indicates heap is in use", name);
Expand All @@ -214,7 +204,6 @@ fn extract_symbols(elf: &ObjectFile, reset_fn_address: u32) -> anyhow::Result<Sy
}
}

let main_fn_address = main_fn_address.ok_or(anyhow!("`main` symbol not found"))?;
let reset_fn_range = {
if reset_symbols.len() == 1 {
let reset = reset_symbols.remove(0);
Expand All @@ -229,7 +218,6 @@ fn extract_symbols(elf: &ObjectFile, reset_fn_address: u32) -> anyhow::Result<Sy
};

Ok(Symbols {
main_fn_address,
program_uses_heap,
reset_fn_range,
rtt_buffer_address,
Expand Down
106 changes: 61 additions & 45 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,7 @@ use probe_rs::{
};
use signal_hook::consts::signal;

use crate::{
canary::Canary,
elf::Elf,
registers::{PC, SP},
target_info::TargetInfo,
};
use crate::{canary::Canary, elf::Elf, target_info::TargetInfo};

const TIMEOUT: Duration = Duration::from_secs(1);

Expand All @@ -54,30 +49,37 @@ fn run_target_program(elf_path: &Path, chip_name: &str, opts: &cli::Opts) -> any
// connect to probe and flash firmware
let probe_target = lookup_probe_target(elf_path, chip_name, opts)?;
let mut sess = attach_to_probe(probe_target.clone(), opts)?;
flash(&mut sess, elf_path, opts)?;
if opts.reset {
flash(&mut sess, elf_path, opts)?;
}

// attack to core
// attach to core
let memory_map = sess.target().memory_map.clone();
let core = &mut sess.core(0)?;

// reset-halt the core; this is necessary for analyzing the vector table and
// painting the stack
core.reset_and_halt(TIMEOUT)?;
// reset if requested, and then halt the core; this is necessary for analyzing
// the vector table and painting the stack
match opts.no_reset {
false => core.reset_and_halt(TIMEOUT),
true => core.halt(TIMEOUT),
}?;

// gather information
let (stack_start, reset_fn_address) = analyze_vector_table(core)?;
let elf_bytes = fs::read(elf_path)?;
let elf = &Elf::parse(&elf_bytes, elf_path, reset_fn_address)?;
let target_info = TargetInfo::new(elf, memory_map, probe_target, stack_start)?;
let elf = &Elf::parse(&elf_bytes, elf_path)?;
let target_info = TargetInfo::new(elf, memory_map, probe_target)?;

// install stack canary
let canary = Canary::install(core, elf, &target_info)?;
let canary = match opts.no_reset {
true => None, // cannot safely touch the stack of a running application
false => Canary::install(core, elf, &target_info)?,
};
if canary.is_none() {
log::info!("stack measurement was not set up");
}

// run program and print logs until there is an exception
start_program(core, elf)?;
attack_the_program(core, elf)?;
let current_dir = env::current_dir()?;
let halted_due_to_signal = print_logs(core, &current_dir, elf, &target_info.memory_map, opts)?; // blocks until exception
print_separator()?;
Expand All @@ -93,8 +95,13 @@ fn run_target_program(elf_path: &Path, chip_name: &str, opts: &cli::Opts) -> any
backtrace::Settings::new(current_dir, halted_due_to_signal, opts, stack_overflow);
let outcome = backtrace::print(core, elf, &target_info, &mut backtrace_settings)?;

// reset the target
core.reset_and_halt(TIMEOUT)?;
if !opts.no_reset {
// reset the target
core.reset_and_halt(TIMEOUT)?;
} else {
// remove our instrumentation and restart the target
detach_from_program(core, elf)?;
}

outcome.log();
Ok(outcome.into())
Expand Down Expand Up @@ -202,24 +209,14 @@ fn flashing_progress() -> flashing::FlashProgress {
})
}

/// Read stack-pointer and reset-handler-address from the vector table.
///
/// Assumes that the target was reset-halted.
///
/// Returns `(stack_start: u32, reset_fn_address: u32)`
fn analyze_vector_table(core: &mut Core) -> anyhow::Result<(u32, u32)> {
let stack_start = core.read_core_reg::<u32>(SP)?;
let reset_address = cortexm::set_thumb_bit(core.read_core_reg::<u32>(PC)?);
Ok((stack_start, reset_address))
}

fn start_program(core: &mut Core, elf: &Elf) -> anyhow::Result<()> {
log::debug!("starting device");
fn attack_the_program(core: &mut Core, elf: &Elf) -> anyhow::Result<()> {
log::debug!("attaching to program");

match (core.available_breakpoint_units()?, elf.rtt_buffer_address()) {
(0, Some(_)) => bail!("RTT not supported on device without HW breakpoints"),
(0, None) => log::warn!("device doesn't support HW breakpoints; HardFault will NOT make `probe-run` exit with an error code"),
(_, Some(rtt_buffer_address)) => set_rtt_to_blocking(core, elf.main_fn_address(), rtt_buffer_address)?,
(_, Some(rtt_buffer_address)) =>
set_rtt_blocking_mode(core, rtt_buffer_address, true)?,
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to halt the program at the beginning of main? Before this PR this was done in set_rtt_to_blocking.

Copy link
Author

Choose a reason for hiding this comment

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

Is there a reason it needs to be? I couldn't find one and that's why I removed the check that was in set_rtt_to_blocking.

Copy link
Member

Choose a reason for hiding this comment

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

I assume (assume!) that we want to wait for potential startup code to finish before we mess around with the micro-controller. Runtime crates like cortex_m_rt (link) run some code before main and also allow users to specify custom code to run before main.

Copy link
Author

Choose a reason for hiding this comment

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

It looks like #[pre_init] specifies any access to memory as UB, so it's not really "life before main".

(_, None) => {}
}

Expand All @@ -229,16 +226,31 @@ fn start_program(core: &mut Core, elf: &Elf) -> anyhow::Result<()> {
Ok(())
}

fn detach_from_program(core: &mut Core, elf: &Elf) -> anyhow::Result<()> {
log::debug!("detaching from program");

if let Some(rtt_buffer_address) = elf.rtt_buffer_address() {
set_rtt_blocking_mode(core, rtt_buffer_address, false)?;
Copy link
Member

@Urhengulas Urhengulas Jun 22, 2023

Choose a reason for hiding this comment

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

What if rtt was in non-blocking mode before we attached? Then we probably don't want to set it to blocking afterwards, right?

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah this is true.

}

core.clear_hw_breakpoint(cortexm::clear_thumb_bit(elf.vector_table.hard_fault).into())?;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this line?

Copy link
Author

Choose a reason for hiding this comment

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

It's some code I moved around--I didn't introduce it. For some reason (I didn't investigate closely) probe-run sets a breakpoint on HardFault. Probably to display a nicer error if you hit one?

core.run()?;

Ok(())
}

/// Set rtt to blocking mode
fn set_rtt_to_blocking(
fn set_rtt_blocking_mode(
core: &mut Core,
main_fn_address: u32,
rtt_buffer_address: u32,
is_blocking: bool,
) -> anyhow::Result<()> {
// set and wait for a hardware breakpoint at the beginning of `fn main()`
core.set_hw_breakpoint(main_fn_address.into())?;
core.run()?;
core.wait_for_core_halted(Duration::from_secs(5))?;
let (mode_name, mode) = match is_blocking {
true => ("blocking", MODE_BLOCK_IF_FULL),
false => ("nonblocking", MODE_NON_BLOCKING_TRIM),
};

log::debug!("setting RTT mode to {mode_name}");

// calculate address of up-channel-flags inside the rtt control block
const OFFSET: u32 = 44;
Expand All @@ -249,14 +261,12 @@ fn set_rtt_to_blocking(
core.read_32(rtt_buffer_address.into(), channel_flags)?;
// modify flags to blocking
const MODE_MASK: u32 = 0b11;
const MODE_NON_BLOCKING_TRIM: u32 = 0b01;
const MODE_BLOCK_IF_FULL: u32 = 0b10;
let modified_channel_flags = (channel_flags[0] & !MODE_MASK) | MODE_BLOCK_IF_FULL;
let modified_channel_flags = (channel_flags[0] & !MODE_MASK) | mode;
// write flags back
core.write_word_32(rtt_buffer_address.into(), modified_channel_flags)?;

// clear the breakpoint we set before
core.clear_hw_breakpoint(main_fn_address.into())?;

Ok(())
}

Expand All @@ -273,17 +283,23 @@ fn print_logs(
let mut logging_channel = if let Some(address) = elf.rtt_buffer_address() {
Some(setup_logging_channel(core, memory_map, address)?)
} else {
eprintln!("RTT logs not available; blocking until the device halts..");
eprintln!("RTT logs not available; blocking until the device halts...");
None
};

let use_defmt = logging_channel
.as_ref()
.map_or(false, |channel| channel.name() == Some("defmt"));

if use_defmt && opts.no_flash {
if use_defmt && (!opts.reset || opts.no_flash) {
log::warn!(
"You are using `--no-flash` and `defmt` logging -- this combination can lead to malformed defmt data!"
"You are using `{}` together with `defmt` logging -- this combination can lead to malformed defmt data!",
match (opts.no_flash, opts.no_reset) {
(true, true) => "--no-flash and --no-reset",
(true, false) => "--no-flash",
(false, true) => "--no-reset",
(false, false) => unreachable!(),
}
);
} else if use_defmt && elf.defmt_table.is_none() {
bail!("\"defmt\" RTT channel is in use, but the firmware binary contains no defmt data");
Expand Down
3 changes: 1 addition & 2 deletions src/target_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ impl TargetInfo {
elf: &Elf,
memory_map: Vec<MemoryRegion>,
probe_target: probe_rs::Target,
stack_start: u32,
) -> anyhow::Result<Self> {
let active_ram_region =
extract_active_ram_region(&probe_target, elf.vector_table.initial_stack_pointer);
Expand All @@ -46,7 +45,7 @@ impl TargetInfo {
memory_map,
probe_target,
stack_info,
stack_start,
stack_start: elf.vector_table.initial_stack_pointer,
})
}
}
Expand Down