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

steveharter · 2024-12-23T15:28:21Z

Fix alignment Assert and add tests for #110686.

steveharter · 2024-12-23T15:28:50Z

src/libraries/System.Reflection.Emit/tests/AssemblyBuilderTests.cs

-	private static void SamplePrivateMethod ()
-	{
-	}
+        private static void SamplePrivateMethod()


Whitespace-only changes to this file

dotnet-policy-service · 2024-12-23T15:28:52Z

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

...libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveResourceTests.cs

jkotas · 2024-12-23T16:08:49Z

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

steveharter · 2024-12-23T18:14:55Z

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.

...es/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs

…blob for padding

jkotas · 2024-12-25T18:51:14Z

...es/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs

@@ -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).


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

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.

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.

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.

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

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.

...es/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs

steveharter · 2025-01-17T15:59:44Z

@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.

steveharter · 2025-02-04T22:34:52Z

ping @tmat @jkotas - any additional comments here?

jkotas · 2025-02-10T04:48:28Z

MappedFieldDataSize has the same problem. If somebody passes in MappedFieldDataSize that is not aligned to MappedFieldDataAlignment, we are going to fail assert here:

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

Line 190 in 1c3b1b5

Debug.Assert(MappedFieldDataSize % MappedFieldDataAlignment == 0);
For MappedFieldData, the alignment padding is inserted before the start of the block, so that the payload is aligned:

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

Lines 306 to 307 in 1c3b1b5

if (mappedFieldDataBuilderOpt.Count != 0)

builder.Align(MappedFieldDataAlignment);

. That makes sense since the alignment of the payload matters.
Similarly, the alignment of RuntimeStartupStub is inserted before the RuntimeStartupStub. Again, the alignment of the RuntimeStartupStub itself matters. The alignment of what follows after it does not matter.
I do not think that the prosed fix that tries to add the padding after the resource section is good. It does not seem to follow what we do in the other places wrt. alignment of user provided blobs.

I think we should:

Delete the asserts that check alignment of mapped fields and resources sizes. They can be trivially hit by user code passing in misaligned blobs:

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

Line 190 in 1c3b1b5

Debug.Assert(MappedFieldDataSize % MappedFieldDataAlignment == 0);

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

Line 257 in 1c3b1b5

Debug.Assert((resourceBuilderOpt?.Count ?? 0) % 4 == 0);

Optionally, if we believe that the alignment of the resource blob is important (there is no evidence in any spec that it is the case), add the padding before the resource blob similarly how it is added before the field data blob. It may be best to wait and see whether this shows up anywhere.

steveharter · 2025-02-12T22:22:15Z

I do not think that the prosed fix that tries to add the padding after the resource section is good. It does not seem to follow what we do in the other places wrt. alignment of user provided blobs.

My understanding: the resource section has a "ResourcesDirectory" entry (Size + RVA) in the CorHeader so when the actual metadata is written it should aligned so the next section in the CorHeader (StrongNameSignature) is also aligned -- I can verify this, but seems right to me.

The other sections like MappedFieldDataSize are in the "MetadataDirectory" CorHeader entry.

jkotas · 2025-02-12T22:29:54Z

the next section in the CorHeader (StrongNameSignature) is also aligned

StrongNameSignature is a byte array without any structure. These is no reason for it to require any sort of alignment.

If we believe that there are any sections that need to be aligned, we should emit the alignment before emitting that section, like it is done for field data here:

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

Lines 306 to 307 in 07e4c1b

    
           if (mappedFieldDataBuilderOpt.Count != 0) 
        
               builder.Align(MappedFieldDataAlignment);

steveharter · 2025-02-14T20:53:11Z

A new commit removes the padding and the original Asserts.

Padding is not necessary, and due to padding\alignment inconsistencies (dicussed offline) between Roslyn, Cecil and here (System.Reflection.Metadata) it makes sense to not add padding which may be an unnecessarily optimization.

jkotas

Thanks!

* main: (71 commits) Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250212.3 (dotnet#112626) JIT: Unify struct arg morphing (dotnet#112612) Enable `SA1015`: Closing generic bracket should not be followed by a space (dotnet#112597) Clean up normalizeLocale for mono browser target (dotnet#112575) SPMI: Ensure proper zero extension for isObjectImmutable and friends (dotnet#112617) Quote --version-scripts path (dotnet#112603) Remove incompatible API from PKCS netstandard2.0 lib [main] Update dependencies from dotnet/emsdk (dotnet#112393) Avoid `Unsafe.As` in `RangeCharSearchValues` (dotnet#112606) Fixed the issue of incorrect return value of PalVirtualAlloc (dotnet#112579) Fix size used for vectorization check in BitArray (dotnet#111558) (dotnet#111564) Fix build of windows arm64 crossdac (dotnet#112553) Simplify `ShuffleTakeIterator.GetCount` (dotnet#112593) Fix VS div-by-0 in DacEnumerableHashTable code (dotnet#112542) R2RDump: normalize GC info totalInterruptibleLength (dotnet#112003) Fix alignment padding and add test for saving managed resources (dotnet#110915) Adds `ccmp` logic into emitter backend. (dotnet#112153) Disable AVX10.2 by default (dotnet#112572) Outbox AesGcm in to Microsoft.Bcl.Cryptography Make test `IUnknown` conforming (dotnet#112566) ...

Add test for saving and reading managed resources

f21da2f

steveharter added area-System.Reflection.Emit test-enhancement Improvements of test source code labels Dec 23, 2024

steveharter self-assigned this Dec 23, 2024

steveharter commented Dec 23, 2024

View reviewed changes

jkotas reviewed Dec 23, 2024

View reviewed changes

...libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveResourceTests.cs Outdated Show resolved Hide resolved

This was referenced Dec 23, 2024

The hosted runner encountered an error while running your job. (Error Type: Disconnect). dotnet/dnceng#1919

Open

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

steveharter removed the test-enhancement Improvements of test source code label Dec 23, 2024

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

Use TempFile helper for tests; align Resource section

d199ed7

steveharter commented Dec 23, 2024

View reviewed changes

...es/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs Outdated Show resolved Hide resolved

Align just the resource buffer

c011569

build-analysis bot mentioned this pull request Dec 24, 2024

ProcessThreadTests.TestStartTimeProperty failure in CI #105526

Open

Verify ManagedPEBuilder.Serialize() doesn't modify incoming resource …

f857e01

…blob for padding

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 requested a review from ericstj December 24, 2024 16:10

build-analysis bot mentioned this pull request Dec 24, 2024

[wasm] Out of memory exception in System.Xml.XmlWriterApiTests.TCFullEndElement tests #110090

Open

jkotas reviewed Dec 25, 2024

View reviewed changes

ericstj requested review from jkotas and tmat January 7, 2025 00:14

jkotas reviewed Jan 7, 2025

View reviewed changes

...es/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs Outdated Show resolved Hide resolved

Remove Assert; test using LoadContext instead of RemoteExecutor

cb2efb3

build-analysis bot mentioned this pull request Jan 16, 2025

ExplicitConversion_FromSingle failing due to NaN != NaN #103347

Open

steveharter mentioned this pull request Jan 16, 2025

Timeout on HttpClientHandlerTest_Headers.GetAsync_MissingExpires_ReturnNull() #111497

Open

Remove Asserts and padding

e8cbf27

Remove unnecessary using statement

306bfb8

jkotas approved these changes Feb 15, 2025

View reviewed changes

jkotas merged commit 16f2675 into dotnet:main Feb 15, 2025
82 of 86 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

steveharter commented Dec 23, 2024 •

edited

Loading

steveharter Dec 23, 2024

dotnet-policy-service bot commented Dec 23, 2024

jkotas commented Dec 23, 2024

steveharter commented Dec 23, 2024

jkotas Dec 25, 2024

steveharter Jan 10, 2025

tmat Jan 13, 2025 •

edited

Loading

steveharter Jan 15, 2025

steveharter Jan 17, 2025

tmat Jan 17, 2025

steveharter Jan 21, 2025

steveharter commented Jan 17, 2025

steveharter commented Feb 4, 2025

jkotas commented Feb 10, 2025 •

edited

Loading

steveharter commented Feb 12, 2025

jkotas commented Feb 12, 2025

steveharter commented Feb 14, 2025

jkotas left a comment

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

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

Conversation

steveharter commented Dec 23, 2024 • edited Loading

steveharter Dec 23, 2024

Choose a reason for hiding this comment

dotnet-policy-service bot commented Dec 23, 2024

jkotas commented Dec 23, 2024

steveharter commented Dec 23, 2024

jkotas Dec 25, 2024

Choose a reason for hiding this comment

steveharter Jan 10, 2025

Choose a reason for hiding this comment

tmat Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

steveharter Jan 15, 2025

Choose a reason for hiding this comment

steveharter Jan 17, 2025

Choose a reason for hiding this comment

tmat Jan 17, 2025

Choose a reason for hiding this comment

steveharter Jan 21, 2025

Choose a reason for hiding this comment

steveharter commented Jan 17, 2025

steveharter commented Feb 4, 2025

jkotas commented Feb 10, 2025 • edited Loading

steveharter commented Feb 12, 2025

jkotas commented Feb 12, 2025

steveharter commented Feb 14, 2025

jkotas left a comment

Choose a reason for hiding this comment

steveharter commented Dec 23, 2024 •

edited

Loading

tmat Jan 13, 2025 •

edited

Loading

jkotas commented Feb 10, 2025 •

edited

Loading