Skip to content
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.

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

[NativeAOT/ARM] Interlocked tests fail #97730

Closed
Tracked by #97729
filipnavara opened this issue Jan 30, 2024 · 8 comments · Fixed by #97743
Closed
Tracked by #97729

[NativeAOT/ARM] Interlocked tests fail #97730

filipnavara opened this issue Jan 30, 2024 · 8 comments · Fixed by #97743

Comments

@filipnavara
Copy link
Member

filipnavara commented Jan 30, 2024

The JIT/Intrinsics/Interlocked_r test running under QEMU ARM user emulation:

/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L87 test failed (expected: equal, actual: 65533-65534).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L92 test failed (expected: equal, actual: -4--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L93 test failed (expected: equal, actual: 65532-65535).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L94 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L101 test failed (expected: equal, actual: 65533-65534).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L106 test failed (expected: equal, actual: -4--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L107 test failed (expected: equal, actual: 65532-65535).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L108 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L110 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L111 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L112 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L113 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L114 test failed (expected: equal, actual: 65531-65535).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L115 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L116 test failed (expected: equal, actual: 65531-65535).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L117 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L151 test failed (expected: equal, actual: -53191--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L153 test failed (expected: equal, actual: -53190--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L155 test failed (expected: equal, actual: -53189--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L157 test failed (expected: equal, actual: -53188--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L160 test failed (expected: equal, actual: -64302--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L162 test failed (expected: equal, actual: -64301--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L164 test failed (expected: equal, actual: -64300--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L166 test failed (expected: equal, actual: -64299--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L169 test failed (expected: equal, actual: -64299--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L171 test failed (expected: equal, actual: -64299--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L173 test failed (expected: equal, actual: -64299--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L175 test failed (expected: equal, actual: -64299--1).
Xunit.Sdk.EqualException: Assert.Equal() Failure: Values differ
Expected: 100
Actual:   128
   at Xunit.Assert.Equal[T](T, T, IEqualityComparer`1) + 0x147
   at Xunit.Assert.Equal[T](T, T) + 0x2b
   at __GeneratedMainWrapper.Main() + 0x29
@ghost
Copy link

ghost commented Jan 30, 2024

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

Issue Details
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L87 test failed (expected: equal, actual: 65533-65534).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L92 test failed (expected: equal, actual: -4--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L93 test failed (expected: equal, actual: 65532-65535).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L94 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L101 test failed (expected: equal, actual: 65533-65534).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L106 test failed (expected: equal, actual: -4--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L107 test failed (expected: equal, actual: 65532-65535).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L108 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L110 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L111 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L112 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L113 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L114 test failed (expected: equal, actual: 65531-65535).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L115 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L116 test failed (expected: equal, actual: 65531-65535).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L117 test failed (expected: equal, actual: -5--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L151 test failed (expected: equal, actual: -53191--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L153 test failed (expected: equal, actual: -53190--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L155 test failed (expected: equal, actual: -53189--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L157 test failed (expected: equal, actual: -53188--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L160 test failed (expected: equal, actual: -64302--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L162 test failed (expected: equal, actual: -64301--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L164 test failed (expected: equal, actual: -64300--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L166 test failed (expected: equal, actual: -64299--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L169 test failed (expected: equal, actual: -64299--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L171 test failed (expected: equal, actual: -64299--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L173 test failed (expected: equal, actual: -64299--1).
/home/navara/runtime/src/tests/JIT/Intrinsics/Interlocked.cs:L175 test failed (expected: equal, actual: -64299--1).
Xunit.Sdk.EqualException: Assert.Equal() Failure: Values differ
Expected: 100
Actual:   128
   at Xunit.Assert.Equal[T](T, T, IEqualityComparer`1) + 0x147
   at Xunit.Assert.Equal[T](T, T) + 0x2b
   at __GeneratedMainWrapper.Main() + 0x29
Author: filipnavara
Assignees: -
Labels:

arch-arm32, area-NativeAOT-coreclr

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 30, 2024
@jkotas
Copy link
Member

jkotas commented Jan 31, 2024

Looks like missing zero extension / sign extension in the assembly helpers added in #97588

@filipnavara
Copy link
Member Author

filipnavara commented Jan 31, 2024

Looks like missing zero extension / sign extension

That was also my first thought. When I read the ARM ABI documentation it seemed to suggest that the caller is responsible for that for arguments passed in the registers (so logically the helpers should be responsible for return value, which could be the missing piece). Clang did generate some zero extension for the 16-bit versions but not for the 8-bit versions.

@jkotas
Copy link
Member

jkotas commented Jan 31, 2024

This: https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/botr/clr-abi.md#small-primitive-returns

@filipnavara
Copy link
Member Author

Thanks for the pointer. I double checked the assembly and both ldrexh and ldrexb are documented to do zero extension, so that part looks okay. I'll have to run it on device under a debugger to see where it fails. At least this is easily reproducible error.

@filipnavara
Copy link
Member Author

FWIW, this seems to fix the issue (and match GCC generated code):

--- a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S
+++ b/src/coreclr/nativeaot/Runtime/arm/Interlocked.S
@@ -36,6 +36,7 @@ LEAF_END RhpLockCmpXchg8, _TEXT
 // r1 = value
 // r2 = comparand
 LEAF_ENTRY RhpLockCmpXchg16, _TEXT
+          uxth    r2, r2
           dmb
 ALTERNATE_ENTRY RhpLockCmpXchg16AVLocation
 LOCAL_LABEL(CmpXchg16Retry):

I am just not sure how to justify the 16-bit argument needing a zero-extension, while the 8-bit one doesn't need it. I need to dig into the ABI docs.

@filipnavara
Copy link
Member Author

Procedure Call Standard for the Arm Architecture, section 6.5 Parameter Passing, states the following:

If the argument is an integral Fundamental Data Type that is smaller than a word, then it is zero- or sign-extended to a full word and its size is set to 4 bytes.

@filipnavara
Copy link
Member Author

Ah... I see... the prototype of the managed version is using short so it gets sign extension (while we need a zero-extension). Conversely, the 8-bit version uses byte which is already unsigned.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 31, 2024
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Jan 31, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants