Fix several issues with CreateSpan support #65919
Merged
Conversation
This file contains 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
1. It turns out we can't use the system types (short, int, long) for data fields when there's an alignment requirement, as those types have .pack 1 and thus a rewriter (e.g. illink) might not align fields using those types appropriately. 2. We weren't incorporating the alignment into the name of the ExplicitSizeStruct created, so we could end up with two types both for the same size but for different alignments and with the same name. 3. In looking at the code again, I realized we could simplify it so that EmitArrayBlockInitializer doesn't take an alignment at all, since for array purposes, we always want to use alignment 1. Because it took an alignment, we were unnecessarily specifying one even for the no-CreateSpan fallback. This was functionally correct but unnecessary and resulting in a bit more complicated IL, especially once (1) above was fixed.
AlekseyTs
reviewed
Dec 10, 2022
src/Compilers/Core/Portable/CodeGen/PrivateImplementationDetails.cs
Outdated
Show resolved
Hide resolved
AlekseyTs
reviewed
Dec 10, 2022
AlekseyTs
reviewed
Dec 10, 2022
AlekseyTs
reviewed
Dec 10, 2022
|
Done with review pass (commit 1) |
|
Thanks. Feedback addressed. |
|
@stephentoub It looks like there are legitimate test failures |
AlekseyTs
reviewed
Dec 10, 2022
src/Compilers/Core/Portable/CodeGen/PrivateImplementationDetails.cs
Outdated
Show resolved
Hide resolved
AlekseyTs
reviewed
Dec 10, 2022
src/Compilers/Core/Portable/CodeGen/PrivateImplementationDetails.cs
Outdated
Show resolved
Hide resolved
|
Done with review pass (commit 2) |
It's some Windows vs Unix difference. |
|
Apparently we expect different output / blob addresses based on OS, e. g. roslyn/src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs Lines 3333 to 3335 in 42f323d Why is that? I'll work around it, but doesn't that go against deterministic builds? |
stephentoub
force-pushed
the
fixcreatespan
branch
from
b987713 to
f90cce2
Compare
|
I found @jaredpar's comment in #51437 (comment) about no guarantees of determinism across OS. |
|
Tests passed now. |
ghost
added this to the
|
Thank you, @stephentoub. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
It turns out we can't use the system types (short, int, long) for data fields when there's an alignment requirement, as those types have .pack 1 and thus a rewriter (e.g. illink) might not align fields using those types appropriately.
We weren't incorporating the alignment into the name of the ExplicitSizeStruct created, so we could end up with two types both for the same size but for different alignments and with the same name.
In looking at the code again, I realized we could simplify it so that EmitArrayBlockInitializer doesn't take an alignment at all, since for array purposes, we always want to use alignment 1. Because it took an alignment, we were unnecessarily specifying one even for the no-CreateSpan fallback. This was functionally correct but unnecessary and resulting in a bit more complicated IL, especially once (1) above was fixed.
The telltale sign of the problem is "System.ArgumentException : Value does not fall within the expected range." in
CreateSpancall.cc: @jcouv
Contributes to dotnet/runtime#79477
Follow-up on #61414