[RISC-V][LoongArch64] Pass structs containing empty struct arrays according to integer calling convention #106266

tomeksowi · 2024-08-12T13:20:30Z

A note from RISC-V Hardware Floating-point Calling Convention:

One exceptional case for the flattening rule is an array of empty structs or unions;
C treats it as an empty field, but C++ treats it as a non-empty field since C++ defines
the size of an empty struct or union as 1. i.e. for struct { struct {} e[1]; float f;
} as the first argument, C will treat it like struct { float f; } and pass f in fa0 as
described below, whereas C++ will pass the pass the entire aggregate in a0 (XLEN =
64) or a0 and a1 (XLEN = 32), as described in the integer calling convention. Zerolength
arrays of empty structs or union will be ignored for both C and C++. i.e. For
struct { struct {} e[0]; float f; };, as the first argument, C and C++ will treat it
like struct { float f; } and pass f in fa0 as described below.

@LuckyXu-HF @shushanhf LoongArch ABI doesn't seem to mention it but from what I could discern GCC behaves the same way so I left it in for LA as well.

Plus, some fixes to the EmptyStructs test code.

Stems from #101796, part of #84834, cc @dotnet/samsung

…re floating-point calling convention

…o_DoubleFloatNestedEmpty_InIntegerRegs_RiscV

… Equals from object

risc-vv · 2024-08-13T11:46:38Z

RISC-V Release-CLR-QEMU: 9389 / 9390 (99.99%)

=======================
      passed: 9389
      failed: 1
     skipped: 108
      killed: 0
------------------------
  TOTAL libs: 9498
 TOTAL tests: 9498
   REAL time: 48min 14s 527ms
=======================

Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz

RISC-V Release-FX-QEMU: 727185 / 732507 (99.27%)

=======================
      passed: 727185
      failed: 403
     skipped: 1774
      killed: 4919
------------------------
  TOTAL libs: 253
 TOTAL tests: 734281
   REAL time: 2h 31min 49s 599ms
=======================

Release-FX-QEMU.md, Release-FX-QEMU.xml, testfx_output.tar.gz

Build information and links

GIT: 67142cf81e2dbdd37ea1fe33999266a918521472
CI: 99840094d8ef9dc8ae10ce340b0684ff2cac384f
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

# CORE_LIBS_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s libs /p:EnableSourceLink=false
# CORE_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s clr+libs+host /p:EnableSourceLink=false

# TESTCLR_BUILD_CMD
runtime/src/tests/build.sh -riscv64 -cross -Release -priority1 -p:UseLocalAppHostPack=true
# TESTCLR_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --core_root ./coreclr.Release/Tests/Core_Root --testhost ./testhost.Release --atest ./coreclr.Release --test ./ --log_dir ./logs  --timeout 2700 --log_level DEBUG
# TESTCLR_RUN
/godata/pipelines/Release-CLR-QEMU/logs/run_tests.log
cd "/_PATH_/_WITH_/_TEST_" && ROOTFS_DIR=/crossrootfs/riscv64 QEMU_LD_PREFIX=/crossrootfs/riscv64 __TestDotNetCmd=/godata/pipelines/Release-CLR-QEMU/testhost.Release/dotnet CORE_ROOT=/godata/pipelines/Release-CLR-QEMU/coreclr.Release/Tests/Core_Root  /usr/bin/time -f "exec_time: %e" ./_TEST_BINARY_

# TESTFX_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -rc Release -hc Release -lc Release -s libs.tests --testscope innerloop /p:EnableSourceLink=false /p:UseLocalAppHostPack=true
# TESTFX_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --corefx --testhost ./testhost.Release --atest ./corefx.Release --log_dir ./logs  --timeout 6000 --memlimit 4096 --jobs 16 --log_level DEBUG
# TESTFX_RUN
/go-agent/pipelines/Release-FX-QEMU/logs/run_tests.log
cd "/_PATH_/_WITH_/_TEST_" && ROOTFS_DIR=/crossrootfs/riscv64 QEMU_LD_PREFIX=/crossrootfs/riscv64 __TestDotNetCmd=/go-agent/pipelines/Release-FX-QEMU/testhost.Release/dotnet  /usr/bin/time -f "exec_time: %e" /go-agent/pipelines/Release-FX-QEMU/testhost.Release/dotnet exec   xunit.console.dll _TEST_BINARY_ -nologo -nocolor -notrait category=failing

tomeksowi · 2024-08-16T12:15:16Z

@MichalStrehovsky @jkotas Can anyone review please?

MichalStrehovsky · 2024-08-16T16:24:55Z

@MichalStrehovsky @jkotas Can anyone review please?

@shushanhf could you have a look please? Cc @dotnet/jit-contrib

jkotas · 2024-08-16T18:27:57Z

C treats it as an empty field, but C++ treats it as a non-empty field since C++ defines the size of an empty struct or union as 1.

Are you implementing the C treatment or C++ treatment in this PR?

tomeksowi · 2024-08-17T10:26:32Z

Are you implementing the C treatment or C++ treatment in this PR?

C++. That matches empty structs in .NET which are also sized 1 byte.

jkotas · 2024-08-18T15:35:21Z

The managed/unmanaged interop should follow C ABI. There are number of differences between C and C++ ABIs, for example #106471.

The managed/managed calling convention does not need to follow C ABI. Managed/unmanaged should either follow the C ABI or we should throw PlatformNotSupportedException if it is hard to implement.

tomeksowi · 2024-08-18T17:13:20Z

The managed/unmanaged interop should follow C ABI. There are number of differences between C and C++ ABIs, for example #106471.

The managed/managed calling convention does not need to follow C ABI.

So it's ok for managed/managed calling convention to follow C++ ABI wrt empty structs?

Managed/unmanaged should either follow the C ABI or we should throw PlatformNotSupportedException if it is hard to implement.

Given empty structs are undefined in C (it's a GCC extension to define them as 0 bytes) and they are defined in .NET as 1 byte like in C++, shouldn't .NET throw PlatformNotSupportedException for anything with an empty struct field in a managed/unmanaged signature? (not just on RISC-V but on any architecture)

I'm ok with that, but in that case how to check that managed/managed follows C++ ABI wrt empty structs? Currently I'm using the EmptyStructs test for confronting it against native compilers.

jkotas · 2024-08-18T17:57:16Z

cc @dotnet/interop-contrib for opinions about interop for empty structs.

shouldn't .NET throw PlatformNotSupportedException for anything with an empty struct field in a managed/unmanaged signature? (not just on RISC-V but on any architecture)

It would be best, but it would be a breaking change. I am not sure whether we would want to make this breaking change.

how to check that managed/managed follows C++ ABI wrt empty structs?

The managed/managed calling convention has several differences from the managed/unmanaged calling convention. We do not have an explicit for the specific managed/managed calling convention details. We just have a tests that validate that all parts of the system agree on the details.

I guess it may be ok to keep the test that you have added, but it should have a comment that it is testing undefined behavior.

risc-vv · 2024-08-19T07:00:59Z

RISC-V Release-CLR-QEMU: 9399 / 9400 (99.99%)

=======================
      passed: 9399
      failed: 1
     skipped: 108
      killed: 0
------------------------
  TOTAL libs: 9508
 TOTAL tests: 9508
   REAL time: 48min 15s 416ms
=======================

Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz

RISC-V Release-FX-QEMU: 670605 / 688189 (97.44%)

=======================
      passed: 670605
      failed: 382
     skipped: 1784
      killed: 17202
------------------------
  TOTAL libs: 253
 TOTAL tests: 689973
   REAL time: 2h 32min 37s 234ms
=======================

Release-FX-QEMU.md, Release-FX-QEMU.xml, testclr_output.tar.gz

Build information and links

GIT: 798cb12987f2177a86ba175795394c48e738c8ed
CI: fa8bcf642f8ae7a749cd6fa0832c1c2844d14c5a
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

# CORE_LIBS_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s libs /p:EnableSourceLink=false
# CORE_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s clr+libs+host /p:EnableSourceLink=false

# TESTCLR_BUILD_CMD
runtime/src/tests/build.sh -riscv64 -cross -Release -priority1 -p:UseLocalAppHostPack=true
# TESTCLR_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --core_root ./coreclr.Release/Tests/Core_Root --testhost ./testhost.Release --atest ./coreclr.Release --test ./ --log_dir ./logs  --timeout 2700 --log_level DEBUG
# TESTCLR_RUN
/godata/pipelines/Release-CLR-QEMU/logs/run_tests.log
cd "/_PATH_/_WITH_/_TEST_" && ROOTFS_DIR=/crossrootfs/riscv64 QEMU_LD_PREFIX=/crossrootfs/riscv64 __TestDotNetCmd=/godata/pipelines/Release-CLR-QEMU/testhost.Release/dotnet CORE_ROOT=/godata/pipelines/Release-CLR-QEMU/coreclr.Release/Tests/Core_Root  /usr/bin/time -f "exec_time: %e" ./_TEST_BINARY_

# TESTFX_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -rc Release -hc Release -lc Release -s libs.tests --testscope innerloop /p:EnableSourceLink=false /p:UseLocalAppHostPack=true
# TESTFX_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --corefx --testhost ./testhost.Release --atest ./corefx.Release --log_dir ./logs  --timeout 6000 --memlimit 4096 --jobs 16 --log_level DEBUG
# TESTFX_RUN
/go-agent/pipelines/Release-FX-QEMU/logs/run_tests.log
cd "/_PATH_/_WITH_/_TEST_" && ROOTFS_DIR=/crossrootfs/riscv64 QEMU_LD_PREFIX=/crossrootfs/riscv64 __TestDotNetCmd=/go-agent/pipelines/Release-FX-QEMU/testhost.Release/dotnet  /usr/bin/time -f "exec_time: %e" /go-agent/pipelines/Release-FX-QEMU/testhost.Release/dotnet exec   xunit.console.dll _TEST_BINARY_ -nologo -nocolor -notrait category=failing

RISC-V Release-CLR-QEMU: 9399 / 9400 (99.99%)

=======================
      passed: 9399
      failed: 1
     skipped: 108
      killed: 0
------------------------
  TOTAL libs: 9508
 TOTAL tests: 9508
   REAL time: 1h 5min 49s 519ms
=======================

Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz

RISC-V Release-FX-QEMU: 670605 / 688189 (97.44%)

=======================
      passed: 670605
      failed: 382
     skipped: 1784
      killed: 17202
------------------------
  TOTAL libs: 253
 TOTAL tests: 689973
   REAL time: 2h 32min 37s 234ms
=======================

Release-FX-QEMU.md, Release-FX-QEMU.xml, testclr_output.tar.gz

Build information and links

GIT: 798cb12987f2177a86ba175795394c48e738c8ed
CI: fa8bcf642f8ae7a749cd6fa0832c1c2844d14c5a
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

# CORE_LIBS_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s libs /p:EnableSourceLink=false
# CORE_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s clr+libs+host /p:EnableSourceLink=false

# TESTCLR_BUILD_CMD
runtime/src/tests/build.sh -riscv64 -cross -Release -priority1 -p:UseLocalAppHostPack=true
# TESTCLR_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --core_root ./coreclr.Release/Tests/Core_Root --testhost ./testhost.Release --atest ./coreclr.Release --test ./ --log_dir ./logs  --timeout 2700 --log_level DEBUG
# TESTCLR_RUN
/godata/pipelines/Release-CLR-QEMU/logs/run_tests.log
cd "/_PATH_/_WITH_/_TEST_" && ROOTFS_DIR=/crossrootfs/riscv64 QEMU_LD_PREFIX=/crossrootfs/riscv64 __TestDotNetCmd=/godata/pipelines/Release-CLR-QEMU/testhost.Release/dotnet CORE_ROOT=/godata/pipelines/Release-CLR-QEMU/coreclr.Release/Tests/Core_Root  /usr/bin/time -f "exec_time: %e" ./_TEST_BINARY_

# TESTFX_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -rc Release -hc Release -lc Release -s libs.tests --testscope innerloop /p:EnableSourceLink=false /p:UseLocalAppHostPack=true
# TESTFX_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --corefx --testhost ./testhost.Release --atest ./corefx.Release --log_dir ./logs  --timeout 6000 --memlimit 4096 --jobs 16 --log_level DEBUG
# TESTFX_RUN
/go-agent/pipelines/Release-FX-QEMU/logs/run_tests.log
cd "/_PATH_/_WITH_/_TEST_" && ROOTFS_DIR=/crossrootfs/riscv64 QEMU_LD_PREFIX=/crossrootfs/riscv64 __TestDotNetCmd=/go-agent/pipelines/Release-FX-QEMU/testhost.Release/dotnet  /usr/bin/time -f "exec_time: %e" /go-agent/pipelines/Release-FX-QEMU/testhost.Release/dotnet exec   xunit.console.dll _TEST_BINARY_ -nologo -nocolor -notrait category=failing

tomeksowi · 2024-08-19T08:54:00Z

The managed/managed calling convention has several differences from the managed/unmanaged calling convention. We do not have an explicit for the specific managed/managed calling convention details. We just have a tests that validate that all parts of the system agree on the details.

What are the reasons for these differences? I'm wondering if they are avoidable as long as the arguments can be represented the same way in .NET and on the native side.

One of the reasons I've been pushing in recent PRs for closer compliance with the RISC-V calling convention also for managed/managed is to avoid maintaining separate code paths for the managed/unmanaged calling convention, which we have to support anyway and is well documented.

Another reason (for supporting for custom field offsets in FP structs) is of course that custom field padding may also appear without empty structs, e.g. when the struct argument is packed, so it would be valid for interop calls. Empty structs just allow for different (oversized) padding cases that couldn't be achieved with StructLayoutAttribute.Pack, whose effects cannot exceed the default alignment for a type.

I guess it may be ok to keep the test that you have added, but it should have a comment that it is testing undefined behavior.

Added comment.

jkotas · 2024-08-19T14:25:32Z

What are the reasons for these differences?

Code quality, e.g. JIT: Sign/zero-extend small primitive arguments on arm32 and Apple arm64 #106312 (comment)
Non-implemented features: e.g. Fix ABI for Int128 #73601 (we block types like Vector or Int128 in interop since we know they are not passed correctly)

tannergooding · 2024-08-19T15:23:35Z

cc @dotnet/interop-contrib for opinions about interop for empty structs.

I think it'd be best/easiest to treat empty structs as 1-byte. This ensures that there isn't any "weirdness" when doing [DisableRuntimeMarshalling] or using APIs like sizeof(T)/Unsafe.SizeOf<T>(), matches the .NET and C++ requirements and is within the allowance of C (where it is undefined).

I would expect we are already treating it as 1-byte on Windows x86/x64/Arm64 due to the need for COM interop (which is typically C++ oriented) and that it likely required less work to make function. If we do have some differing back-compat requirement that treats it differently, however, then I'd expect we just preserve that everywhere for consistency.

AaronRobinsonMSFT · 2024-08-19T15:27:36Z

I think it'd be best/easiest to treat empty structs as 1-byte. This ensures that there isn't any "weirdness" when doing [DisableRuntimeMarshalling] or using APIs like sizeof(T)/Unsafe.SizeOf(), matches the .NET and C++ requirements and is within the allowance of C (where it is undefined).

Agree. I know that @jkoritzinsky did some work in this area and it caused a great deal of grief trying to get it right. I'll defer to him, but if I recall correctly, we came down on empty structs were 1-byte.

jkoritzinsky · 2024-08-19T17:01:55Z

Yes, today we treat empty structs as 1-byte. I'd recommend we continue doing so.

The only alternative I'd be comfortable with at this time would be erroring out for zero-sized structs in interop calls on platforms where there is a special ABI for zero-sized structs (like the Int128 and VectorX cases) and treating them as 1-byte in managed code.

tomeksowi added 3 commits August 12, 2024 14:53

Don't pass structs containing empty struct arrays according to hardwa…

2706425

…re floating-point calling convention

Fix adding registers after the passed struct in native version of Ech…

4f1e596

…o_DoubleFloatNestedEmpty_InIntegerRegs_RiscV

Make sure xunit uses the Equals implementation provided by overriding…

67142cf

… Equals from object

tomeksowi requested a review from MichalStrehovsky as a code owner August 12, 2024 13:20

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 12, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 12, 2024

am11 added area-VM-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 13, 2024

jkotas added arch-riscv Related to the RISC-V architecture arch-loongarch64 labels Aug 13, 2024

MichalStrehovsky removed their request for review August 16, 2024 16:25

jkotas added area-Interop-coreclr and removed area-VM-coreclr labels Aug 18, 2024

Add comment to clarify why EmptyStructs test uses C++ instead of C

798cb12

build-analysis bot mentioned this pull request Aug 19, 2024

Test failure "RemoteExecutionException : Half-way through waiting for remote process." in System.Threading.ThreadPools.Tests.ThreadPoolTests.IOCompletionPortCountConfigVarTest #106494

Closed

jkotas approved these changes Aug 19, 2024

View reviewed changes

jkotas merged commit 71373ae into dotnet:main Aug 19, 2024
93 of 96 checks passed

github-actions bot locked and limited conversation to collaborators Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V][LoongArch64] Pass structs containing empty struct arrays according to integer calling convention #106266

