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

Override std::ptr::align_offset #2396

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tautschnig
Copy link
Member

Description of changes:

This hook intercepts calls to std::ptr::align_offset<T> as CBMC's memory model has no concept of alignment of allocations, so we would have to non-deterministically choose an alignment of the base pointer, add the pointer's offset to it, and then do the math that is done in library/core/src/ptr/mod.rs. Instead, we choose to always return usize::MAX, per align_offset's documentation, which states: "It is permissible for the implementation to always return usize::MAX. Only your algorithm’s performance can depend on getting a usable offset here, not its correctness."

Resolved issues:

Fixes: #2363

Related RFC:

n/a

Call-outs:

n/a

Testing:

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

This hook intercepts calls to `std::ptr::align_offset<T>` as CBMC's memory
model has no concept of alignment of allocations, so we would have to
non-deterministically choose an alignment of the base pointer, add the
pointer's offset to it, and then do the math that is done in
`library/core/src/ptr/mod.rs`. Instead, we choose to always return
`usize::MAX`, per `align_offset`'s documentation, which states: "It is
permissible for the implementation to always return usize::MAX. Only your
algorithm’s performance can depend on getting a usable offset here, not its
correctness."

Fixes: model-checking#2363
@tautschnig
Copy link
Member Author

Perhaps @karkhaz:

  1. The alleged performance regression (https://github.com/model-checking/kani/actions/runs/4753944159/jobs/8446153448?pr=2396) is the same as previously reported upon a merge into main: https://github.com/model-checking/kani/actions/runs/4747428362/jobs/8432323295.
  2. Would benchcomp also tell us about any performance improvements?

Copy link
Contributor

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

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

Is there any potential unsoundness with this change?

impl<'tcx> GotocHook<'tcx> for AlignOffset {
fn hook_applies(&self, tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> bool {
let name = with_no_trimmed_paths!(tcx.def_path_str(instance.def_id()));
name == "std::ptr::align_offset"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should include core::ptr::align_offset as well.

Copy link
Contributor

@celinval celinval Apr 25, 2023

Choose a reason for hiding this comment

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

Actually, they are two different methods. the std one is safe, while the core one is unsafe. Instead of using the name here, I would recommend using the align_offset lang_item instead, since it is way more reliable. This is how MIRI does it:

https://github.com/rust-lang/rust/blob/94524020ea12f7947275063b65f8b7d705be073e/src/tools/miri/src/shims/mod.rs#L43

In this case, you would be overriding the core version, which is unsafe and it is UB to execute it with an input that is not power of 2. So please add a check for that.

Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

I don't think this implementation is sound. The code should panic if the input is not a power of two.
For example, the following code panic in Rust:

use std::mem::align_of;

fn main() {
  let x = 10;
  let ptr = &x as *const i32;
  let _ = ptr.align_offset(5);
}

@tautschnig tautschnig self-assigned this Apr 27, 2023
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.

Improve symbolic execution performance when running against concrete values
3 participants