Skip to content

Commit

Permalink
[x64][SysV] Classify empty structs for passing like padding (dotnet#1…
Browse files Browse the repository at this point in the history
…03799)

The current implementation barred a struct containing empty struct fields from enregistration. This did not match the [System V ABI](https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf) which says "NO_CLASS This class is used as initializer in the algorithms. It will be used for padding and **empty structures** and unions". It also does not match the behavior of GCC & Clang on Linux.
  • Loading branch information
tomeksowi authored Jun 28, 2024
1 parent 6f7def8 commit 03b75f0
Show file tree
Hide file tree
Showing 6 changed files with 283 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using ILCompiler;
using Internal.TypeSystem;
using System.Runtime.CompilerServices;
using static Internal.JitInterface.SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR;
using static Internal.JitInterface.SystemVClassificationType;

Expand Down Expand Up @@ -207,7 +208,10 @@ private static bool ClassifyEightBytes(TypeDesc typeDesc,

if (numIntroducedFields == 0)
{
return false;
// Classify empty struct like padding
helper.LargestFieldOffset = startOffsetOfStruct;
AssignClassifiedEightByteTypes(ref helper);
return true;
}

// The SIMD and Int128 Intrinsic types are meant to be handled specially and should not be passed as struct registers
Expand Down Expand Up @@ -375,8 +379,12 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
// Calculate the eightbytes and their types.

int lastFieldOrdinal = sortedFieldOrder[largestFieldOffset];
int offsetAfterLastFieldByte = largestFieldOffset + helper.FieldSizes[lastFieldOrdinal];
SystemVClassificationType lastFieldClassification = helper.FieldClassifications[lastFieldOrdinal];
int lastFieldSize = (lastFieldOrdinal >= 0) ? helper.FieldSizes[lastFieldOrdinal] : 0;
int offsetAfterLastFieldByte = largestFieldOffset + lastFieldSize;
Debug.Assert(offsetAfterLastFieldByte <= helper.StructSize);
SystemVClassificationType lastFieldClassification = (lastFieldOrdinal >= 0)
? helper.FieldClassifications[lastFieldOrdinal]
: SystemVClassificationTypeNoClass;

int usedEightBytes = 0;
int accumulatedSizeForEightBytes = 0;
Expand All @@ -403,6 +411,8 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
// the SysV ABI spec.
fieldSize = 1;
fieldClassificationType = offset < offsetAfterLastFieldByte ? SystemVClassificationTypeNoClass : lastFieldClassification;
if (offset % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // new eightbyte
foundFieldInEightByte = false;
}
else
{
Expand Down Expand Up @@ -455,13 +465,16 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
}
}

if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // If we just finished checking the last byte of an eightbyte
// If we just finished checking the last byte of an eightbyte or the entire struct
if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0 || (offset + 1) == helper.StructSize)
{
if (!foundFieldInEightByte)
{
// If we didn't find a field in an eight-byte (i.e. there are no explicit offsets that start a field in this eightbyte)
// If we didn't find a field in an eightbyte (i.e. there are no explicit offsets that start a field in this eightbyte)
// then the classification of this eightbyte might be NoClass. We can't hand a classification of NoClass to the JIT
// so set the class to Integer (as though the struct has a char[8] padding) if the class is NoClass.
//
// TODO: Fix JIT, NoClass eightbytes are valid and passing them is broken because of this.
if (helper.EightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] == SystemVClassificationTypeNoClass)
{
helper.EightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] = SystemVClassificationTypeInteger;
Expand Down
35 changes: 26 additions & 9 deletions src/coreclr/vm/methodtable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2114,11 +2114,14 @@ bool MethodTable::ClassifyEightBytesWithManagedLayout(SystemVStructRegisterPassi

DWORD numIntroducedFields = GetNumIntroducedInstanceFields();

// It appears the VM gives a struct with no fields of size 1.
// Don't pass in register such structure.
if (numIntroducedFields == 0)
{
return false;
helperPtr->largestFieldOffset = startOffsetOfStruct;
LOG((LF_JIT, LL_EVERYTHING, "%*s**** Classify empty struct %s (%p) like padding, startOffset %d, total struct size %d\n",
nestingLevel * 5, "", this->GetDebugClassName(), this, startOffsetOfStruct, helperPtr->structSize));

AssignClassifiedEightByteTypes(helperPtr, nestingLevel);
return true;
}

// The SIMD Intrinsic types are meant to be handled specially and should not be passed as struct registers
Expand Down Expand Up @@ -2334,7 +2337,12 @@ bool MethodTable::ClassifyEightBytesWithNativeLayout(SystemVStructRegisterPassin
// No fields.
if (numIntroducedFields == 0)
{
return false;
helperPtr->largestFieldOffset = startOffsetOfStruct;
LOG((LF_JIT, LL_EVERYTHING, "%*s**** Classify empty struct %s (%p) like padding, startOffset %d, total struct size %d\n",
nestingLevel * 5, "", this->GetDebugClassName(), this, startOffsetOfStruct, helperPtr->structSize));

AssignClassifiedEightByteTypes(helperPtr, nestingLevel);
return true;
}

bool hasImpliedRepeatedFields = HasImpliedRepeatedFields(this);
Expand Down Expand Up @@ -2586,8 +2594,12 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
// Calculate the eightbytes and their types.

int lastFieldOrdinal = sortedFieldOrder[largestFieldOffset];
unsigned int offsetAfterLastFieldByte = largestFieldOffset + helperPtr->fieldSizes[lastFieldOrdinal];
SystemVClassificationType lastFieldClassification = helperPtr->fieldClassifications[lastFieldOrdinal];
unsigned int lastFieldSize = (lastFieldOrdinal >= 0) ? helperPtr->fieldSizes[lastFieldOrdinal] : 0;
unsigned int offsetAfterLastFieldByte = largestFieldOffset + lastFieldSize;
_ASSERTE(offsetAfterLastFieldByte <= helperPtr->structSize);
SystemVClassificationType lastFieldClassification = (lastFieldOrdinal >= 0)
? helperPtr->fieldClassifications[lastFieldOrdinal]
: SystemVClassificationTypeNoClass;

unsigned int usedEightBytes = 0;
unsigned int accumulatedSizeForEightBytes = 0;
Expand All @@ -2614,6 +2626,8 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
// the SysV ABI spec.
fieldSize = 1;
fieldClassificationType = offset < offsetAfterLastFieldByte ? SystemVClassificationTypeNoClass : lastFieldClassification;
if (offset % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // new eightbyte
foundFieldInEightByte = false;
}
else
{
Expand Down Expand Up @@ -2666,13 +2680,16 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
}
}

if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // If we just finished checking the last byte of an eightbyte
// If we just finished checking the last byte of an eightbyte or the entire struct
if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0 || (offset + 1) == helperPtr->structSize)
{
if (!foundFieldInEightByte)
{
// If we didn't find a field in an eight-byte (i.e. there are no explicit offsets that start a field in this eightbyte)
// If we didn't find a field in an eightbyte (i.e. there are no explicit offsets that start a field in this eightbyte)
// then the classification of this eightbyte might be NoClass. We can't hand a classification of NoClass to the JIT
// so set the class to Integer (as though the struct has a char[8] padding) if the class is NoClass.
//
// TODO: Fix JIT, NoClass eightbytes are valid and passing them is broken because of this.
if (helperPtr->eightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] == SystemVClassificationTypeNoClass)
{
helperPtr->eightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] = SystemVClassificationTypeInteger;
Expand Down Expand Up @@ -2705,9 +2722,9 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
LOG((LF_JIT, LL_EVERYTHING, " **** Number EightBytes: %d\n", helperPtr->eightByteCount));
for (unsigned i = 0; i < helperPtr->eightByteCount; i++)
{
_ASSERTE(helperPtr->eightByteClassifications[i] != SystemVClassificationTypeNoClass);
LOG((LF_JIT, LL_EVERYTHING, " **** eightByte %d -- classType: %s, eightByteOffset: %d, eightByteSize: %d\n",
i, GetSystemVClassificationTypeName(helperPtr->eightByteClassifications[i]), helperPtr->eightByteOffsets[i], helperPtr->eightByteSizes[i]));
_ASSERTE(helperPtr->eightByteClassifications[i] != SystemVClassificationTypeNoClass);
}
#endif // _DEBUG
}
Expand Down
6 changes: 4 additions & 2 deletions src/tests/JIT/Directed/StructABI/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ project (StructABILib)
include_directories(${INC_PLATFORM_DIR})

if(CLR_CMAKE_HOST_WIN32)
add_compile_options(/TC) # compile all files as C
set_source_files_properties(StructABI.c PROPERTIES COMPILE_OPTIONS /TC) # compile as C
else()
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fvisibility=hidden")
set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -fvisibility=hidden -Wno-return-type-c-linkage")
endif()

# add the executable
add_library (StructABILib SHARED StructABI.c)
add_library (EmptyStructsLib SHARED EmptyStructs.cpp)

# add the install targets
install (TARGETS StructABILib DESTINATION bin)
install (TARGETS StructABILib EmptyStructsLib DESTINATION bin)
68 changes: 68 additions & 0 deletions src/tests/JIT/Directed/StructABI/EmptyStructs.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

#include <stdint.h>
#include <stddef.h>

#ifdef _MSC_VER
#define DLLEXPORT __declspec(dllexport)
#else
#define DLLEXPORT __attribute__((visibility("default")))
#endif // _MSC_VER

struct Empty
{
};
static_assert(sizeof(Empty) == 1, "Empty struct must be sized like in .NET");


struct IntEmpty
{
int32_t Int0;
Empty Empty0;
};

extern "C" DLLEXPORT IntEmpty EchoIntEmptySysV(int i0, IntEmpty val)
{
return val;
}


struct IntEmptyPair
{
IntEmpty IntEmpty0;
IntEmpty IntEmpty1;
};

extern "C" DLLEXPORT IntEmptyPair EchoIntEmptyPairSysV(int i0, IntEmptyPair val)
{
return val;
}


struct EmptyFloatIntInt
{
Empty Empty0;
float Float0;
int32_t Int0;
int32_t Int1;
};

extern "C" DLLEXPORT EmptyFloatIntInt EchoEmptyFloatIntIntSysV(int i0, float f0, EmptyFloatIntInt val)
{
return val;
}


struct FloatFloatEmptyFloat
{
float Float0;
float Float1;
Empty Empty0;
float Float2;
};

extern "C" DLLEXPORT FloatFloatEmptyFloat EchoFloatFloatEmptyFloatSysV(float f0, FloatFloatEmptyFloat val)
{
return val;
}

150 changes: 150 additions & 0 deletions src/tests/JIT/Directed/StructABI/EmptyStructs.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Runtime.InteropServices;
using System.Runtime.CompilerServices;
using Xunit;

public static class Program
{
public struct Empty
{
}


public struct IntEmpty
{
public int Int0;
public Empty Empty0;

public static IntEmpty Get()
=> new IntEmpty { Int0 = 0xBabc1a };

public bool Equals(IntEmpty other)
=> Int0 == other.Int0;

public override string ToString()
=> $"{{Int0:{Int0:x}}}";
}

[DllImport("EmptyStructsLib")]
public static extern IntEmpty EchoIntEmptySysV(int i0, IntEmpty val);

[MethodImpl(MethodImplOptions.NoInlining)]
public static IntEmpty EchoIntEmptySysVManaged(int i0, IntEmpty val) => val;

[Fact]
public static void TestIntEmptySysV()
{
IntEmpty expected = IntEmpty.Get();
IntEmpty native = EchoIntEmptySysV(0, expected);
IntEmpty managed = EchoIntEmptySysVManaged(0, expected);

Assert.Equal(expected, native);
Assert.Equal(expected, managed);
}


public struct IntEmptyPair
{
public IntEmpty IntEmpty0;
public IntEmpty IntEmpty1;

public static IntEmptyPair Get()
=> new IntEmptyPair { IntEmpty0 = IntEmpty.Get(), IntEmpty1 = IntEmpty.Get() };

public bool Equals(IntEmptyPair other)
=> IntEmpty0.Equals(other.IntEmpty0) && IntEmpty1.Equals(other.IntEmpty1);

public override string ToString()
=> $"{{IntEmpty0:{IntEmpty0}, IntEmpty1:{IntEmpty1}}}";
}

[DllImport("EmptyStructsLib")]
public static extern IntEmptyPair EchoIntEmptyPairSysV(int i0, IntEmptyPair val);

[MethodImpl(MethodImplOptions.NoInlining)]
public static IntEmptyPair EchoIntEmptyPairSysVManaged(int i0, IntEmptyPair val) => val;

[Fact]
public static void TestIntEmptyPairSysV()
{
IntEmptyPair expected = IntEmptyPair.Get();
IntEmptyPair native = EchoIntEmptyPairSysV(0, expected);
IntEmptyPair managed = EchoIntEmptyPairSysVManaged(0, expected);

Assert.Equal(expected, native);
Assert.Equal(expected, managed);
}


public struct EmptyFloatIntInt
{
public Empty Empty0;
public float Float0;
public int Int0;
public int Int1;

public static EmptyFloatIntInt Get()
=> new EmptyFloatIntInt { Float0 = 2.71828f, Int0 = 0xBabc1a, Int1 = 0xC10c1a };

public bool Equals(EmptyFloatIntInt other)
=> Float0 == other.Float0 && Int0 == other.Int0 && Int1 == other.Int1;

public override string ToString()
=> $"{{Float0:{Float0}, Int0:{Int0:x}, Int1:{Int1:x}}}";
}

[DllImport("EmptyStructsLib")]
public static extern EmptyFloatIntInt EchoEmptyFloatIntIntSysV(int i0, float f0, EmptyFloatIntInt val);

[MethodImpl(MethodImplOptions.NoInlining)]
public static EmptyFloatIntInt EchoEmptyFloatIntIntSysVManaged(int i0, float f0, EmptyFloatIntInt val) => val;

[Fact]
public static void TestEmptyFloatIntIntSysV()
{
EmptyFloatIntInt expected = EmptyFloatIntInt.Get();
EmptyFloatIntInt native = EchoEmptyFloatIntIntSysV(0, 0f, expected);
EmptyFloatIntInt managed = EchoEmptyFloatIntIntSysVManaged(0, 0f, expected);

Assert.Equal(expected, native);
Assert.Equal(expected, managed);
}


public struct FloatFloatEmptyFloat
{
public float Float0;
public float Float1;
public Empty Empty0;
public float Float2;

public static FloatFloatEmptyFloat Get()
=> new FloatFloatEmptyFloat { Float0 = 2.71828f, Float1 = 3.14159f, Float2 = 1.61803f };

public bool Equals(FloatFloatEmptyFloat other)
=> Float0 == other.Float0 && Float1 == other.Float1 && Float2 == other.Float2;

public override string ToString()
=> $"{{Float0:{Float0}, Float1:{Float1}, Float2:{Float2}}}";
}

[DllImport("EmptyStructsLib")]
public static extern FloatFloatEmptyFloat EchoFloatFloatEmptyFloatSysV(float f0, FloatFloatEmptyFloat val);

[MethodImpl(MethodImplOptions.NoInlining)]
public static FloatFloatEmptyFloat EchoFloatFloatEmptyFloatSysVManaged(float f0, FloatFloatEmptyFloat val) => val;

[Fact]
public static void TestFloatFloatEmptyFloatSysV()
{
FloatFloatEmptyFloat expected = FloatFloatEmptyFloat.Get();
FloatFloatEmptyFloat native = EchoFloatFloatEmptyFloatSysV(0f, expected);
FloatFloatEmptyFloat managed = EchoFloatFloatEmptyFloatSysVManaged(0f, expected);

Assert.Equal(expected, native);
Assert.Equal(expected, managed);
}
}
Loading

0 comments on commit 03b75f0

Please sign in to comment.