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

Allow use under miri (aka: fix UB / update Error impl to contain some of anyhow's bugfixes since Jan) #59

Closed
thomcc opened this issue Oct 10, 2021 · 2 comments · Fixed by #81
Assignees
Labels
P-urgent (Priority) something with a deadline or consequences for inaction

Comments

@thomcc
Copy link

thomcc commented Oct 10, 2021

I'd like to move to using eyre over anyhow more. One downside is that eyre doesn't have the changes that make anyhow pass under miri, which means if I use eyre, much of it needs to be excluded from miri.

Also, there's at least case where the old code was UB in a way that the mutable noalias addition can exploit in theory (although in practice I suspect it would be very difficult). Fixing that requires most of the work to fix the other things, and in general

See dtolnay/anyhow#134 for the original PR to anyhow (which I wrote, for essentially the same reason that I'd like this fixed in eyre), which talks about explicitly which patterns are bad, and gives some background.

In general the TL;DR is "doing dodgy things to &T, &mut T, Box<T>1 is very bad, so you need to use raw pointers". A concrete example is that stuff like ErrorImpl<()> pretty much needs to be held only behind a raw pointer. Instead of using an &ErrorImpl<()> to get at the fields, you need to eitehr use core::ptr::addr_of!/addr_of_mut!, or convert to *const ErrorImpl<E> which can be derefed. There are several other things like this, which are discussed in that PR.

The ergonomics of that PR are quite bad, but it got up cleaned it up significantly by introducing https://github.com/dtolnay/anyhow/blob/master/src/ptr.rs afterwards.

These basically just give NonNull more semantic meaning, and making deref/deref_mut unsafe (even though it probably should explain when it's safe to use it (rarely) and erm, use the correct bounds for Send/Sync 😓)

I'm willing to do the work to make this happen, but... realistically it's probably going to make the code more complex and harder to maintain2, so I figured I'd give a heads up first.

Footnotes

  1. Including ManuallyDrop<Box<T>>

  2. I wish this weren't the case, but the ergonomics of raw pointers are pretty awful across the board :(

@yaahc
Copy link
Collaborator

yaahc commented Oct 11, 2021

I'm willing to do the work to make this happen, but... realistically it's probably going to make the code more complex and harder to maintain2, so I figured I'd give a heads up first.

That's perfectly okay. Any help on getting the fixes implemented would be immensely appreciated.

@yaahc yaahc linked a pull request Jul 7, 2022 that will close this issue
@yaahc yaahc closed this as completed in #81 Jul 7, 2022
@yaahc yaahc reopened this Jul 8, 2022
@yaahc yaahc mentioned this issue Jul 8, 2022
@ten3roberts ten3roberts added P-important (Priority) something that contributes towards achieving the long term goals of the project P-urgent (Priority) something with a deadline or consequences for inaction and removed P-important (Priority) something that contributes towards achieving the long term goals of the project labels Aug 16, 2023
@ten3roberts ten3roberts self-assigned this Aug 23, 2023
@ten3roberts
Copy link
Contributor

Working on this right now 😃

Got it mostly passing, but will need to set up CI testing as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-urgent (Priority) something with a deadline or consequences for inaction
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants