[CppCodeGen] Thread unable to start due to assert

[iperov]

cs prog

namespace Test1
{
    class Program
    {
        static Thread m_Thread;

        static public void threadProc()
        {
            for (; ; )
            {
                Thread.Sleep(10);
            }
        }

        static void Main(string[] args)
        {
            m_Thread = new Thread(threadProc);
            m_Thread.Start();
            for (; ; )
            {
                Thread.Sleep(10);
            }

        }
    }
}

cpp assert

--------------------------------------------------
Debug Assertion Violation

Message: You must p/invoke to RhYield

Expression: '!ThreadStore::GetCurrentThread()->IsCurrentThreadInCooperativeMode(
)'

File: C:\corert\src\Native\Runtime\MiscHelpers.cpp, Line: 60
--------------------------------------------------
--------------------------------------------------
Debug Assertion Violation

Expression: 'pCurThread->IsInitialized()'

File: c:\corert\src\native\runtime\threadstore.inl, Line: 20
--------------------------------------------------

[jkotas]

The problem is that CppCodeGen does not emit reverse PInvoke transition. We have hacked around it for the main entrypoint here. Other entrypoints - like new thread entrypoint - are missing the transition, and that's why you see this assert.

The fix should be to:

• In : https://github.com/dotnet/corert/blob/master/src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs: If method.IsNativeCallable returns true, emit ReversePInvokeFrame __frame; __reverse_pinvoke(&__frame); in Compile and __reverse_pinvoke_return(&__frame); in ImportReturn
• In https://github.com/dotnet/corert/blob/master/src/Native/Bootstrap/main.cpp: Delete the hack for the main entrypoint.

[hippiehunter]

@jkotas
I've made the fixes you describe but it appears to be that prior to the RhYield call, someone has to set m_pTransitionFrame in the current thread, transitioning into preemptive rather than cooperative mode. based on the comments in RhYield this appears to be an expectation for PInvoked methods. It looks like the transitionFrame needs to be a PInvokeTransitionFrame but the only place i see reference to that is inside bits that look JIT specific. Is this missing a larger piece of infrastructure for CppCodeGen or am i missing something obvious.

[jkotas]

@hippiehunter Thank you for taking a look!

Ok, there are really two separate bugs here: one is in ReversePInvoke and second one is in PInvoke.

My comment was about how to first the first bug. To verify that your fix for the first bug is working well, you can comment out the IsCurrentThreadInCooperativeMode assert temporarily and see whether the thread gets created gracefully. If it does, feel free to submit the PR with the fix. We prefer more smaller PRs.

There is a bit more code missing to fix the second bug, but it should not be large:

• Add simplified version of PInvokeTransitionFrame for CppCodeGen. It really just needs m_pThread field for now.
• Emit calls to RhpPInvoke/RhpReversePInvoke around the actual call in ImportCall when IsRawPInvoke returns true.
• Fill in the implementation of RhpPInvoke/RhpReversePInvoke in https://github.com/dotnet/corert/blob/master/src/Native/Runtime/portable.cpp#L214. It should match what the asm implementations do https://github.com/dotnet/corert/blob/master/src/Native/Runtime/amd64/PInvoke.S#L18 except that it does not need to fill any of the PInvokeTransitionFrame fields except of m_pThread.

[hippiehunter]

@jkotas
I've implemented RhpPInvoke2/RhpPInvokeReturn2 in Thread.cpp to match the inline behaviors used for RhpReversePInvoke, and wrapped the output from ImportCall when IsRawPInvoke is true.

I ran into an interesting problem with PInvokeTransitionFrame, because its defined in rhbinder.h and that's not accessible in the generated cpp code, I've had to match its layout as a duplicate declaration in CPPCodeGen.h. Unfortunately the platform macros don't appear to be applicable here so as a prototype I hard coded the field sizes. With the hard coded field sizes things appear to be working.

Is there a right way to get PInvokeTransitionFrame to be accessible inside the generated cpp files?

[jkotas]

Duplicating the required defininitions inside CPPCodeGen.h should be fine.

Unfortunately the platform macros don't appear to be applicable

I would fix this by introducing portable definition of PInvokeTransitionFrame that does not vary from platform to platform.

#ifdef USE_PORTABLE_HELPERS
struct PInvokeTransitionFrame
{
     TgtPTR_Thread   m_pThread;
};
#else // USE_PORTABLE_HELPERS
struct PInvokeTransitionFrame
{
     ... existing definition ...
};
#endif // USE_PORTABLE_HELPERS

[jkotas]

Fixed by #4503 and #4499.

cc @hippiehunter