Skip to content
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

Allow skipping the defmt version check #296

Merged
merged 2 commits into from
Dec 3, 2020
Merged

Allow skipping the defmt version check #296

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5506c7
Allow skipping the defmt version check
jonas-schievink Dec 3, 2020
8f3cac1
Make the warning more prominent
jonas-schievink Dec 3, 2020
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
20 changes: 17 additions & 3 deletions elf2table/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,22 @@ pub use defmt_decoder::Table;
use defmt_decoder::{StringEntry, TableEntry};
use object::{Object, ObjectSection};

/// Parses an ELF file and returns the decoded `defmt` table
/// Parses an ELF file and returns the decoded `defmt` table.
///
/// This function returns `None` if the ELF file contains no `.defmt` section
/// This function returns `None` if the ELF file contains no `.defmt` section.
pub fn parse(elf: &[u8]) -> Result<Option<Table>, anyhow::Error> {
parse_impl(elf, true)
}

/// Like `parse`, but does not verify that the defmt version in the firmware matches the host.
///
/// CAUTION: This is meant for defmt/probe-run development only and can result in reading garbage
/// data.
pub fn parse_ignore_version(elf: &[u8]) -> Result<Option<Table>, anyhow::Error> {
parse_impl(elf, false)
}

fn parse_impl(elf: &[u8], check_version: bool) -> Result<Option<Table>, anyhow::Error> {
let elf = object::File::parse(elf)?;
// first pass to extract the `_defmt_version`
let mut version = None;
Expand Down Expand Up @@ -75,7 +87,9 @@ pub fn parse(elf: &[u8]) -> Result<Option<Table>, anyhow::Error> {
}
};

defmt_decoder::check_version(version).map_err(anyhow::Error::msg)?;
if check_version {
Copy link
Contributor

@Lotterleben Lotterleben Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

personally, I'd rather pull this out of parse_impl() and move it into parse() right before the call to parse_impl(). That way we can avoid having a flag argument* as well as tight coupling between the parsing and defmt consistency check, which imo are two different tasks.

* the function names of parse_ignore_version() and parse() imo already communicate what's happening better than parse(.. , true) does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing the version check requires some amount of processing to get the version, so I'd have to duplicate that into the caller.

tight coupling between the parsing and defmt consistency check, which imo are two different tasks.

There is some amount of intentional coupling between them, for example, we always check that there's only one version in use, and we check for the existence of the .defmt section and the version symbol to determine whether defmt is in use at all, or the linker script is broken.

This PR is only intended to skip the version check, not any of the other consistency checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

hrm , ok then that's maybe one for future refactoring sessions...

defmt_decoder::check_version(version).map_err(anyhow::Error::msg)?;
}

// second pass to demangle symbols
let mut map = BTreeMap::new();
Expand Down