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

Fix alignment padding and add test for saving managed resources #110915

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Dec 23, 2024

Fix alignment Assert and add tests for #110686.

@steveharter steveharter added area-System.Reflection.Emit test-enhancement Improvements of test source code labels Dec 23, 2024
@steveharter steveharter self-assigned this Dec 23, 2024
private static void SamplePrivateMethod ()
{
}
private static void SamplePrivateMethod()
Copy link
Member Author

Choose a reason for hiding this comment

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

Whitespace-only changes to this file

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

@jkotas
Copy link
Member

jkotas commented Dec 23, 2024

What was the alignment issue that you have mentioned in #110686 (comment) originally?

@steveharter steveharter removed the test-enhancement Improvements of test source code label Dec 23, 2024
@steveharter steveharter changed the title Add test for saving and reading managed resources Fix alignment and add test for saving resources using PersistedAssemblyBuilder Dec 23, 2024
@steveharter
Copy link
Member Author

What was the alignment issue that you have mentioned in #110686 (comment) originally?

I pushed a new commit for that. I assume the best practice here is to not modify the incoming blob directly to add the padding, so the alignment is done when writing.

Also, the Assert checked on 4-byte alignment, but the unused const had 8, so I'm assuming 8. I'll also double-check to see what Roslyn does.

@@ -147,14 +148,16 @@ public int CalculateOffsetToMappedFieldDataStream()

internal int ComputeOffsetToDebugDirectory()
{
Debug.Assert(MetadataSize % 4 == 0);
Debug.Assert(ResourceDataSize % 4 == 0);
Debug.Assert(MetadataSize % MetadataSizes.StreamAlignment == 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not 100% sure if this is the intent (4 byte alignment across the various sections); I think we could remove this Assert since they should all be aligned anyway

@steveharter steveharter changed the title Fix alignment and add test for saving resources using PersistedAssemblyBuilder Fix alignment padding and add test for saving managed resources Dec 24, 2024
@steveharter steveharter requested a review from ericstj December 24, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants