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

[tomeksowi]

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

[risc-vv]

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]

@MichalStrehovsky @jkotas Can anyone review please?

[MichalStrehovsky]

@MichalStrehovsky @jkotas Can anyone review please?

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

[jkotas]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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.