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.

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

Add unaligned feature #231

Merged
merged 1 commit into from
May 28, 2020
Merged

Add unaligned feature #231

merged 1 commit into from
May 28, 2020

Conversation

philipc
Copy link
Contributor

@philipc philipc commented May 23, 2020

While the structs defined in this crate have the alignment required by the specifications, some toolchains violate these requirements. Add an unaligned feature that allows these files to be processed, at the cost of potentially slower code.

Fixes #230

I've kept this out of the all feature for now. I'm not sure if this should be enabled by default. @alexcrichton What are your thoughts on what backtrace-rs will want for this? I'm not sure how much the performance of this matters, and whether you want to selectively enable it for some architectures (most likely x86-64).

@philipc philipc requested a review from fitzgen May 23, 2020 04:55
@alexcrichton
Copy link
Contributor

I think I may be a bit confused, what happens today if anything is unaligned? Does an error get returned in parsing or does it end up being unsound?

The way I thought something like this would look is that by default everything is considered unaligned to be safe, and then there'd be a crate feature for "assume everything is aligned" and alignment checks are added early on in parsing. (which may be higher performance?)

@bjorn3
Copy link
Contributor

bjorn3 commented May 23, 2020

From gimli-rs/addr2line#170:

thread 'main' panicked at 'called Result::unwrap() on an Err value: Error("Invalid ELF section header offset/size/alignment")', examples\addr2line.rs:156:17

@philipc
Copy link
Contributor Author

philipc commented May 23, 2020

Today the alignment checks are already there, and it returns an error if anything is unaligned. The feature has to be "assume everything is unaligned", because features must be additive: it is adding the ability to parse unaligned headers.

@alexcrichton
Copy link
Contributor

Ah ok, makes sense. I don't really have an idea of what the performance impact to this would be, but I doubt that it'd affect the backtrace crate too too much either way.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

👍

LGTM; the discussion in this thread was clarifying.

While the structs defined in this crate have the alignment required by the
specifications, some toolchains violate these requirements.
Add an `unaligned` feature that allows these files to be processed,
at the cost of potentially slower code.
@philipc
Copy link
Contributor Author

philipc commented May 28, 2020

Based on sentiment here and in the bug report, it sounds like it will be more convenient if this enabled by default, so I've changed it to that.

@philipc philipc merged commit 9dec442 into gimli-rs:master May 28, 2020
@philipc philipc deleted the unaligned branch May 28, 2020 06:18
mcbegamerxx954 pushed a commit to mcbegamerxx954/object that referenced this pull request Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide how to handle incorrect alignment
4 participants