Skip to content

Make BitArray.CreateArray(ReadOnlySpan<bool>) memory safe - #131838

Draft
EgorBo wants to merge 2 commits into
dotnet:mainfrom
EgorBo:bitarray-safe-createarray
Draft

Make BitArray.CreateArray(ReadOnlySpan<bool>) memory safe#131838
EgorBo wants to merge 2 commits into
dotnet:mainfrom
EgorBo:bitarray-safe-createarray

Conversation

@EgorBo

@EgorBo EgorBo commented Aug 4, 2026

Copy link
Copy Markdown
Member

Makes BitArray.CreateArray(ReadOnlySpan<bool>) memory safe: drops the Unsafe.WriteUnaligned/Unsafe.Add writes (the file no longer uses Unsafe at all) in favor of Vector*.Create(span) + BinaryPrimitives.Write*LittleEndian(span, ...).

Replace the Unsafe.WriteUnaligned/Unsafe.Add writes with span-based
BinaryPrimitives writes. Both spans are tested against a constant and
sliced by that same constant so the JIT elides all bounds checks.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2733b301-fb64-443d-bc8b-ce7bbbbcb941
Copilot AI review requested due to automatic review settings August 4, 2026 23:19
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 4, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the BitArray span-of-bool constructor’s backing implementation to avoid unsafe, unaligned writes by switching to span-based vector loads (Vector*.Create(span)) and safe little-endian stores (BinaryPrimitives.Write*LittleEndian), while preserving the existing scalar fallback behavior.

Changes:

  • Replaces Unsafe.WriteUnaligned + manual ref arithmetic with BinaryPrimitives.WriteUInt{32,64}LittleEndian to write vector-produced bitmasks safely.
  • Reworks the SIMD loop to advance via source = source.Slice(constant) / destination = destination.Slice(constant) and computes the scalar remainder start index from the remaining source span.

@EgorBo

EgorBo commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

@EgorBot -windows_x64 -linux_x64 -arm --filter "Perf_BitArray.BitArrayBoolArrayCtor"

@EgorBo

EgorBo commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

@MihuBot

The bulk loop only runs at the widest available width, so up to 63 bools
could fall into the scalar loop. Follow it with 256/128/64-bit steps that
each run at most once, guarded by a single length check so the exact
multiple case is unaffected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2733b301-fb64-443d-bc8b-ce7bbbbcb941
Copilot AI review requested due to automatic review settings August 5, 2026 00:39
@EgorBo

EgorBo commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

@EgorBot -windows_x64 -linux_x64 -arm --filter "Perf_BitArray.BitArrayBoolArrayCtor"

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/libraries/System.Private.CoreLib/src/System/Collections/BitArray.cs:275

  • The remainder-drain path uses Vector256 operations but only guards with Vector512.IsHardwareAccelerated. Per the repo SIMD guidelines, each vector width used should be guarded by its own IsHardwareAccelerated check to avoid relying on implied ISA relationships.
                    if (Vector512.IsHardwareAccelerated &&
                        source.Length >= Vector256<byte>.Count && destination.Length >= sizeof(uint))
                    {
                        uint isFalse = Vector256.Equals(Vector256.Create(source), Vector256<byte>.Zero).ExtractMostSignificantBits();
                        BinaryPrimitives.WriteUInt32LittleEndian(destination, ~isFalse);

src/libraries/System.Private.CoreLib/src/System/Collections/BitArray.cs:218

  • This block now runs for any little-endian input with length >= 8, even when all Vector*.IsHardwareAccelerated flags are false (e.g., HW intrinsics disabled). That adds extra branching and span setup work to the purely-scalar path; consider gating the block on hardware acceleration.

This issue also appears on line 271 of the same file.

            if (BitConverter.IsLittleEndian && values.Length >= sizeof(ulong))

@joshuajyue

joshuajyue commented Aug 5, 2026

Copy link
Copy Markdown
Member

Hi @EgorBo,
My understanding of unsafe memory is limited but to my knowledge, prior to the regression, BitArray array constructors also used unsafe memory.
Therefore I’m not completely sure if it’s a direct cause of the regressions.

However, maybe this is not the reason for this PR — I just wanted to let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI reduce-unsafe

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants