Remove a few unsafe blocks from Decimal.DecCalc.cs by EgorBo · Pull Request #131972 · dotnet/runtime

EgorBo · 2026-08-06T22:19:35Z

No description provided.

…afe code A decimal reinterpreted from arbitrary bytes (e.g. MemoryMarshal.Read<decimal>) can carry a scale factor larger than DEC_SCALE_MAX. DecAddSub sizes its Buf24 scaling buffer for valid scales only (96 bits scaled by 10^28 needs 189 bits), so such an operand makes the scaling loop write past the end of the stack buffer. Adding 1m to a decimal built with scale 58 corrupts the stack, and scale 78 faults: Span<byte> raw = stackalloc byte[16]; raw[2] = 58; raw[8] = 1; decimal d = MemoryMarshal.Read<decimal>(raw); decimal r = d + 1m; // OOB write / AccessViolationException Replace the raw pointer arithmetic in DecCalc with bounds checked spans and bound the buffer growth explicitly, throwing OverflowException (consistent with the rest of DecCalc) instead of running off the buffer. This removes the unsafe modifier from DecAddSub, ScaleResult, DivByConst, VarDecMul and VarDecModFull: * Buf24/Buf28 expose AsSpan(); the constant length lets the JIT keep the hot scaling and division loops bounds check free. * ScaleResult takes ref Buf24 instead of Buf24*. * Buf28 gains explicit layout Buf12/Buf16 windows (U0To2..U3To6), matching the existing Buf16.Low96/High96 idiom, replacing the *(Buf12*)&b casts. * The 32-bit DivByConst path uses endian independent shifts instead of byte*/ushort* arithmetic, dropping the BIGENDIAN special case. Three changes keep the codegen at or better than the pointer based version, each verified with perf record and JitDisasm on linux-x64: * ScaleResult checks hiRes against Buf24.Length on entry and at the top of the scaling loop. Without them the JIT emits a bounds check in each of the nine DivByConst specializations, growing the method past the loop alignment budget and costing ~10% on decimal multiply. * Buf24/Buf28.AsSpan() use Unsafe.As rather than "ref U0" so the JIT does not null check the byref when ScaleResult is called with ref Buf24. * VarDecMul jumps straight to ScaleResult on the SkipScan path, which is only reached with hiProd == 3 and therefore always needs scaling. Behavior for valid decimals is unchanged: a differential run over 5.8M random + - * / % and Math.Round operations produces an identical result hash on the baseline and patched builds, and System.Runtime.Tests passes (77605 tests, 0 failures). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44cd33c7-4868-4450-85cf-dc9aaa4d7032

Explicit layout structs need an explicit safe or unsafe keyword on every field. Two places were still missing it: the BIGENDIAN ulo/umid pair in DecCalc, which already had its <safety> docs, and the Buf28 windows added by the previous commit, which get both the keyword and the docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44cd33c7-4868-4450-85cf-dc9aaa4d7032

azure-pipelines · 2026-08-06T22:20:17Z

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.

EgorBo · 2026-08-06T22:22:44Z

@EgorBot -arm -amd -linux

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(DecimalBench).Assembly).Run(args);

public class DecimalBench
{
    private decimal _a1 = 12345678901234567890.12345678m;
    private decimal _a2 = 98765432109876543210.87654321m;
    private decimal _small1 = 1234567.89m;
    private decimal _small2 = 9876.54321m;

    // scale 0 + scale 28: the left operand is scaled by 10^28 (189 bits),
    // so DecAddSub takes the Buf24 scaling loop + ScaleResult
    private decimal _big = 7922816251426433759354395033m;
    private decimal _tiny = 0.0000000000000000000000000009m;
    private decimal _negTiny = -0.0000000000000000000000000009m;

    // low uints are zero, so the carry ripples all the way up the buffer
    private decimal _max = 79228162514264337593543950335m;
    private decimal _ulp = 0.0000000000000000000000000001m;

    private decimal _mulSmall1 = 12345m;
    private decimal _mulSmall2 = 6789m;
    private decimal _mulMid1 = 123456789.123456789m;
    private decimal _mulMid2 = 987654321.987654321m;

    private decimal _divisor = 1234567.891011m;
    private decimal _mod96 = 18446744073709551617m;          // > 2^64 -> Div96By64 path
    private decimal _mod128 = 340282366920938463463374607m;  // High set -> Div128By96 path
    private decimal _modScaled = 1844674407370955161.7000000m;

    private decimal _round = 79228162514264337593.543950335m;

    // Unchanged fast paths (control)
    [Benchmark] public decimal AddEqualScale() => _a1 + _a2;
    [Benchmark] public decimal SubEqualScale() => _a1 - _a2;
    [Benchmark] public decimal AddSmallScaleDelta() => _small1 + _small2;

    // DecAddSub: Buf24 scaling loop + ScaleResult
    [Benchmark] public decimal AddBigScaleDelta() => _big + _tiny;
    [Benchmark] public decimal SubBigScaleDelta() => _big - _negTiny;
    [Benchmark] public decimal AddBigScaleDeltaCarry() => _max + _ulp;

    // VarDecMul (+ ScaleResult / DivByConst for the wider ones)
    [Benchmark] public decimal MulSmall() => _mulSmall1 * _mulSmall2;
    [Benchmark] public decimal MulMid() => _mulMid1 * _mulMid2;
    [Benchmark] public decimal MulBig() => _max * _tiny;

    // VarDecModFull
    [Benchmark] public decimal ModFull96By64() => _max % _mod96;
    [Benchmark] public decimal ModFull128By96() => _max % _mod128;
    [Benchmark] public decimal ModFullScaled() => _max % _modScaled;

    // Unchanged (control)
    [Benchmark] public decimal Div() => _max / _divisor;
    [Benchmark] public decimal Round() => Math.Round(_round, 4);

    [Benchmark]
    public decimal MixedRealistic()
    {
        decimal a = _a1 + _a2;
        a -= _small1;
        a *= 1.05m;
        return a;
    }
}

Note

This comment (including the benchmark above) was generated by GitHub Copilot.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

dotnet-policy-service · 2026-08-06T23:16:58Z

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

EgorBo · 2026-08-06T23:29:10Z

@EgorBot -arm -amd -linux --filter "System.Tests.Perf_Decimal*"

azure-pipelines · 2026-08-07T11:12:36Z

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.

tannergooding · 2026-08-07T13:37:54Z

+                public Span<uint> AsSpan() => MemoryMarshal.CreateSpan(ref Unsafe.As<Buf24, uint>(ref this), Length);
            }

+            [StructLayout(LayoutKind.Explicit)]


Can we look at removing the explicit layout rather than introducing more?

This is still "unsafe" in that not all bit values are valid for a decimal and it is going to hinder promotion, enregistration, and other scenarios.

I'd much rather see us use some InlineArray + slice if we really need to have such windows, but I think we can in fact remove them all without losing perf.

tannergooding · 2026-08-07T13:38:08Z

+                // Unsafe.As instead of "ref U0" keeps the JIT from null checking the byref when this
+                // is called on a ScaleResult style "ref Buf24" parameter; U0 is at offset 0.
+                [UnscopedRef]
+                public Span<uint> AsSpan() => MemoryMarshal.CreateSpan(ref Unsafe.As<Buf24, uint>(ref this), Length);


This is notably going to require unsafe once the annotations are done.

Yep, had to keep this one for perf reasons. But at least the resulting span guards from out of bounds access vs previous code.

…oops * VarDecModFull no longer needs the overlapping Buf12/Buf16 fields on Buf28, so Buf28 and its explicit layout are gone entirely. The dividend is now an InlineArray7<uint> and the division windows come from a small Window<T> helper that range checks the window against the buffer before aliasing it. The offsets are constants, so the check folds away and the emitted code matches the previous version: same 9 call sites, 592 vs 535 bytes. Aliasing rather than copying matters here. Carrying the window in a local and copying it back cost 1.7x on decimal remainder, and seeding the buffer with uint stores instead of 64-bit ones cost another 15% because the 64-bit reads inside the division helpers then overlap several narrower stores and lose store to load forwarding. * Replaced the `x++ == 0` style loops in the DecAddSub carry/borrow propagation and in the ScaleResult rounding carry with explicit statements, so the side effect is no longer hidden inside the loop condition. * Dropped the goto that was added to VarDecMul; the file is back to the same number of gotos as before this PR. Differential hashes over 5.8M random operations and 210K remainder specific cases (including the maximum scaling cases that push the dividend to 221 bits) are unchanged, and System.Runtime.Tests passes (77605 tests, 0 failures). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44cd33c7-4868-4450-85cf-dc9aaa4d7032

Copilot

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs:2286

The bounds check in Window can be bypassed for negative index values due to unsigned wraparound in (uint)index + ..., and the Unsafe.Add(..., (uint)index) then indexes far outside the span. Even though current call sites pass constants, this helper is meant to be the safety gate for the reinterpret cast, so it should be correct for all int inputs. Also consider constraining TWindow to unmanaged since the method reinterprets raw uint storage as TWindow.

                // Checked rather than asserted: this is what keeps the reinterpret below in bounds.
                if ((uint)index + (uint)(Unsafe.SizeOf<TWindow>() / sizeof(uint)) > (uint)buf.Length)
                    Number.ThrowDecimalOverflowException();

                return ref Unsafe.As<uint, TWindow>(ref Unsafe.Add(ref MemoryMarshal.GetReference(buf), (uint)index));

EgorBo · 2026-08-07T23:07:04Z

@EgorBot -arm -amd -linux

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(DecimalBench).Assembly).Run(args);

public class DecimalBench
{
    private decimal _a1 = 12345678901234567890.12345678m;
    private decimal _a2 = 98765432109876543210.87654321m;
    private decimal _small1 = 1234567.89m;
    private decimal _small2 = 9876.54321m;

    // scale 0 + scale 28: the left operand is scaled by 10^28 (189 bits),
    // so DecAddSub takes the Buf24 scaling loop + ScaleResult
    private decimal _big = 7922816251426433759354395033m;
    private decimal _tiny = 0.0000000000000000000000000009m;
    private decimal _negTiny = -0.0000000000000000000000000009m;

    // low uints are zero, so the carry ripples all the way up the buffer
    private decimal _max = 79228162514264337593543950335m;
    private decimal _ulp = 0.0000000000000000000000000001m;

    private decimal _mulSmall1 = 12345m;
    private decimal _mulSmall2 = 6789m;
    private decimal _mulMid1 = 123456789.123456789m;
    private decimal _mulMid2 = 987654321.987654321m;

    private decimal _divisor = 1234567.891011m;
    private decimal _mod96 = 18446744073709551617m;          // > 2^64 -> Div96By64 path
    private decimal _mod128 = 340282366920938463463374607m;  // High set -> Div128By96 path
    private decimal _modScaled = 1844674407370955161.7000000m;

    private decimal _round = 79228162514264337593.543950335m;

    // Unchanged fast paths (control)
    [Benchmark] public decimal AddEqualScale() => _a1 + _a2;
    [Benchmark] public decimal SubEqualScale() => _a1 - _a2;
    [Benchmark] public decimal AddSmallScaleDelta() => _small1 + _small2;

    // DecAddSub: Buf24 scaling loop + ScaleResult
    [Benchmark] public decimal AddBigScaleDelta() => _big + _tiny;
    [Benchmark] public decimal SubBigScaleDelta() => _big - _negTiny;
    [Benchmark] public decimal AddBigScaleDeltaCarry() => _max + _ulp;

    // VarDecMul (+ ScaleResult / DivByConst for the wider ones)
    [Benchmark] public decimal MulSmall() => _mulSmall1 * _mulSmall2;
    [Benchmark] public decimal MulMid() => _mulMid1 * _mulMid2;
    [Benchmark] public decimal MulBig() => _max * _tiny;

    // VarDecModFull
    [Benchmark] public decimal ModFull96By64() => _max % _mod96;
    [Benchmark] public decimal ModFull128By96() => _max % _mod128;
    [Benchmark] public decimal ModFullScaled() => _max % _modScaled;

    // Unchanged (control)
    [Benchmark] public decimal Div() => _max / _divisor;
    [Benchmark] public decimal Round() => Math.Round(_round, 4);

    [Benchmark]
    public decimal MixedRealistic()
    {
        decimal a = _a1 + _a2;
        a -= _small1;
        a *= 1.05m;
        return a;
    }
}

Note

This comment (including the benchmark above) was generated by GitHub Copilot.

EgorBo · 2026-08-10T21:17:45Z

@tannergooding I've addressed your feedback while maintaining the same perf, anything else here?

EgorBo · 2026-08-10T21:22:08Z

Actually, wait, I pushed the wrong code to Window (unsafe) :|

The previous commit swapped the *(Buf12*)&buf[i] casts for Unsafe.As, which respells the reinterpret without making it any harder to get wrong. Use MemoryMarshal.Cast over a slice instead: it derives the destination length from the source length so it cannot address outside the buffer, and it rejects types holding managed references. The emitted code is unchanged, 592 bytes with the same 9 call sites and no bounds checks left after the constant offsets fold. The windows do have to alias the dividend. Long division needs overlapping views and gets the shift between steps for free by moving the window, so keeping the window in registers and shifting it explicitly costs 1.7x on decimal remainder even with the loop fully unrolled. Differential hashes over 5.8M random operations and 210K remainder specific cases are unchanged, and System.Runtime.Tests passes (77605 tests, 0 failures). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44cd33c7-4868-4450-85cf-dc9aaa4d7032

Copilot

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/Decimal.DecCalc.cs:2310

VarDecModFull now stores the dividend in InlineArray7<uint> and then reinterprets part of it as Buf24 to use the 64-bit Low64/Mid64 views. InlineArray7<uint> only guarantees 4-byte alignment, so on little-endian targets the ulong overlays in Buf24 can become unaligned (potentially problematic on 32-bit ARM and other alignment-sensitive platforms). Consider backing the buffer with an 8-byte-aligned element type and casting to Span<uint> so the Buf24 overlay remains naturally aligned.

                Unsafe.SkipInit(out InlineArray7<uint> bufNum);
                Span<uint> buf = bufNum;
                Debug.Assert(buf.Length == BufLength);

src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs:1128

