Fix bug in validate_cast_and_convert_metadata #376
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joshlf
force-pushed
the
fix-validate-cast-and-convert-metadata
branch
3 times, most recently
from
d221456 to
27c0511
Compare
jswrenn
force-pushed
the
fix-validate-cast-and-convert-metadata
branch
3 times, most recently
from
abe2e9d to
60777ef
Compare
joshlf
force-pushed
the
fix-validate-cast-and-convert-metadata
branch
4 times, most recently
from
2fb97bb to
91a1547
Compare
jswrenn
previously approved these changes
Sep 18, 2023
Previously, `validate_cast_and_convert_metadata` incorrectly assumed that, for slice DSTs, the trailing slice element would always have an alignment at least as large as the outer type's alignment, and so there would never be any padding added after the trailing slice. In this commit, we rewrite `validate_cast_and_convert_metadata` to no longer assume this behavior. While we're here, we add the `test_validate_rust_layout` test, which validates many of our assumptions about Rust's type layout against the standard library to help catch mismatches like this in the future.
joshlf
force-pushed
the
fix-validate-cast-and-convert-metadata
branch
from
91a1547 to
c2f0580
Compare
jswrenn
approved these changes
Sep 18, 2023
joshlf
added a commit
that referenced
this pull request
Sep 19, 2023
In #376, we introduced `test_validate_rust_layout`, which took a long time to run under Miri. In order to prevent it from consuming too much time in CI, we marked it as `#[cfg_attr(miri, ignore)]`, and configured CI to pass `-- --ignored` to Miri only a small percentage of the time. This had the unintended side effect of running `test_validate_cast_and_convert_metadata` under Miri. That test is marked as `#[cfg_attr(miri, ignore)]` for good reason - it takes far too long to run under Miri, and doesn't exercise any `unsafe` code, so doesn't benefit from Miri. Enabling this test has caused CI to timeout. In the interim, in #395, we optimized `test_validate_rust_layout` so that it no longer takes an unreasonably long time to run under Miri. Thus, in this commit, we remove the `#[cfg_attr(miri, ignore)]` annotation from that test, and no longer pass `-- --ignored` to Miri in CI. Closes #397
joshlf
added a commit
that referenced
this pull request
Sep 19, 2023
In #376, we introduced `test_validate_rust_layout`, which took a long time to run under Miri. In order to prevent it from consuming too much time in CI, we marked it as `#[cfg_attr(miri, ignore)]`, and configured CI to pass `-- --ignored` to Miri only a small percentage of the time. This had the unintended side effect of running `test_validate_cast_and_convert_metadata` under Miri. That test is marked as `#[cfg_attr(miri, ignore)]` for good reason - it takes far too long to run under Miri, and doesn't exercise any `unsafe` code, so doesn't benefit from Miri. Enabling this test has caused CI to timeout. In the interim, in #395, we optimized `test_validate_rust_layout` so that it no longer takes an unreasonably long time to run under Miri. Thus, in this commit, we remove the `#[cfg_attr(miri, ignore)]` annotation from that test, and no longer pass `-- --ignored` to Miri in CI. Closes #397
35 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously,
validate_cast_and_convert_metadataincorrectly assumedthat, for slice DSTs, the trailing slice element would always have an
alignment at least as large as the outer type's alignment, and so there
would never be any padding added after the trailing slice. In this
commit, we rewrite
validate_cast_and_convert_metadatato no longerassume this behavior.
While we're here, we add the
test_validate_rust_layouttest, whichvalidates many of our assumptions about Rust's type layout against the
standard library to help catch mismatches like this in the future.