Allow test suite to pass under miri, and fix various other miri/stacked borrow issues

[thomcc]

There are a few things that anyhow does that are currently considered unsound by miri and stacked borrows, this fixes them to the best of my ability. After this, the test suite passes under miri. It also cases where miri didn't complain, but appeared to be unsound (that said the tests don't write to error types a lot).

I'd understand if you don't like the way I've done some things, I kind of expect if this is to land it would need at least a round of review to clean it up further, as some of the stuff doesn't make as much sense now.

Changes

Anyway, I've tried to make this happen a few times and it's mostly evaded me. This time I managed to get it working.

These are, broadly, the changes it makes, and why I made them, so that you have hopefully enough context to feel in-the-loop for reviewing.

• Avoids using Box<ErrorImpl<()>> There are reasons for this:

  1. Converting between Box<T> and Box<U> where T and U don't have the same layout is UB in many (all?) cases (for example: if U is larger than T it is definitely UB for reasons mentioned in the second bit)
  2. This avoids the situations where have a not-entirely-unique Box<T>, for example when we read one out of a ManuallyDrop field, and such. ManuallyDrop doesn't save us here, since it has no impact on aliasing or validity. We could have uses MaybeUninit instead, but just using NonNull is cleaner.

  See Aliasing rules for Box<T> rust-lang/unsafe-code-guidelines#258

• Avoids &ErrorImpl<()>, &mut ErrorImpl<()> essentially everywhere. There are too many ways this can go wrong. The most notable one that anyhow hits is that it likes to turn &ErrorImpl<()> into &ErrorImpl<E>, where E is bigger than ().

  Currently this is UB. The mental model I use for this is that the first borrows is only for one specific range of bytes, and so you don't get to decide that you're borrowing more. (It kinda sucks and applies to Box too)

  See Storing an object as &Header, but reading the data past the end of the header rust-lang/unsafe-code-guidelines#256

  The other reason for this change is that in general the code seemed to play it pretty fast and lose with pointers, which can be fine, but (among other things) sorta requires they be raw.

  That said, I was a bit more paranoid than perhaps I need to be here, both because the kind of code that would cause issues (mutable access) isn't well covered by tests, and because it's easier to just follow a rule of "always avoid &ErrorImpl<()>" rather than a lot of thinking about it a ton.

• Avoids cases where we go &T => *const T => *mut T => &mut T.

  Annoyingly, it turns out it's not 100% true that const/mut are equivalent. Pointers that start out as const are illegal to ever write to. The biggest change was that a distinct downcast_mut function was needed in the vtable, but i think there were smaller onces.

  See Differences between *const T and *mut T. Initially *const T pointers are forever read-only? rust-lang/unsafe-code-guidelines#257

That's probably it.

Note: I also switched things away from using transumute in favor of raw ptr conversions, but in retrospect the transmute would have definitely been fine, even between NonNull/Box, so if you prefer them I can try and undo that part.

Options for cleaning this all up

So, again, I'm unsurprised if you'd rather not take it as is. It makes a lot more code unsafe, and harder to work with.

Sadly, using references is a bit of a double-edged sword. They make maintenance easier, but also make it much easier to have unsound code, since they let the compiler assume so much about what's allowed. And instead, using pointers greatly increases how much unsafe code there is (any function taking a pointer it reads from needs to be).

One option to improve things if you'd like would be add a transparent wrapper for NonNull<ErrorImpl<()>> that would have more ref-like semantics, hold a lifetime, and allow use from safe code (it would just be unsafe to create). (Then, stuff like ErrorImpl::{display,debug,error,backtrace,error_mut,etc} could be safe again, and it could be used in vtable functions to make some ref/mut versions more obviously distinct).

I didn't do this for a few reasons:

• adds complexity, you'd have to sort out which bits are required and which are are just to make things nicer.
• you might have a different design you'd prefer to clean this up
• you might not want to take the patch at all (or might want to take it as is)

