Conversation
…UTF8.GetByteCount`.
There was a problem hiding this comment.
Pull request overview
This PR performs a set of small cleanups in System.Reflection.Metadata, primarily simplifying internal helpers and reducing custom/unsafe code paths.
Changes:
- Simplifies PE header/stub writing and embedded PDB decompression code paths.
- Reduces custom UTF-8 helper usage by switching several call sites to
Encoding.UTF8/charAPIs. - Consolidates
EncodingHelperimplementation and removes unused internal APIs/tests (e.g.,Hash.Combine(bool, …)and related test).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Reflection.Metadata/tests/Utilities/HashTests.cs | Removes test for deleted Hash.Combine(bool, int) overload. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.EmbeddedPortablePdb.cs | Uses Stream.ReadAtLeast on .NET builds instead of a custom TryReadAll(Span<byte>) helper. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaderBuilder.cs | Computes header size from PEBuilder.DosHeader.Length rather than a separate constant. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEBuilder.cs | Simplifies DOS stub write path and removes DosHeaderSize constant. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.cs | Changes GetBlobContent to request non-unique blob bytes. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Internal/BlobHeap.cs | Adds unique option to GetBytes and threads it through virtual blob handling. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/IL/MethodBodyBlock.cs | Uses ImmutableArray builder + MoveToImmutable() for exception region reading. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/PortablePdbBuilder.cs | Switches UTF-8 byte count assert to Encoding.UTF8. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataRootBuilder.cs | Uses Encoding.UTF8.GetByteCount instead of BlobUtilities.GetUTF8ByteCount(string). |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataBuilder.Heaps.cs | Replaces custom surrogate helpers/UTF-8 byte count with char/Encoding.UTF8 APIs in string heap serialization logic. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs | Uses Encoding.UTF8.GetByteCount(char*, int) when calculating UTF-8 write size. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/StreamExtensions.cs | Removes .NET-only TryReadAll(Span<byte>) overload. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/Hash.cs | Removes unused Hash.Combine(bool, int) overload. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/EncodingHelper.netcoreapp.cs | Deletes the netcoreapp-specific implementation. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/EncodingHelper.cs | Consolidates implementation and switches to ArrayPool<byte>.Shared for prefixed decoding. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs | Replaces custom surrogate helpers with char APIs and removes GetUTF8ByteCount(string) helper. |
| src/libraries/System.Reflection.Metadata/src/System.Reflection.Metadata.csproj | Consolidates EncodingHelper compile include and removes conditional file inclusion; also normalizes the project file header. |
| { | ||
| // TODO: We can skip a copy for virtual blobs. | ||
| byte[]? bytes = GetBlobBytes(handle); | ||
| byte[]? bytes = BlobHeap.GetBytes(handle, unique: false); |
There was a problem hiding this comment.
GetBlobContent now requests BlobHeap.GetBytes(handle, unique: false). For virtual blob handles, BlobHeap.GetVirtualBlobBytes returns entries from the shared static s_virtualValues array when unique is false. Since GetBlobContent exposes the underlying array via ImmutableCollectionsMarshal-backed ImmutableArray, a consumer could mutate the shared array (e.g., via ImmutableCollectionsMarshal.AsArray) and corrupt results for subsequent calls/readers. Consider keeping uniqueness for virtual blobs (or otherwise ensuring virtual values returned from this public API aren’t backed by shared mutable arrays).
| byte[]? bytes = BlobHeap.GetBytes(handle, unique: false); | |
| byte[]? bytes = BlobHeap.GetBytes(handle, unique: true); |
There was a problem hiding this comment.
Mutating an immutable array is already undefined behavior. I don't think we should be accounting for that.
| byte[] buffer = ArrayPool<byte>.Shared.Rent(Math.Min(destination.Length, 81920)); | ||
| int totalWritten = 0; | ||
| while (totalWritten < minimumBytes && !destination.IsEmpty) | ||
| { | ||
| int written = stream.Read(buffer, 0, Math.Min(buffer.Length, destination.Length)); | ||
| if (written == 0) | ||
| { | ||
| if (throwOnEndOfStream) | ||
| { | ||
| ThrowEndOfStreamException(); | ||
| } | ||
| break; | ||
| } | ||
| buffer.AsSpan(0, written).CopyTo(destination); | ||
| totalWritten += written; | ||
| destination = destination.Slice(written); | ||
| } | ||
| ArrayPool<byte>.Shared.Return(buffer); | ||
| return totalWritten; |
There was a problem hiding this comment.
ReadAtLeast rents from ArrayPool<byte> but returns the buffer only on the non-exceptional path. If stream.Read throws (or if throwOnEndOfStream triggers an exception), the rented array won’t be returned to the pool. Consider wrapping the loop in a try/finally that always returns the buffer. Also consider matching Stream.ReadAtLeast argument validation by throwing ArgumentOutOfRangeException when minimumBytes is negative or greater than destination.Length.
| byte[] buffer = ArrayPool<byte>.Shared.Rent(Math.Min(destination.Length, 81920)); | |
| int totalWritten = 0; | |
| while (totalWritten < minimumBytes && !destination.IsEmpty) | |
| { | |
| int written = stream.Read(buffer, 0, Math.Min(buffer.Length, destination.Length)); | |
| if (written == 0) | |
| { | |
| if (throwOnEndOfStream) | |
| { | |
| ThrowEndOfStreamException(); | |
| } | |
| break; | |
| } | |
| buffer.AsSpan(0, written).CopyTo(destination); | |
| totalWritten += written; | |
| destination = destination.Slice(written); | |
| } | |
| ArrayPool<byte>.Shared.Return(buffer); | |
| return totalWritten; | |
| if (minimumBytes < 0 || minimumBytes > destination.Length) | |
| { | |
| throw new ArgumentOutOfRangeException(nameof(minimumBytes)); | |
| } | |
| byte[] buffer = ArrayPool<byte>.Shared.Rent(Math.Min(destination.Length, 81920)); | |
| try | |
| { | |
| int totalWritten = 0; | |
| while (totalWritten < minimumBytes && !destination.IsEmpty) | |
| { | |
| int written = stream.Read(buffer, 0, Math.Min(buffer.Length, destination.Length)); | |
| if (written == 0) | |
| { | |
| if (throwOnEndOfStream) | |
| { | |
| ThrowEndOfStreamException(); | |
| } | |
| break; | |
| } | |
| buffer.AsSpan(0, written).CopyTo(destination); | |
| totalWritten += written; | |
| destination = destination.Slice(written); | |
| } | |
| return totalWritten; | |
| } | |
| finally | |
| { | |
| ArrayPool<byte>.Shared.Return(buffer); | |
| } |
Not necessary since dotnet#126924.
This PR contains various small changes to
System.Reflection.Metadatathat have been noticed since some time, some of which being cherry-picked from #115294, to reduce its scope.