Fix alignment padding and add test for saving managed resources #110915
Conversation
steveharter
added
area-System.Reflection.Emit
test-enhancement
| private static void SamplePrivateMethod () | ||
| { | ||
| } | ||
| private static void SamplePrivateMethod() |
There was a problem hiding this comment.
Whitespace-only changes to this file
|
Tagging subscribers to this area: @dotnet/area-system-reflection-emit |
...libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveResourceTests.cs
Show resolved
Hide resolved
|
What was the alignment issue that you have mentioned in #110686 (comment) originally? |
steveharter
changed the title
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); | |||
There was a problem hiding this comment.
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
changed the title
| @@ -40,8 +41,8 @@ internal sealed class ManagedTextSection | |||
| public int MetadataSize { get; } | |||
|
|
|||
| /// <summary> | |||
| /// The size of managed resource data stream. | |||
| /// Aligned to <see cref="ManagedResourcesDataAlignment"/>. | |||
| /// The size of managed resource data stream (unaligned). | |||
There was a problem hiding this comment.
@tmat Could you please take a look at the changes in this file? How are the alignment requirements expected to be handled by ManagedPEBuilder?
Fix alignment Assert and add tests for #110686.