Oh, other big missing thing is that I need to update safety annotations on several comments, but I figured I'd do a temp check on this first.

---

Anyway, for clarity: I don't think any of these issues can cause any problems in practice under current compilers. It's possible that for several of them the outcome will be "stacked borrows is wrong".

That said, it feels plausible that at least some of these are real problems, and is nice if code that uses anyhow can use miri too, even if they happen to test a case that triggers an error.

P.S. very sorry for writing an whole dang novel about this PR. It's complex and didn't want to just drop it with no context, since I know how that can be.

[dtolnay]

Thanks for the PR. My view is "stacked borrows is wrong" and needs to be adapted to accommodate what some of this code was doing, especially rust-lang/unsafe-code-guidelines#256, but I understand it will be a long time as this plays out. I am hesitant about accepting this PR because:

• it makes things less safe, harder to work with and keep working, as you observed.
• it increases bloat, with the duplication of vtable methods with functionally identical behavior. I really hope that an alternative will emerge for this and the vtables can be shrunk back down.
• people may perceive it as implying that I endorse Miri's current stacked borrows-based implementation as authoritative of Rust semantics.

However Miri support in downstream code is compelling from a feature perspective so on balance I think it's worth accepting the change. Implementation-wise this seems fine to me. Feel free to send followup cleanups in subsequent PRs. Thanks!

[thomcc]

Great! This means (among other things), I can use anyhow in a custom test runtime/driver I'm working on, since it being unable to use miri for the tests it's running would be pretty unfortunate! (That said, I've already removed it, which is perhaps for the best in this case...).

I'm a little surprised, after writing the PR I saw #62 (comment), and prepared for an r-. Glad you took it!

My view is "stacked borrows is wrong" and needs to be adapted to accommodate what some of this code was doing, especially rust-lang/unsafe-code-guidelines#256

Yeah, I definitely agree... Rust has made a lot of decisions in the past to make unsafe code easier to write (no strict aliasing, no move ctors, etc), and I'm worried about going in the direction of making unsafe harder to write, in exchange for optimizations and simpler formal semantics (for example, we already lost uninitialized integers, even if they are never read from...).

... duplication of vtable methods with functionally identical behavior. I really hope that an alternative will emerge for this and the vtables can be shrunk back down.

So, I think raw refs will allow unifying these, but I believe the rules about going &T -> *const T -> *mut T -> &mut T aren't just Miri/SB being picky, and are because the LLVM IR we currently generate assumes it can't happen.

That is, as soon as you have a &T, it gets tagged as readonly in the LLVM IR, which can then be applied to all pointers based on it (not recursively, but I think it wouldn't need to be recursive in this case). That said, it's pretty likely that LLVM can't devirtualize this stuff in real world code, and it would need to.

(Ironically, you could fix LLVM's UB here and have only one downcast function by having it always takes/work with &mut T, transmuting &T -> &mut T when calling it in the normal downcast. This would definitely be Rust-level UB tho, but AFAIK not in a way reflected in the LLVM-IR, which considers &mut T to be strictly less restrictive than &T. I'm not actually suggesting this, though!)

Implementation-wise this seems fine to me. Feel free to send followup cleanups in subsequent PRs. Thanks!

If you're fine with it as-is, then that's fine. I may later try to clean up the comments (which likely are not complete in terms of "Safety:" notices, and pull unsafe into callers in things like the fmt stuff, but not sure when I'll get around to it.

---

P.S. At the risk of stating the obvious: if you feel strongly about Miri/SB semantics being wrong, it's probably worth participating in some extent in the UCG discussions.

That is, I think without people saying "no, unsafe code needs this" the current model won't improve (if anything it feels like it's getting stricter), and your voice would carry a lot of weight — definitely more than comes from just citing that one of your libraries does something (since the assumption is that it just needs to be fixed).

I know you're busy tho, but yeah, just thought I'd say something.