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.

Sign up for GitHub

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

Jump to bottom

Fix miri validation errors through now stricter provenance #103

Merged
merged 11 commits into from
Sep 19, 2023
Merged

Fix miri validation errors through now stricter provenance #103

merged 11 commits into from
Sep 19, 2023

Conversation

ten3roberts
Copy link
Contributor

@ten3roberts ten3roberts commented Aug 23, 2023 •
edited
Loading

Miri has become even stricter with regards to pointer borrowing and stacked borrows, especially with the now default full-field-retagging

@ten3roberts
fix(miri): box transmute and invalid references
8c5cabb 
The general causes for the miri invalidation is the prevelant use of
`Box` and its references to `ErrorImpl<()>`.

`mem::transmute` does not preserve the tag stack for transmuting the
boxes.

Additionally, having references to `ErrorImpl<()>` which has a different
layout than the allocation or `ErrorImpl<E>` for some unknown `E`. This
causes the new "untyped" reference to now have a provenance that
includes the size of E and thus is outside the provenance.
@ten3roberts
fix(miri): downcast_mut using &mut _ => *const _ => *mut
3640ba8
@ten3roberts
fix(miri): stub file reading
4547d19
@ten3roberts
Copy link
Contributor Author

Fixes #59

@ten3roberts
Copy link
Contributor Author

ten3roberts commented Aug 23, 2023 •
edited
Loading

Remaining:

  • Fullfill MSRV requirements
  • Test miri in CI
  • Don't use invalid references to get a ref to handler in ErrorImpl
  • Remove development scaffolding

@ten3roberts ten3roberts force-pushed the fix-miri branch 4 times, most recently from 921ba04 to a3ee4a9 Compare August 25, 2023 22:49
@ten3roberts
Copy link
Contributor Author

Miri now passes

The only thing which does not pass is that some of our dependencies (mainly once_cell) does not fullfill our MSRV requirement.

We need to either bump our MSRV or lock once_cell, though I'dprefer that call and discussion to go in a separate PR to limit scope.

@yaahc let me know when you have some time to look through this. Don't worry about being too pedantic; I am here to learn 😊

@ten3roberts ten3roberts requested a review from yaahc August 25, 2023 22:59
@ten3roberts ten3roberts marked this pull request as ready for review August 25, 2023 23:00
yaahc
yaahc reviewed Sep 4, 2023
View reviewed changes
Copy link
Collaborator

@yaahc yaahc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It looks great. I didn't spend too much time trying to audit the correctness of the unsafe; I'm going to trust MIRI and you on that one.

As for the question about once_cell and MSRV, I have no objections to resolving that separately, though I prefer resolving that first, landing that, and then rebasing that into this PR before merging. However, I am open to other plans if you have a different one in mind.

.github/workflows/ci.yml Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
tests/test_location.rs Outdated Show resolved Hide resolved
@ten3roberts
Copy link
Contributor Author

Should we merge this and tackle the failing test that looks like something entirely different in another PR, or should we solve it here?

I suppose this goes in general, do you prefer larger PRs which implement things in full, or smaller atomic PRs; as one would do on trunk based development?

@yaahc
Copy link
Collaborator

yaahc commented Sep 8, 2023

Should we merge this and tackle the failing test that looks like something entirely different in another PR, or should we solve it here?

I suppose this goes in general, do you prefer larger PRs which implement things in full, or smaller atomic PRs; as one would do on trunk based development?

I don't have a strong preference. I don't love having CI be broken on main but it's been a while since we were in that state. I guess I'd probably lean towards atomic PRs.

How about you, which do you prefer?

@ten3roberts
Copy link
Contributor Author

I'd say I am slightly towards somewhat bigger PRs where a feature is working for the most part before being merged in, rather than merging bit by bit and feature flagging it until it is done.

This way, you get a single place to refer to and have the diff gathered and duplicated (if you end up changing the same lines again in another PR)

I don't prefer broken CIs either, and I will see what I can do with the remaining checks 😊

@ten3roberts
fix: ci workflow names
f04d9e9
@ten3roberts
chore: raise msrv to 1.65 (addr2line)
3f529f4
@ten3roberts
chore: revert distinctive CI names due to branch protection rules
b5dd86a 
The new names, such as `Test Platform Matrix` which do make it easier to
see which jobs failed, rather than msrv, test, and miri all being called
`Test Suite`, the in-place branch protection rules wait forever until
the now non-existent `Test Suite` passes
@ten3roberts
Copy link
Contributor Author

@yaahc CI seems to work now. Is it alright by you to merge this now?

@yaahc yaahc merged commit 9f4ecc4 into eyre-rs:master Sep 19, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaahc yaahc yaahc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ten3roberts @yaahc