Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 15, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f21da2f
Add test for saving and reading managed resources
steveharter Dec 23, 2024
d199ed7
Use TempFile helper for tests; align Resource section
steveharter Dec 23, 2024
c011569
Align just the resource buffer
steveharter Dec 23, 2024
f857e01
Verify ManagedPEBuilder.Serialize() doesn't modify incoming resource …
steveharter Dec 24, 2024
cb2efb3
Remove Assert; test using LoadContext instead of RemoteExecutor
steveharter Jan 15, 2025
e8cbf27
Remove Asserts and padding
steveharter Feb 14, 2025
306bfb8
Remove unnecessary using statement
steveharter Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions ...raries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveResourceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@ public void ManagedResources(byte[] byteValue, string byteValueExpected)

// Create a temporary assembly.
string tempFilePath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".dll");
using (FileStream fileStream = new FileStream(tempFilePath, FileMode.Create, FileAccess.Write))
{
blob.WriteContentTo(fileStream);
}
using TempFile file = new TempFile(tempFilePath, blob.ToArray());

// In order to verify the resources work with ResourceManager, we need to load the assembly.
using (RemoteInvokeHandle remoteHandle = RemoteExecutor.Invoke(static (tempFilePath, byteValue, byteValueExpected) =>
Expand Down Expand Up @@ -88,8 +85,6 @@ void Verify(IDictionaryEnumerator resources)
Assert.False(resources.MoveNext());
}
}, tempFilePath, BitConverter.ToString(byteValue), byteValueExpected)) { }

File.Delete(tempFilePath);
}
}
}
2 changes: 1 addition & 1 deletion ...raries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataSizes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace System.Reflection.Metadata.Ecma335
/// </summary>
public sealed class MetadataSizes
{
private const int StreamAlignment = 4;
internal const int StreamAlignment = 4;

// Call the length of the string (including the terminator) m (we require m <= 255);
internal const int MaxMetadataVersionByteCount = 0xff - 1;
Expand Down
21 changes: 12 additions & 9 deletions ...System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Diagnostics;
using System.Reflection.Internal;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;

namespace System.Reflection.PortableExecutable
{
Expand Down Expand Up @@ -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).
Copy link
Member

@jkotas jkotas Dec 25, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@steveharter steveharter Jan 10, 2025

Choose a reason for hiding this comment

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

ping @tmat

Copy link
Member

@tmat tmat Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@steveharter steveharter Jan 15, 2025

Choose a reason for hiding this comment

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

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:

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.

Copy link
Contributor Author

@steveharter steveharter Jan 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@tmat tmat Jan 17, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@steveharter steveharter Jan 21, 2025

Choose a reason for hiding this comment

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

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.

/// When written, will be aligned to <see cref="ManagedResourcesDataAlignment"/>.
/// </summary>
public int ResourceDataSize { get; }

Expand Down Expand Up @@ -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);

return
int offset =
ComputeOffsetToMetadata() +
MetadataSize +
ResourceDataSize +
BitArithmetic.Align(ResourceDataSize, ManagedResourcesDataAlignment) +
StrongNameSignatureSize;

Debug.Assert(offset % MetadataSizes.StreamAlignment == 0);
return offset;
}

private int ComputeOffsetToImportTable()
Expand Down Expand Up @@ -254,7 +257,6 @@ public void Serialize(
Debug.Assert(ilBuilder.Count == ILStreamSize);
Debug.Assert((mappedFieldDataBuilderOpt?.Count ?? 0) == MappedFieldDataSize);
Debug.Assert((resourceBuilderOpt?.Count ?? 0) == ResourceDataSize);
Debug.Assert((resourceBuilderOpt?.Count ?? 0) % 4 == 0);

// TODO: avoid recalculation
int importTableRva = GetImportTableDirectoryEntry(relativeVirtualAddess).RelativeVirtualAddress;
Expand All @@ -278,6 +280,7 @@ public void Serialize(
if (resourceBuilderOpt != null)
{
builder.LinkSuffix(resourceBuilderOpt);
builder.Align(ManagedTextSection.ManagedResourcesDataAlignment);
}

// strong name signature:
Expand Down Expand Up @@ -387,7 +390,7 @@ private void WriteCorHeader(BlobBuilder builder, int textSectionRva, int entryPo

int metadataRva = textSectionRva + ComputeOffsetToMetadata();
int resourcesRva = metadataRva + MetadataSize;
int signatureRva = resourcesRva + ResourceDataSize;
int signatureRva = resourcesRva + BitArithmetic.Align(ResourceDataSize, ManagedResourcesDataAlignment);

int start = builder.Count;

Expand All @@ -410,7 +413,7 @@ private void WriteCorHeader(BlobBuilder builder, int textSectionRva, int entryPo

// ResourcesDirectory:
builder.WriteUInt32((uint)(ResourceDataSize == 0 ? 0 : resourcesRva)); // 28
builder.WriteUInt32((uint)ResourceDataSize);
builder.WriteUInt32((uint)BitArithmetic.Align(ResourceDataSize, ManagedResourcesDataAlignment));

// StrongNameSignatureDirectory:
builder.WriteUInt32((uint)(StrongNameSignatureSize == 0 ? 0 : signatureRva)); // 36
Expand Down
Loading