In the scaled subtraction path, the borrow-propagation loop (while (rgulNum[cur] == 0) { ... cur++; }) has no bounds check before indexing rgulNum[cur]. If the borrow ever propagates beyond the buffer (e.g., due to an out-of-range decimal bit pattern), this will throw IndexOutOfRangeException rather than the expected decimal overflow. Add an explicit bounds check before indexing so failure stays consistent and intentional.

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

                        int cur = 3;
                        while (rgulNum[cur] == 0)
                        {
                            // Borrowing from a zero uint wraps it and keeps the borrow going.
                            rgulNum[cur] = uint.MaxValue;

The uint count argument was redundant: MemoryMarshal.Cast already derives the destination length from the source, so slicing to the exact window size added nothing that the cast did not already do. Drop it and make the helper an expression body, which leaves the call sites as Window<Buf12>(buf, index). Generated code is unchanged, 592 bytes with the same call sites, and the helper still inlines away completely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44cd33c7-4868-4450-85cf-dc9aaa4d7032

Copilot

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/Decimal.DecCalc.cs:1129

This borrow-propagation loop can advance past the end of the 192-bit buffer if the internal decimal state is malformed (e.g., all higher limbs are zero while a borrow is required). With Span indexing that becomes an IndexOutOfRangeException; it should consistently throw Number.ThrowDecimalOverflowException() like the other buffer-growth/overflow guards added in this method.

                        while (rgulNum[cur] == 0)
                        {
                            // Borrowing from a zero uint wraps it and keeps the borrow going.
                            rgulNum[cur] = uint.MaxValue;
                            cur++;

src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs:739

In release builds this carry-propagation loop can run off the end of the 192-bit buffer and throw IndexOutOfRangeException (Span bounds check) rather than the intended decimal overflow. Since this path can be reached when processing a decimal with out-of-range internal state, add an explicit bounds check and throw Number.ThrowDecimalOverflowException() before indexing past Buf24.Length.

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

                                do
                                {
                                    cur++;
                                    Debug.Assert(cur < Buf24.Length);
                                    result[cur]++;

Buf24 was an explicit layout struct overlapping six uints with three ulongs. Make it an [InlineArray(6)] of uint: the length is statically known to the JIT, U0..U5 become plain indexing, and the 64-bit views become MemoryMarshal.Cast over the uint span, which derives its length from the source instead of relying on hand written field offsets. That leaves Buf12 and Buf16 as the only explicit layout buffers, both unchanged by this PR. AsSpan keeps reinterpreting by hand. The compiler's inline array to span conversion null checks the byref when it runs on a "ref Buf24" parameter such as ScaleResult's, and the extra code also pushes four loops out of alignment, which together cost about 9% on decimal multiply. Differential hashes over 5.8M random operations and 210K remainder specific cases are unchanged, and System.Runtime.Tests passes (77605 tests, 0 failures). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44cd33c7-4868-4450-85cf-dc9aaa4d7032

Copilot

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs:2332

When the dividend scaling loop needs to grow beyond the 7-uint buffer, this currently relies on Debug.Assert and then indexes buf[++high]. In release builds this would throw an IndexOutOfRangeException (from Span bounds checks) rather than the intended DecimalOverflowException, and it also increments high before the failure. Add an explicit bounds check and throw Number.ThrowDecimalOverflowException() to keep behavior consistent with other buffer-growth paths in this file.

                    // The high bit of the dividend must not be set.
                    if (tmp64 > int.MaxValue)
                    {
                        Debug.Assert(high + 1 < BufLength);
                        buf[++high] = (uint)(tmp64 >> 32);
                    }

EgorBo and others added 2 commits August 6, 2026 23:54

Copilot AI lite review requested due to automatic review settings August 6, 2026 22:19

EgorBo added the reduce-unsafe label Aug 6, 2026

dotnet-policy-service Bot assigned EgorBo Aug 6, 2026

EgorBot mentioned this pull request Aug 6, 2026

Benchmarks for dotnet/runtime#131972 (for @EgorBo) EgorBot/Benchmarks#462

Open

Copilot AI reviewed Aug 6, 2026

github-actions Bot added the area-System.Runtime label Aug 6, 2026

EgorBot mentioned this pull request Aug 6, 2026

Benchmarks for dotnet/runtime#131972 (for @EgorBo) EgorBot/Benchmarks#464

Open

Copilot started reviewing on behalf of EgorBo August 6, 2026 23:33 View session

build-analysis Bot mentioned this pull request Aug 7, 2026

Android WebSocket failure #121518

Open

EgorBo marked this pull request as ready for review August 7, 2026 11:11

tannergooding reviewed Aug 7, 2026

View reviewed changes

Comment thread src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs Outdated

tannergooding reviewed Aug 7, 2026

View reviewed changes

Comment thread src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs Outdated

tannergooding reviewed Aug 7, 2026

View reviewed changes

Comment thread src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs

Copilot AI review requested due to automatic review settings August 7, 2026 15:35

Copilot started reviewing on behalf of EgorBo August 7, 2026 15:36 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

EgorBot mentioned this pull request Aug 7, 2026

Benchmarks for dotnet/runtime#131972 (for @EgorBo) EgorBot/Benchmarks#468

Open

build-analysis Bot mentioned this pull request Aug 8, 2026

HttpWebRequestTest_Async.SendHttpRequest_BindIPEndPoint_Success failure from too many BindIPEndPointDelegate retries #131951

Open

Copilot AI review requested due to automatic review settings August 10, 2026 22:08

Copilot started reviewing on behalf of EgorBo August 10, 2026 22:09 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 10, 2026 23:21

Copilot started reviewing on behalf of EgorBo August 10, 2026 23:22 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

tannergooding self-requested a review August 11, 2026 05:12

Copilot AI review requested due to automatic review settings August 11, 2026 17:29

Copilot started reviewing on behalf of EgorBo August 11, 2026 17:30 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Uh oh!

Conversation

EgorBo commented Aug 6, 2026

Uh oh!

azure-pipelines Bot commented Aug 6, 2026

Uh oh!

EgorBo commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

dotnet-policy-service Bot commented Aug 6, 2026

Uh oh!

EgorBo commented Aug 6, 2026

Uh oh!

azure-pipelines Bot commented Aug 7, 2026

Uh oh!

tannergooding Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

tannergooding Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

EgorBo Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

EgorBo commented Aug 7, 2026

Uh oh!

EgorBo commented Aug 10, 2026

Uh oh!

EgorBo commented Aug 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants