[x86/Linux] JIT.Methodical.structs.systemvbringup.structinregs test failure

[parjong]

An exception is thrown while running InvokeCallback5 test:

$ ./corerun coreclr-unittest/JIT/Methodical/structs/systemvbringup/structinregs/structinregs.exe
...
Native S5: 1, 0.000000
S5: 1, 0

Unhandled Exception: System.Exception: Exception of type 'System.Exception' was thrown.
   at structinreg.Program3.<>c.<Main1>b__84_4(S5 par)
   at structinreg.Program3.Main1()
Aborted (core dumped)

[parjong]

It seems that there is mismatch between managed struct layout and native struct layout.

The native method shows the following result when adding [StructLayout(LayoutKind.Sequential, Pack=4)] attribute on S5 struct declaration:

Native S5: 1, 2.000000
S5: 1, 2

[parjong]

@jkotas @BruceForstall Could you let me know which code (JIT, EE) is related with such layout issues?

[parjong]

The following patch seems to resolve S5 case, but introduces other test failures.

diff --git a/src/vm/fieldmarshaler.h b/src/vm/fieldmarshaler.h
index 9ec7e87..b193b46 100644
--- a/src/vm/fieldmarshaler.h
+++ b/src/vm/fieldmarshaler.h
@@ -85,7 +85,11 @@ enum NStructFieldType
 //=======================================================================
 // Magic number for default struct packing size.
 //=======================================================================
+#if defined(_TARGET_X86_) && defined(FEATURE_PAL)
+#define DEFAULT_PACKING_SIZE 4
+#else // _TARGET_X86 && FEATURE_APL
 #define DEFAULT_PACKING_SIZE 8
+#endif // !_TARGET_X86_ || !FEATURE_PAL


 //=======================================================================

[jkotas]

which code (JIT, EE) is related with such layout issues?

Yes, you are looking in the right place. methodtablebuilder.cpp is also involved in this.

[parjong]

I observed the following test failure while evaluating the above patch:

FAILED   - [ 647 :    6 sec ] Interop/MarshalAPI/OffsetOf/OffsetOf/OffsetOf.sh
               BEGIN EXECUTION
               /root/dotnet-test/coreclr_unittest_20170313_125623_x86_x86-emu/coreclr-unittest-170313-035653/overlay/corerun OffsetOf.exe
               
               Unhandled Exception: CoreFXTestLibrary.AssertTestException: Assert.AreEqual: Expected: [96]. Actual: [72]. 
                  at CoreFXTestLibrary.Assert.HandleFail(String assertionName, String message)
                  at CoreFXTestLibrary.Assert.AreEqual[T](T expected, T actual, String message)
                  at OffsetTest.TestFieldAlignment_Decimal()
                  at OffsetTest.Main(String[] args)
               ./OffsetOf.sh: line 131:  5076 Aborted                 (core dumped) $_DebuggerFullPath "$CORE_ROOT/corerun" OffsetOf.exe $CLRTestExecutionArguments
               Expected: 100
               Actual: 134
               END EXECUTION - FAILED

The related test uses a hard-coded size for check:

    public static void TestFieldAlignment_Decimal()
    {
        var t = typeof(FieldAlignementTest_Decimal);
        Assert.AreEqual(96, Marshal.SizeOf(t));

        Assert.AreEqual(new IntPtr(0), Marshal.OffsetOf(t, "b"));
        Assert.AreEqual(new IntPtr(8), Marshal.OffsetOf(t, "p"));
        Assert.AreEqual(new IntPtr(88), Marshal.OffsetOf(t, "s"));
    }

Is there any standard related with struct layout? I'm not sure whether this failure just comes from test issue, or not.

[BruceForstall]

@parjong It seems odd that we would need to change DEFAULT_PACKING_SIZE just for Linux/x86, when it is not changed for Windows/x86, or other 32-bit platforms (i.e., arm32). Also, I see that same value appears in mscorlib\src\System\Runtime\InteropServices\Attributes.cs.

It seems like the issue is in the structinregs.cpp test case. It seems that clang on Linux/x86 compiles this:

struct S5
{
    int x;
    double y;
};

without 4 bytes of padding between the int and double types. VC++ on Windows has a default packing (pragma pack) value of 8, and double is 8-byte aligned, so it pads to 8 when the double is encountered. Apparently this happens on Linux/x64.

Can you check?

Does the Linux/x86 compiler have the equivalent of #pragma pack(8) that we can add at the top of structinregs.cpp?

[parjong]

@BruceForstall This article says that struct is 4-byte aligned even if it includes double fields for GCC on Linux (unlike VC++ on Windows).

[parjong]

As I know, there is clang attribute for this issue __attribute__((__ms_struct__)) (which is used to address dotnet/coreclr#8377).

[BruceForstall]

Seems like we should use that in the structinregs.cpp test.

[parjong]

I checked __attribute__((__ms_struct__))-based approach, and found that it also addresses the above S5 issue (I encountered failure on S11 case, but it looks similar).

As I understand, this approach has the same effect with adding [StructLayout(LayoutKind.Sequential, Pack=4)] on corresponding managed struct declaration, which also address the above S5 case failure as I mentioned earlier.

This means that any x86/Linux native code that P/Invoke targets should use this kind of attributes, or the corresponding managed code should use struct layout attributes.

I would like to ask about the current .NET guideline on cross-platform development. Does that recommend this approach? If so, we could resolve this via simply adding __attribute__((__ms_struct__)) attribute to all the struct in structinregs.cpp.

[jkotas]

We should do what Mono does on Linux x86. If Mono uses the platform native padding for Linux x86, we should do the same for .NET Core.

[parjong]

Mono seems to use native padding for x86/Linux. Here are test samples and the result:

• managed.cs

using System;
using System.Runtime.InteropServices;

namespace structinreg
{
    class Program
    {
        public struct S5
        {
            public int x;
            public double y;
        };

        public delegate void MyCallback5(S5 s);

        [DllImport("native")]
        public static extern void InvokeCallback5(MyCallback5 callback, S5 s);

        public static void Main()
        {
                S5 s5;
                s5.x = 1;
                s5.y = 2;
                InvokeCallback5((par) =>
                {
                    Console.WriteLine("S5: {0}, {1}", par.x, par.y);
                    if (par.x != 1 || par.y != 2)
                    {
                        throw new System.Exception();
                    }
                }, s5);
        }
    };
}
~

• native.cpp

#include <stdio.h>

struct S5
{
    int x;
    double y;
};

typedef void (*PFNACTION5)(S5 s);

extern "C" void InvokeCallback5(PFNACTION5 callback, S5 s)
{
    printf("Native S5: %d, %f\n", s.x, s.y);
    callback(s);
}

• result

$ mcs managed.cs
$ g++ -fPIC --shared -o libnative.so native.cpp
$ mono managed.exe
Native S5: 1, 2.000000
S5: 1, 2

[BruceForstall]

@jkotas It seems like changing how we layout structs, per platform, is going to be very tricky to get right, with lots of pervasive changes required, e.g., to tests, maybe to frameworks assemblies, etc. Especially since it appears we haven't done this before. Do you agree?

[jkotas]

We have done it before - the layout rules are different between x86 and arm on Windows as side-effect of FEATURE_64BIT_ALIGNMENT. Changing the default alignment for double should be pretty local change in type loader. It should not have any effect outside of the type loader because of you can get any layout via explicit layout and the rest of the system should handle it gracefully.

I agree that there may be some tests that need to be updated.

However, the effect on framework assemblies or code out there should be exact opposite: it should just work on Linux x86 CoreCLR if it worked on other platforms.

[parjong]

@BruceForstall @jkotas The following tests failed when we set the default struct alignment as 4:

• Interop/MarshalAPI/OffsetOf/OffsetOf/OffsetOf.sh
• JIT/Directed/RVAInit/nested/nested.sh
• JIT/Directed/RVAInit/simple/simple.sh
• JIT/Regression/CLR-x86-JIT/V1.2-Beta1/b103058/b103058/b103058.sh
• JIT/Performance/CodeQuality/Span/SpanBench/SpanBench.sh

[BruceForstall]

@parjong I don't know about SpanBench, but it's easy to see looking at the others that they all depend on existing struct layout rules, so they would need to be altered (dual versioned, or #ifdef'ed) or disabled for Linux/x86. My guess is you'll find more such failures in the dotnet/corefx test run.

[parjong]

@BruceForstall As you mentioned, all the tests except SpanBench seems to depend on struct layout rules. It seems that SpanBench comes from another issue (may be related with recent devirt changes?).