Skip to content

Attempt a basic PC range check as part of hubris.validate #711

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
labbott wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Attempt a basic PC range check as part of hubris.validate #711

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a716c34
Attempt a basic PC range check as part of `hubris.validate`
labbott Jul 7, 2026
015a3a3
restart core after checking PC
evan-oxide Jul 8, 2026
9d708f0
add read_image_id_from_flash()
evan-oxide Jul 8, 2026
4f99d53
don't call validate() before flashing, just check ID
evan-oxide Jul 8, 2026
02df730
warn before flashing if the PC suggests this is the wrong A/B image
evan-oxide Jul 8, 2026
0dd349c
document is_pc_within_archive
evan-oxide Jul 9, 2026
2635ae7
use separate error variants for actual mismatch vs failure to check
evan-oxide Jul 9, 2026
1bab5bc
mention bootloader and ROM in error message
evan-oxide Jul 13, 2026
c01d91c
remove redundant PC check
evan-oxide Jul 13, 2026
cfa9fe2
add comment about single-task dumps
evan-oxide Jul 13, 2026
b683227
overwrite tests that just have a different error message now
evan-oxide Jul 13, 2026
787b3a1
overwrite flash-ram-mismatch tests that started erroring
evan-oxide Jul 13, 2026
3d1e66b
overwrite in_bootloader tests that started erroring
evan-oxide Jul 13, 2026
220886a
don't leave core halted if PC check fails
evan-oxide Jul 13, 2026
e6726f4
refactor is_pc_within_archive using PcResult
evan-oxide Jul 13, 2026
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
12 changes: 10 additions & 2 deletions humility-core/src/hubris.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2103,11 +2103,19 @@
);
}

if criteria == HubrisValidate::ArchiveMatch {
if self.task_dump().is_some() {
return Ok(());

@evan-oxide evan-oxide Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@labbott What does it mean for an archive to be "a dump from a single task"? I'm wondering if it's intentional/correct that if you pass in HubrisValidate::Booted in that case, it returns early here and skips the boot check.

@jamesmunns jamesmunns Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's possible to take a dump from only a single task's memory range (e.g. a "single task dump"), but not an "entire system" dump. Laura can confirm though :)

@labbott labbott Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

James is correct. We have the notion of single task dumps vs a wholes system dump. You can run many of the same commands like e.g. humility tasks and humility ringbuf and things should work as expected. I don't think we capture the boot information in a single task dump which is why it gets skipped.

@evan-oxide evan-oxide Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, I added a comment about it.

} else {
core.halt()?;

@evan-oxide evan-oxide Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we do core.run()? again after checking the PC?

@labbott labbott Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Absolutely yes this got lost in some refactoring 😬

if let Ok(pc) = core.read_reg(ARMRegister::PC) {

Check failure on line 2110 in humility-core/src/hubris.rs

View workflow job for this annotation

GitHub Actions / Clippy 

this `if` statement can be collapsed
if self.instr_mod(pc).is_none() {
bail!("PC at 0x{pc:x} is not part of any module. This is \

@hawkw hawkw Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unimportant turbo nitpick: if we write

Suggested change
bail!("PC at 0x{pc:x} is not part of any module. This is \
bail!("PC at {pc:#x} is not part of any module. This is \

that tells the formatter to emit the 0x prefix itself. which...changes absolutely nothing here so it doesn't matter...

@evan-oxide evan-oxide Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the way this interacts with flashing is not ideal. If you try to re-flash an A image that's already been flashed to the A slot while the RoT is running a B image:

  • the old behavior was to fail with "archive appears to be already flashed on attached device; use -F ("--force") to force re-flash"
  • this PR's behavior is to re-flash the A image
  • the ideal behavior is probably to warn the user that they probably meant to flash the other image, then fail with the "archive appears to be already flashed" error.

@labbott labbott Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah you're right that's confusing behavior. I think that would point to needing to fix validate to return a more specific error

@evan-oxide evan-oxide Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or don't use validate for the pre-flash check. In my PR, I considered a bunch of different return types then gave up trying to make the same validate function work for flashing and for both of the HubrisValidate options.

likely an incorrect archive.");
}
}

@hawkw hawkw Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i believe clippy would like you to have written something more like:

Suggested change
if let Ok(pc) = core.read_reg(ARMRegister::PC) {
if self.instr_mod(pc).is_none() {
bail!("PC at 0x{pc:x} is not part of any module. This is \
likely an incorrect archive.");
}
}
if let Ok(pc) = core.read_reg(ARMRegister::PC)
&& self.instr_mod(pc).is_none() {
bail!("PC at 0x{pc:x} is not part of any module. \
This is likely an incorrect archive.");
}

}

if self.task_dump().is_some() {
if criteria == HubrisValidate::ArchiveMatch {
return Ok(());
}

Expand Down
Loading