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

Be compatible with -Zmiri-strict-provenance #477

Merged
merged 1 commit into from Sep 1, 2022

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Aug 27, 2022

CI for #476 is currently failing, because CI asserts that we are compatible with strict provenance but we aren't.

For a crate like this I don't know if the right choice is to disable the checks or not. The use of pointers here is a bit unconventional. (decision: remove the strict provenance check)

@saethlin saethlin marked this pull request as draft August 27, 2022 04:18
@saethlin saethlin force-pushed the strict-provenance branch 2 times, most recently from e3d98d8 to 7ad0ec4 Compare August 27, 2022 05:04
@saethlin
Copy link
Member Author

No idea what's going on with the CI at the moment. My best guess is that something with the windows runners is broken. I'll try again tomorrow.

@alexcrichton
Copy link
Member

Is it possible to perhaps disable this in miri? I'd rather not lose the ability to have debugging assertions and otherwise it seems like larger refactorings would be necessary in this crate to truly support the strict-provenance changes.

@saethlin
Copy link
Member Author

The problem with the existing code is that the backtrace-rs CI runs Miri with -Zmiri-strict-provenance, which reports an error if the code attempts to cast an integer to a pointer. The reason we have this behavior is honestly that the strict_provenance feature has not been stabilized, so we do not have any special intrinsics or any way to indicate that "I am converting a pointer to an integer and this allocation is now exposed" and "I am converting an integer to a pointer, and I expect to be able to dereference this pointer". Inside the strict provenance feature, these are called ptr::expose_addr and ptr::from_exposed_addr.

What backtrace-rs wants is a way to convert pointers to integers and back but with no expectation that the pointer will be dereferenced. Personally reading the code it is very strange to me that these pointers are ever used as usize. But currently, the way we express the operation that backtrace-rs wants is transmute (because the strict provenance feature is not stabilized), which I used in the first example.

Very pragmatically, since this repo sees relatively little attention and the whole strict provenance checking in Miri is unstable and the APIs to work in the model are still unstable, I think the best thing to do here is just remove the -Zmiri-strict-provenance flag from the CI. The whole point of the flag is to opt into a stricter model which is more checkable, and this codebase could probably follow that model but migrating to it might be a big PR (a lot of usize would probably have to be changed to some pointer type).

@RalfJung
Copy link
Member

What is the problem with these debug assertions? I think they can be kept.

If the strict provenance API was stable, we could use ptr::invalid -- that is exactly the function to use for pointers that will not be dereferenced. This PRs just inlines that function since it is not stable yet.

@saethlin
Copy link
Member Author

saethlin commented Aug 31, 2022

We could also add a dependency on sptr. (Or I guess paste its invalid impl into this crate)

@@ -23,7 +23,6 @@ fn backtrace_new_unresolved_should_start_with_call_site_trace() {
assert!(!b.frames().is_empty());

let this_ip = backtrace_new_unresolved_should_start_with_call_site_trace as usize;
println!("this_ip: {:?}", this_ip as *const usize);
Copy link
Member

Choose a reason for hiding this comment

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

So why did you remove these assertions?

If this is because :p internally calls expose_addr, then we should probably start the discussion of using addr there instead -- and in the mean time either gate these prints by cfg(not(miri)) or just remove the -Zmiri-strict-provenance from the CI flags.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is because :p internally calls expose_addr.

If everyone else is comfortable with it, I think the most appropriate approach is to remove -Zmiri-strict-provenance, because as i said above, this codebase plays a bit fast and loose with types, and it wouldn't hurt to address that... but not here.

@saethlin saethlin marked this pull request as ready for review September 1, 2022 00:04
@saethlin
Copy link
Member Author

saethlin commented Sep 1, 2022

I believe CI still won't pass because of the problem with the 32-bit Windows check, but 🤷 this will get it farther than before

@alexcrichton
Copy link
Member

Ok I think this is reasonable enough to land for now. If it becomes more necessary though to enable strict provenance in the standard library and CI I think it would probably be a good time to revisit all of the API details here to figure out an API that better suits that world.

@alexcrichton alexcrichton merged commit 07872f2 into rust-lang:master Sep 1, 2022
@saethlin saethlin deleted the strict-provenance branch February 16, 2024 23:42
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.

None yet

3 participants