implemented portable PInvoke infrastructure for CppCodeGen #4503

hippiehunter · 2017-09-15T00:26:33Z

No description provided.

dnfclas · 2017-09-15T00:26:37Z

@hippiehunter,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

jkotas · 2017-09-15T00:47:23Z

src/Native/Runtime/thread.cpp

+COOP_PINVOKE_HELPER(void, RhpPInvoke2, (PInvokeTransitionFrame* pFrame))
+{
+    Thread * pCurThread = ThreadStore::RawGetCurrentThread();
+	pCurThread->InlinePInvoke(pFrame);


Nit: Indentation

jkotas · 2017-09-15T00:48:28Z

src/Native/Runtime/thread.cpp

@@ -1158,6 +1158,31 @@ FORCEINLINE void Thread::InlineReversePInvokeReturn(ReversePInvokeFrame * pFrame
    }
 }

+FORCEINLINE void Thread::InlinePInvoke(PInvokeTransitionFrame * pFrame)
+{
+	pFrame->m_pThread = this;


Nit: indentation

jkotas · 2017-09-15T00:48:42Z

src/Native/Runtime/thread.cpp

+
+FORCEINLINE void Thread::InlinePInvokeReturn(PInvokeTransitionFrame * pFrame)
+{
+	m_pTransitionFrame = NULL;


Nit: indentation

jkotas · 2017-09-15T00:51:30Z

src/Native/Runtime/thread.h

@@ -255,6 +255,9 @@ class Thread : private ThreadBuffer
    bool InlineTryFastReversePInvoke(ReversePInvokeFrame * pFrame);
    void InlineReversePInvokeReturn(ReversePInvokeFrame * pFrame);

+	void InlinePInvoke(PInvokeTransitionFrame * pFrame);


Nit: Indentiation

jkotas · 2017-09-15T01:01:05Z

src/Native/Runtime/thread.cpp

@@ -1287,4 +1312,16 @@ COOP_PINVOKE_HELPER(void, RhpReversePInvokeReturn2, (ReversePInvokeFrame * pFram
    pFrame->m_savedThread->InlineReversePInvokeReturn(pFrame);
 }

+COOP_PINVOKE_HELPER(void, RhpPInvoke2, (PInvokeTransitionFrame* pFrame))


RhpPInvoke2/RhpPInvokeReturn2 should be under #ifdef USE_PORTABLE_HELPERS here. It does not make sense otherwise.

jkotas · 2017-09-15T01:04:14Z

src/Native/Bootstrap/CppCodeGen.h

@@ -41,4 +41,33 @@ inline double __uint64_to_double(uint64_t v)
    return val.d;
 }

-#endif // __CPP_CODE_GEN_H
+#ifdef USE_PORTABLE_HELPERS


I meant this ifdef to be in rhbinder.h. This file is only used when USE_PORTABLE_HELPERS is defined, so the ifdef does not need to be here.

jkotas · 2017-09-15T01:06:01Z

src/Native/Bootstrap/CppCodeGen.h

+#ifdef _TARGET_ARM_
+    TgtPTR_Void     m_ChainPointer; // R11, used by OS to walk stack quickly
+#endif
+    TgtPTR_Void     m_RIP;


What is the minimal struct PInvokeTransitionFrame that we can get away for USE_PORTABLE_HELPERS ? Ideally, it would have just m_pThread and nothing else.

jkotas · 2017-09-15T01:07:29Z

src/ILCompiler.CppCodeGen/src/CppCodeGen/ILToCppImporter.cs

+            if (method.IsRawPInvoke())
+            {
+                AppendLine();
+                Append("PInvokeTransitionFrame __piframe");


This assumes that there will be at most one RawPInvoke callsite per method. It is a safe assumption to make today, but it maybe worth a comment.

jkotas · 2017-09-15T01:09:10Z

src/Native/Bootstrap/CppCodeGen.h

@@ -41,4 +41,33 @@ inline double __uint64_to_double(uint64_t v)
    return val.d;
 }


You can move ReversePInvokeFrame from common.h here as well while you are on it.

(Most of the CppCodeGen specific stuff should really be in this file actually and not in common.h.)

hippiehunter · 2017-09-15T04:38:30Z

It appears that StackFrameIterator.cpp depends on most of the fields in PInvokeTransitionFrame. I guess that brings me to my next question, what is the way forward on enabling the stack walker here? Is it as simple as filling out the values of the transition frame when we pinvoke?

jkotas · 2017-09-15T06:24:46Z

The stackwalker will need significant overhaul for CppCodeGen. It maybe useful to mention it in a comment in the portable version of PInvokeTransitionFrame.

Stackwalker is used for several purposes today:

Exception handling ([CppCodeGen] Managed Exception Dispatch and Unwind #910): Each thread needs to maintain linked list of exception handlers. Every method with exception handling needs to push/pop frame into the linked list. The linked list is necessary to support .NET two phase exception handling (exception filters). The actual unwinding can be then done by throwing C++ exception.
Textual stacktrace generation (e.g. stacktrace for exceptions, or Environment.StackTrace): This is extreme case of the above. Each method would need to push/pop frame into the linked list. It is very expensive to do. It may be optional feature - it is how Unity IL2CPP does it (https://blogs.unity3d.com/2015/05/13/il2cpp-internals-a-tour-of-generated-code/).
Root enumeration for Garbage collection ([CppCodeGen] Enable conservative GC #2033): The standard way to do garbage collection for C++ is to do conservative scanning of all potential pointers on the stack. There is a small trick to it: It is necessary to ensure that pointers in registers are spilled into into memory. The best way to do that is using __builtin_unwind_init when it is available - it will need to be added to the PInvoke GC transitions. Alternative design is for the linked list of frame records to contain pointers to all GC roots. It makes the generated harder to read and less efficient (all GC pointers have to be spilled into this structure, they cannot be kept in registers). On the other hand, GC has less work to do because of the list of stack roots can be precise.

jkotas · 2017-09-15T06:26:04Z

src/Native/Runtime/thread.cpp

+    pFrame->m_pThread->InlinePInvokeReturn(pFrame);
+}
+
+#endif


Nit: We typically have the name of the ifdef as comment on #endif.

jkotas · 2017-09-15T06:26:18Z

src/Native/Runtime/inc/rhbinder.h

@@ -646,6 +657,7 @@ struct PInvokeTransitionFrame
 #endif
    UIntTarget      m_PreservedRegs[];
 };
+#endif


hippiehunter · 2017-09-15T19:45:02Z

updated for comments

jkotas

Thanks!

dnfclas added the cla-already-signed label Sep 15, 2017

jkotas reviewed Sep 15, 2017

View reviewed changes

hippiehunter force-pushed the master branch 2 times, most recently from 14204b5 to bf53dc5 Compare September 15, 2017 04:28

jkotas reviewed Sep 15, 2017

View reviewed changes

Implemented portable pinvoke infrastructure for CppCodeGen

10698e4

hippiehunter force-pushed the master branch from bf53dc5 to 10698e4 Compare September 15, 2017 16:48

jkotas approved these changes Sep 15, 2017

View reviewed changes

jkotas merged commit 3a1a99a into dotnet:master Sep 15, 2017

jkotas mentioned this pull request Sep 15, 2017

[CppCodeGen] Thread unable to start due to assert #4428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented portable PInvoke infrastructure for CppCodeGen #4503

implemented portable PInvoke infrastructure for CppCodeGen #4503

hippiehunter commented Sep 15, 2017

dnfclas commented Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

jkotas Sep 15, 2017

hippiehunter commented Sep 15, 2017

jkotas commented Sep 15, 2017 •

edited

Loading

jkotas Sep 15, 2017

jkotas Sep 15, 2017

hippiehunter commented Sep 15, 2017

jkotas left a comment

		@@ -41,4 +41,33 @@ inline double __uint64_to_double(uint64_t v)
		return val.d;
		}

implemented portable PInvoke infrastructure for CppCodeGen #4503

implemented portable PInvoke infrastructure for CppCodeGen #4503

Conversation

hippiehunter commented Sep 15, 2017

dnfclas commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hippiehunter commented Sep 15, 2017

jkotas commented Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hippiehunter commented Sep 15, 2017

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Sep 15, 2017 •

edited

Loading