Fix alignment padding and add test for saving managed resources

[steveharter]

Fix alignment Assert and add tests for #110686.

[steveharter]

Whitespace-only changes to this file

[dotnet-policy-service]

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

[jkotas]

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

[steveharter]

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.

[jkotas]

@tmat Could you please take a look at the changes in this file? How are the alignment requirements expected to be handled by ManagedPEBuilder?

[steveharter]

ping @tmat

[tmat]

I don't remember what the expectations were. It's been a while.

To me it looks like we should have checked the alignments in ManagedPEBuilder constructor and throw if they are off. Can we add that check? It would break someone who emits unaligned data, but if they do the emitted assembly would be bad anyways.

[steveharter]

There are basically two choices: throw if not aligned or align it for the user.

This PR takes the path of aligning it for the user like we do in other areas:

• MappedFieldDataSize
  

  runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs

  Line 143 in 07e4c1b

  result = BitArithmetic.Align(result, MappedFieldDataAlignment);

• ImportTable

  runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs

  Line 131 in 1ce82e7

  result = BitArithmetic.Align(result, Is32Bit ? 4 : 8); //optional padding to make startup stub's target address align on word or double word boundary

• Startup Stub

  runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs

  Line 185 in 07e4c1b

  return OffsetToILStream + BitArithmetic.Align(ILStreamSize, 4);

If we don't align it for the user, the user would need an extra LOC to do the alignment for the somewhat common case of using resources, and that extra LOC that would be needed is not obvious to the caller.

[steveharter]

but if they do the emitted assembly would be bad anyways.

I don't believe the lack of alignment breaks anything, unless there is some platform-specific alignment limitations that I'm not aware of -- e.g. the test added here worked before the alignment was added.

[tmat]

I'm not sure I follow. If the alignment is not needed then why are we adding it?

[steveharter]

There was an Assert that was failing so we either need to remove it or add the alignment \ padding. Since there was a ManagedResourcesDataAlignment public const I decided to use that (it wasn't used before). Finally, the assert was checking if alignment to 4 while the public const is 8.

I do not know why the const was added, or why the alignment and checks are there in part. Git history shows this existed 8+ years ago as part of large commits. Likely the alignment was added for efficiency when reading the subsequent data. The code may have assumed the caller would align the data, but if so then the code should have thrown and not just asserted.

[steveharter]

@jkotas @tmat any final comments or asks?

I don't think we need to port this to v9, as the alignment added here appears optional.