Skip to content

Commit 2234a3b

Browse files
CopilotEgorBo
andauthored
Add generic And<T> and Or<T> methods to System.Threading.Interlocked (#120978)
## Summary This PR adds generic versions of the `And` and `Or` methods to `System.Threading.Interlocked` as specified in API proposal #114568. ## Changes - [x] Add generic `And<T>(ref T, T) where T : struct` method - [x] Add generic `Or<T>(ref T, T) where T : struct` method - [x] Support integer primitive types and enums backed by integer types - [x] Reject float/double types and floating-point backed enums - [x] Add appropriate error handling and messages - [x] Update reference assembly - [x] Add comprehensive theory-based tests (40+ test methods with 100+ test cases) - [x] Replace existing usages of `Interlocked.And/Or(ref Unsafe.As...)` with new generic API - [x] Restrict JIT intrinsic expansion to TYP_INT and TYP_LONG only - [x] Add TODO comment for small integer intrinsic support - [x] Update XML documentation to clarify floating-point restrictions - [x] Simplify validation logic to single-level if statement - [x] Security checks (CodeQL - no issues) ## Implementation Details - **For 1-2 byte types** (`byte`, `sbyte`, `short`, `ushort`): Uses CompareExchange-based loop (managed fallback) - **For 4-byte types** (`int`, `uint`): Delegates to int And/Or with bitcast (JIT intrinsic) - **For 8-byte types** (`long`, `ulong`): Delegates to long And/Or with bitcast (JIT intrinsic) - **Float/Double**: Rejected via type checks - **Floating-point backed enums**: Rejected via `Type.GetEnumUnderlyingType()` check (JIT-foldable) - **JIT Integration**: Modified to only expand intrinsic for TYP_INT and TYP_LONG; other types fall back to managed - **Pattern**: Follows same approach as generic `CompareExchange<T>` ## JIT Changes Modified `importercalls.cpp` to restrict intrinsic expansion: - Only `TYP_INT` and `TYP_LONG` are expanded as atomic operations - All other types (byte, short, float, double) fall back to managed CompareExchange loops - Uses `callType` consistently for type checks - Added TODO comment: "Implement support for XAND/XORR with small integer types (byte/short)" - Ensures correct behavior across all supported primitive types and enums ## Type Rejection The methods reject: - Non-primitive, non-enum types - Floating-point primitives (float, double) - Enums backed by floating-point types (via `Type.GetEnumUnderlyingType()`) The validation check is a single-level if statement with proper indentation: ```cs if ((!typeof(T).IsPrimitive && !typeof(T).IsEnum) || typeof(T) == typeof(float) || typeof(T) == typeof(double) || (typeof(T).IsEnum && (typeof(T).GetEnumUnderlyingType() == typeof(float) || typeof(T).GetEnumUnderlyingType() == typeof(double)))) { throw new NotSupportedException(SR.NotSupported_IntegerEnumOrPrimitiveTypeRequired); } ``` ## Documentation The XML documentation clearly states the restrictions: ```cs /// <typeparam name="T"> /// The type to be used for <paramref name="location1"/> and <paramref name="value"/>. /// This type must be an integer primitive type or an enum type backed by an integer type. /// Floating-point types (float, double) are not supported. /// </typeparam> ``` ## Test Coverage Converted all tests to `[Theory]` with `[InlineData]` covering: - **Multiple test cases per type**: 5-10 values each including edge cases - **Negative values**: Comprehensive coverage for all signed types (sbyte, short, int, long) - **Edge cases**: Min/max values, zero, all-ones, alternating bit patterns - **Float/Double/Half NotSupportedException tests**: Verify rejection of floating-point types - **Null reference exceptions**: Separate tests for all types - **Unsupported types**: Tests for non-primitive value types ## Usages Replaced Replaced 7 call sites across the codebase: - `System.Private.CoreLib/ComWrappers.cs` (2 calls) - `System.Net.Http/CacheControlHeaderValue.cs` (2 calls) - `System.Private.Uri/Uri.cs` (1 call) - `System.Private.Uri/UriSyntax.cs` (1 call) - `System.Private.Uri/UriScheme.cs` (1 call) All usages now use the cleaner generic API without `Unsafe.As` casts. ## Test Results ✅ All 705 System.Threading tests pass (0 errors, 0 failures) ## Security ✅ CodeQL analysis passed with no issues <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > We have the following public APIs in `System.Threading.Interlocked` class: > > ```cs > public static int Or(ref int location1, int value) > public static uint Or(ref uint location1, uint value) > public static long Or(ref long location1, long value) > public static ulong Or(ref ulong location1, ulong value) > > public static int And(ref int location1, int value) > public static uint And(ref uint location1, uint value) > public static long And(ref long location1, long value) > public static ulong And(ref ulong location1, ulong value) > ``` > We want to add their generic versions: > ``` > public static T Or<T>(ref T location1, T value) where T : struct; > public static T And<T>(ref T location1, T value) where T : struct; > ``` > for the same types + enums backed by those types. > > Similar to what we did for CompareExchange in #104558. > NOTE: this will require a bit of JIT work, e.g. NI_System_Threading_Interlocked_Or and NI_System_Threading_Interlocked_And > > Create a PR in dotnet/runtime to add that. Effectively, implement this API proposal: #114568 </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>
1 parent 0c185bc commit 2234a3b

File tree

10 files changed

+576
-16
lines changed

10 files changed

+576
-16
lines changed

src/coreclr/jit/importercalls.cpp

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4165,16 +4165,23 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd,
41654165
case NI_System_Threading_Interlocked_Or:
41664166
case NI_System_Threading_Interlocked_And:
41674167
{
4168+
#if defined(TARGET_X86)
4169+
// On x86, TYP_LONG is not supported as an intrinsic
4170+
if (genActualType(callType) == TYP_LONG)
4171+
{
4172+
break;
4173+
}
4174+
#endif
4175+
// TODO: Implement support for XAND/XORR with small integer types (byte/short)
4176+
if ((callType != TYP_INT) && (callType != TYP_LONG))
4177+
{
4178+
break;
4179+
}
4180+
41684181
#if defined(TARGET_ARM64)
41694182
if (compOpportunisticallyDependsOn(InstructionSet_Atomics))
41704183
#endif
41714184
{
4172-
#if defined(TARGET_X86)
4173-
if (genActualType(callType) == TYP_LONG)
4174-
{
4175-
break;
4176-
}
4177-
#endif
41784185
assert(sig->numArgs == 2);
41794186
GenTree* op2 = impPopStack().val;
41804187
GenTree* op1 = impPopStack().val;

src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderValue.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,11 @@ private void SetFlag(Flags flag, bool value)
7070
// that concurrent modifications of different properties don't interfere with each other.
7171
if (value)
7272
{
73-
Interlocked.Or(ref Unsafe.As<Flags, int>(ref _flags), (int)flag);
73+
Interlocked.Or(ref _flags, flag);
7474
}
7575
else
7676
{
77-
Interlocked.And(ref Unsafe.As<Flags, int>(ref _flags), (int)~flag);
77+
Interlocked.And(ref _flags, ~flag);
7878
}
7979
}
8080

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4388,6 +4388,9 @@
43884388
<data name="NotSupported_ReferenceEnumOrPrimitiveTypeRequired" xml:space="preserve">
43894389
<value>The specified type must be a reference type, a primitive type, or an enum type.</value>
43904390
</data>
4391+
<data name="NotSupported_IntegerEnumOrPrimitiveTypeRequired" xml:space="preserve">
4392+
<value>The specified type must be an integer primitive type or an enum type.</value>
4393+
</data>
43914394
<data name="Argument_BadFieldForInitializeArray" xml:space="preserve">
43924395
<value>The field is invalid for initializing array or span.</value>
43934396
</data>

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -444,14 +444,12 @@ private IntPtr AsUserDefined(in Guid riid)
444444

445445
private void SetFlag(CreateComInterfaceFlagsEx flag)
446446
{
447-
int setMask = (int)flag;
448-
Interlocked.Or(ref Unsafe.As<CreateComInterfaceFlagsEx, int>(ref Flags), setMask);
447+
Interlocked.Or(ref Flags, flag);
449448
}
450449

451450
private void ResetFlag(CreateComInterfaceFlagsEx flag)
452451
{
453-
int resetMask = ~(int)flag;
454-
Interlocked.And(ref Unsafe.As<CreateComInterfaceFlagsEx, int>(ref Flags), resetMask);
452+
Interlocked.And(ref Flags, ~flag);
455453
}
456454

457455
private static uint GetTrackerCount(ulong c)

src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,83 @@ public static long And(ref long location1, long value)
634634
[CLSCompliant(false)]
635635
public static ulong And(ref ulong location1, ulong value) =>
636636
(ulong)And(ref Unsafe.As<ulong, long>(ref location1), (long)value);
637+
638+
/// <summary>Bitwise "ands" two values of type <typeparamref name="T"/> and replaces the first value with the result, as an atomic operation.</summary>
639+
/// <param name="location1">A variable containing the first value to be combined. The result is stored in <paramref name="location1"/>.</param>
640+
/// <param name="value">The value to be combined with the value at <paramref name="location1"/>.</param>
641+
/// <returns>The original value in <paramref name="location1"/>.</returns>
642+
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
643+
/// <exception cref="NotSupportedException">An unsupported <typeparamref name="T"/> is specified.</exception>
644+
/// <typeparam name="T">
645+
/// The type to be used for <paramref name="location1"/> and <paramref name="value"/>.
646+
/// This type must be an integer primitive type or an enum type backed by an integer type.
647+
/// Floating-point types (float, double) are not supported.
648+
/// </typeparam>
649+
[Intrinsic]
650+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
651+
public static unsafe T And<T>(ref T location1, T value) where T : struct
652+
{
653+
// Only integer primitive types and enum types backed by integer types are supported.
654+
// Floating-point types and floating-point backed enums are not supported.
655+
if ((!typeof(T).IsPrimitive && !typeof(T).IsEnum) ||
656+
typeof(T) == typeof(float) || typeof(T) == typeof(double) ||
657+
(typeof(T).IsEnum && (typeof(T).GetEnumUnderlyingType() == typeof(float) || typeof(T).GetEnumUnderlyingType() == typeof(double))))
658+
{
659+
throw new NotSupportedException(SR.NotSupported_IntegerEnumOrPrimitiveTypeRequired);
660+
}
661+
662+
// For 1-byte and 2-byte types, we need to use CompareExchange-based implementations
663+
// because there are no direct atomic And operations for these sizes.
664+
if (sizeof(T) == 1)
665+
{
666+
byte current = Unsafe.BitCast<T, byte>(location1);
667+
while (true)
668+
{
669+
byte newValue = (byte)(current & Unsafe.BitCast<T, byte>(value));
670+
byte oldValue = CompareExchange(
671+
ref Unsafe.As<T, byte>(ref location1),
672+
newValue,
673+
current);
674+
if (oldValue == current)
675+
{
676+
return Unsafe.BitCast<byte, T>(oldValue);
677+
}
678+
current = oldValue;
679+
}
680+
}
681+
682+
if (sizeof(T) == 2)
683+
{
684+
ushort current = Unsafe.BitCast<T, ushort>(location1);
685+
while (true)
686+
{
687+
ushort newValue = (ushort)(current & Unsafe.BitCast<T, ushort>(value));
688+
ushort oldValue = CompareExchange(
689+
ref Unsafe.As<T, ushort>(ref location1),
690+
newValue,
691+
current);
692+
if (oldValue == current)
693+
{
694+
return Unsafe.BitCast<ushort, T>(oldValue);
695+
}
696+
current = oldValue;
697+
}
698+
}
699+
700+
if (sizeof(T) == 4)
701+
{
702+
return Unsafe.BitCast<int, T>(
703+
And(
704+
ref Unsafe.As<T, int>(ref location1),
705+
Unsafe.BitCast<T, int>(value)));
706+
}
707+
708+
Debug.Assert(sizeof(T) == 8);
709+
return Unsafe.BitCast<long, T>(
710+
And(
711+
ref Unsafe.As<T, long>(ref location1),
712+
Unsafe.BitCast<T, long>(value)));
713+
}
637714
#endregion
638715

639716
#region Or
@@ -700,6 +777,83 @@ public static long Or(ref long location1, long value)
700777
[CLSCompliant(false)]
701778
public static ulong Or(ref ulong location1, ulong value) =>
702779
(ulong)Or(ref Unsafe.As<ulong, long>(ref location1), (long)value);
780+
781+
/// <summary>Bitwise "ors" two values of type <typeparamref name="T"/> and replaces the first value with the result, as an atomic operation.</summary>
782+
/// <param name="location1">A variable containing the first value to be combined. The result is stored in <paramref name="location1"/>.</param>
783+
/// <param name="value">The value to be combined with the value at <paramref name="location1"/>.</param>
784+
/// <returns>The original value in <paramref name="location1"/>.</returns>
785+
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
786+
/// <exception cref="NotSupportedException">An unsupported <typeparamref name="T"/> is specified.</exception>
787+
/// <typeparam name="T">
788+
/// The type to be used for <paramref name="location1"/> and <paramref name="value"/>.
789+
/// This type must be an integer primitive type or an enum type backed by an integer type.
790+
/// Floating-point types (float, double) are not supported.
791+
/// </typeparam>
792+
[Intrinsic]
793+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
794+
public static unsafe T Or<T>(ref T location1, T value) where T : struct
795+
{
796+
// Only integer primitive types and enum types backed by integer types are supported.
797+
// Floating-point types and floating-point backed enums are not supported.
798+
if ((!typeof(T).IsPrimitive && !typeof(T).IsEnum) ||
799+
typeof(T) == typeof(float) || typeof(T) == typeof(double) ||
800+
(typeof(T).IsEnum && (typeof(T).GetEnumUnderlyingType() == typeof(float) || typeof(T).GetEnumUnderlyingType() == typeof(double))))
801+
{
802+
throw new NotSupportedException(SR.NotSupported_IntegerEnumOrPrimitiveTypeRequired);
803+
}
804+
805+
// For 1-byte and 2-byte types, we need to use CompareExchange-based implementations
806+
// because there are no direct atomic Or operations for these sizes.
807+
if (sizeof(T) == 1)
808+
{
809+
byte current = Unsafe.BitCast<T, byte>(location1);
810+
while (true)
811+
{
812+
byte newValue = (byte)(current | Unsafe.BitCast<T, byte>(value));
813+
byte oldValue = CompareExchange(
814+
ref Unsafe.As<T, byte>(ref location1),
815+
newValue,
816+
current);
817+
if (oldValue == current)
818+
{
819+
return Unsafe.BitCast<byte, T>(oldValue);
820+
}
821+
current = oldValue;
822+
}
823+
}
824+
825+
if (sizeof(T) == 2)
826+
{
827+
ushort current = Unsafe.BitCast<T, ushort>(location1);
828+
while (true)
829+
{
830+
ushort newValue = (ushort)(current | Unsafe.BitCast<T, ushort>(value));
831+
ushort oldValue = CompareExchange(
832+
ref Unsafe.As<T, ushort>(ref location1),
833+
newValue,
834+
current);
835+
if (oldValue == current)
836+
{
837+
return Unsafe.BitCast<ushort, T>(oldValue);
838+
}
839+
current = oldValue;
840+
}
841+
}
842+
843+
if (sizeof(T) == 4)
844+
{
845+
return Unsafe.BitCast<int, T>(
846+
Or(
847+
ref Unsafe.As<T, int>(ref location1),
848+
Unsafe.BitCast<T, int>(value)));
849+
}
850+
851+
Debug.Assert(sizeof(T) == 8);
852+
return Unsafe.BitCast<long, T>(
853+
Or(
854+
ref Unsafe.As<T, long>(ref location1),
855+
Unsafe.BitCast<T, long>(value)));
856+
}
703857
#endregion
704858

705859
#region MemoryBarrier

src/libraries/System.Private.Uri/src/System/Uri.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ private void InterlockedSetFlags(Flags flags)
252252
{
253253
// For built-in (simple) parsers, it is safe to do an Interlocked update here
254254
Debug.Assert(sizeof(Flags) == sizeof(ulong));
255-
Interlocked.Or(ref Unsafe.As<Flags, ulong>(ref _flags), (ulong)flags);
255+
Interlocked.Or(ref _flags, flags);
256256
}
257257
else
258258
{

src/libraries/System.Private.Uri/src/System/UriScheme.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ protected virtual void InitializeAndValidate(Uri uri, out UriFormatException? pa
7777
Debug.Assert(sizeof(Uri.Flags) == sizeof(ulong));
7878

7979
// If ParseMinimal is called multiple times this Uri instance may be corrupted, throw an exception instead
80-
ulong previous = Interlocked.Or(ref Unsafe.As<Uri.Flags, ulong>(ref uri._flags), (ulong)Uri.Flags.CustomParser_ParseMinimalAlreadyCalled);
81-
if (((Uri.Flags)previous & Uri.Flags.CustomParser_ParseMinimalAlreadyCalled) != 0)
80+
Uri.Flags previous = Interlocked.Or(ref uri._flags, Uri.Flags.CustomParser_ParseMinimalAlreadyCalled);
81+
if ((previous & Uri.Flags.CustomParser_ParseMinimalAlreadyCalled) != 0)
8282
{
8383
throw new InvalidOperationException(SR.net_uri_InitializeCalledAlreadyOrTooLate);
8484
}

src/libraries/System.Private.Uri/src/System/UriSyntax.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ internal void InternalValidate(Uri thisUri, out UriFormatException? parsingError
269269

270270
// InitializeAndValidate should not be called outside of the constructor
271271
Debug.Assert(sizeof(Uri.Flags) == sizeof(ulong));
272-
Interlocked.Or(ref Unsafe.As<Uri.Flags, ulong>(ref thisUri._flags), (ulong)Uri.Flags.CustomParser_ParseMinimalAlreadyCalled);
272+
Interlocked.Or(ref thisUri._flags, Uri.Flags.CustomParser_ParseMinimalAlreadyCalled);
273273
}
274274

275275
internal string? InternalResolve(Uri thisBaseUri, Uri uriLink, out UriFormatException? parsingError)

src/libraries/System.Threading/ref/System.Threading.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,7 @@ public static partial class Interlocked
255255
public static uint And(ref uint location1, uint value) { throw null; }
256256
[System.CLSCompliantAttribute(false)]
257257
public static ulong And(ref ulong location1, ulong value) { throw null; }
258+
public static T And<T>(ref T location1, T value) where T : struct { throw null; }
258259
public static double CompareExchange(ref double location1, double value, double comparand) { throw null; }
259260
public static byte CompareExchange(ref byte location1, byte value, byte comparand) { throw null; }
260261
[System.CLSCompliantAttribute(false)]
@@ -317,6 +318,7 @@ public static void MemoryBarrierProcessWide() { }
317318
public static uint Or(ref uint location1, uint value) { throw null; }
318319
[System.CLSCompliantAttribute(false)]
319320
public static ulong Or(ref ulong location1, ulong value) { throw null; }
321+
public static T Or<T>(ref T location1, T value) where T : struct { throw null; }
320322
public static long Read(ref readonly long location) { throw null; }
321323
[System.CLSCompliantAttribute(false)]
322324
public static ulong Read(ref readonly ulong location) { throw null; }

0 commit comments

Comments
 (0)