[RISC-V][LoongArch64] Pass structs containing empty struct arrays according to integer calling convention #106266

tomeksowi commented Aug 12, 2024

risc-vv commented Aug 13, 2024 •

edited

Loading

tomeksowi commented Aug 16, 2024

MichalStrehovsky commented Aug 16, 2024

jkotas commented Aug 16, 2024

tomeksowi commented Aug 17, 2024

jkotas commented Aug 18, 2024

tomeksowi commented Aug 18, 2024

jkotas commented Aug 18, 2024

risc-vv commented Aug 19, 2024 •

edited

Loading

tomeksowi commented Aug 19, 2024

jkotas commented Aug 19, 2024

tannergooding commented Aug 19, 2024 •

edited

Loading

AaronRobinsonMSFT commented Aug 19, 2024 •

edited

Loading

jkoritzinsky commented Aug 19, 2024

[RISC-V][LoongArch64] Pass structs containing empty struct arrays according to integer calling convention #106266

[RISC-V][LoongArch64] Pass structs containing empty struct arrays according to integer calling convention #106266

Conversation

tomeksowi commented Aug 12, 2024

risc-vv commented Aug 13, 2024 • edited Loading

tomeksowi commented Aug 16, 2024

MichalStrehovsky commented Aug 16, 2024

jkotas commented Aug 16, 2024

tomeksowi commented Aug 17, 2024

jkotas commented Aug 18, 2024

tomeksowi commented Aug 18, 2024

jkotas commented Aug 18, 2024

risc-vv commented Aug 19, 2024 • edited Loading

tomeksowi commented Aug 19, 2024

jkotas commented Aug 19, 2024

tannergooding commented Aug 19, 2024 • edited Loading

AaronRobinsonMSFT commented Aug 19, 2024 • edited Loading

jkoritzinsky commented Aug 19, 2024

risc-vv commented Aug 13, 2024 •

edited

Loading

risc-vv commented Aug 19, 2024 •

edited

Loading

tannergooding commented Aug 19, 2024 •

edited

Loading

AaronRobinsonMSFT commented Aug 19, 2024 •

edited

Loading