From 673a0839429ff3fdd79a87190ea557b32ea51ef4 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sat, 26 Oct 2019 19:00:25 -0700 Subject: [PATCH 1/6] Revert removal of SuppressGCTransition from SystemNative_GetTimestamp() --- .../shared/Interop/Unix/System.Native/Interop.GetTimestamp.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Private.CoreLib/shared/Interop/Unix/System.Native/Interop.GetTimestamp.cs b/src/System.Private.CoreLib/shared/Interop/Unix/System.Native/Interop.GetTimestamp.cs index 60968d824b9c..53acdbc3cc3b 100644 --- a/src/System.Private.CoreLib/shared/Interop/Unix/System.Native/Interop.GetTimestamp.cs +++ b/src/System.Private.CoreLib/shared/Interop/Unix/System.Native/Interop.GetTimestamp.cs @@ -12,7 +12,7 @@ internal static partial class Sys internal static extern ulong GetTimestampResolution(); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetTimestamp", ExactSpelling = true)] - // [SuppressGCTransition] // https://github.com/dotnet/coreclr/issues/27465 + [SuppressGCTransition] internal static extern ulong GetTimestamp(); } } From 8a8ef7ad6755bdd9888d00245109aaf7029d626c Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sun, 27 Oct 2019 14:51:23 -0700 Subject: [PATCH 2/6] Insert the GC_POLL as a new statement immediately after the statement enclosing the unmanaged call. --- src/jit/compiler.h | 2 +- src/jit/flowgraph.cpp | 29 ++++++++++++++++++++++++++--- src/jit/morph.cpp | 2 +- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/jit/compiler.h b/src/jit/compiler.h index d989863b0e9c..947236b8bc10 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -4827,7 +4827,7 @@ class Compiler bool fgGCPollsCreated; void fgMarkGCPollBlocks(); void fgCreateGCPolls(); - bool fgCreateGCPoll(GCPollType pollType, BasicBlock* block); + bool fgCreateGCPoll(GCPollType pollType, BasicBlock* block, Statement *stmt = nullptr); // Requires that "block" is a block that returns from // a finally. Returns the number of successors (jump targets of diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index 3d877a61d224..a2d89f525b31 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -3858,7 +3858,7 @@ void Compiler::fgCreateGCPolls() * a basic block. */ -bool Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block) +bool Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block, Statement *stmt) { bool createdPollBlocks; @@ -3883,15 +3883,38 @@ bool Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block) pollType = GCPOLL_CALL; } +#ifdef DEBUG + // If a statment was supplied it should be contained in the block. + if (stmt != nullptr) + { + bool containsStmt = false; + for (Statement* stmtMaybe : block->Statements()) + { + containsStmt = (stmtMaybe == stmt); + if (containsStmt) + { + break; + } + } + + noway_assert(containsStmt); + } +#endif + if (GCPOLL_CALL == pollType) { createdPollBlocks = false; GenTreeCall* call = gtNewHelperCallNode(CORINFO_HELP_POLL_GC, TYP_VOID); GenTree* temp = fgMorphCall(call); - // for BBJ_ALWAYS I don't need to insert it before the condition. Just append it. - if (block->bbJumpKind == BBJ_ALWAYS) + if (stmt != nullptr) + { + Statement* newStmt = gtNewStmt(temp); + fgInsertStmtAfter(block, stmt, newStmt); + } + else if (block->bbJumpKind == BBJ_ALWAYS) { + // for BBJ_ALWAYS I don't need to insert it before the condition. Just append it. fgNewStmtAtEnd(block, temp); } else diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index 13ceb53dd642..78ebcfd58624 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -8226,7 +8226,7 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call) if (fgGlobalMorph && call->IsUnmanaged() && call->IsSuppressGCTransition()) { // Insert a GC poll. - bool insertedBB = fgCreateGCPoll(GCPOLL_CALL, compCurBB); + bool insertedBB = fgCreateGCPoll(GCPOLL_CALL, compCurBB, compCurStmt); assert(!insertedBB); // No new block should be inserted } From bb28a07fef3679c8d35246fcda1290a91e4e7920 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sun, 27 Oct 2019 15:02:54 -0700 Subject: [PATCH 3/6] Apply format to JIT code. --- src/jit/compiler.h | 2 +- src/jit/flowgraph.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 947236b8bc10..a0fb7ca63fd5 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -4827,7 +4827,7 @@ class Compiler bool fgGCPollsCreated; void fgMarkGCPollBlocks(); void fgCreateGCPolls(); - bool fgCreateGCPoll(GCPollType pollType, BasicBlock* block, Statement *stmt = nullptr); + bool fgCreateGCPoll(GCPollType pollType, BasicBlock* block, Statement* stmt = nullptr); // Requires that "block" is a block that returns from // a finally. Returns the number of successors (jump targets of diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index a2d89f525b31..f3ef760e1730 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -3858,7 +3858,7 @@ void Compiler::fgCreateGCPolls() * a basic block. */ -bool Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block, Statement *stmt) +bool Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block, Statement* stmt) { bool createdPollBlocks; From 310f9018f52b677cb650bbcfdba580daa1a6c276 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sun, 27 Oct 2019 20:23:27 -0700 Subject: [PATCH 4/6] Insert GC_POLL before statement with unmanaged call. Review feedback --- src/jit/flowgraph.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index f3ef760e1730..e18759086e01 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -3897,7 +3897,7 @@ bool Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block, Statement* } } - noway_assert(containsStmt); + assert(containsStmt); } #endif @@ -3909,8 +3909,14 @@ bool Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block, Statement* if (stmt != nullptr) { + // The GC_POLL should be inserted relative to the supplied statement. The safer + // location for the insertion is prior to the current statement since the supplied + // statement could be a GT_JTRUE (see fgNewStmtNearEnd() for more details). Statement* newStmt = gtNewStmt(temp); - fgInsertStmtAfter(block, stmt, newStmt); + + // Set the GC_POLL statement to have the same IL offset at the subsequent one. + newStmt->SetILOffsetX(stmt->GetILOffsetX()); + fgInsertStmtBefore(block, stmt, newStmt); } else if (block->bbJumpKind == BBJ_ALWAYS) { From 64e751657dd3a60c84ddcdde8356e3ef73fa46e1 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Mon, 28 Oct 2019 19:58:53 -0700 Subject: [PATCH 5/6] JIT test for insertion of GCPoll --- .../src/JIT/Methodical/gc_poll/CMakeLists.txt | 11 ++ .../JIT/Methodical/gc_poll/GCPollNative.cpp | 36 ++++++ .../JIT/Methodical/gc_poll/InsertGCPoll.cs | 108 ++++++++++++++++++ .../Methodical/gc_poll/InsertGCPoll.csproj | 15 +++ 4 files changed, 170 insertions(+) create mode 100644 tests/src/JIT/Methodical/gc_poll/CMakeLists.txt create mode 100644 tests/src/JIT/Methodical/gc_poll/GCPollNative.cpp create mode 100644 tests/src/JIT/Methodical/gc_poll/InsertGCPoll.cs create mode 100644 tests/src/JIT/Methodical/gc_poll/InsertGCPoll.csproj diff --git a/tests/src/JIT/Methodical/gc_poll/CMakeLists.txt b/tests/src/JIT/Methodical/gc_poll/CMakeLists.txt new file mode 100644 index 000000000000..b832809e0643 --- /dev/null +++ b/tests/src/JIT/Methodical/gc_poll/CMakeLists.txt @@ -0,0 +1,11 @@ +cmake_minimum_required (VERSION 2.6) +project (GCPollNative) + +set(SOURCES + GCPollNative.cpp +) + +add_library (GCPollNative SHARED ${SOURCES}) + +# add the install targets +install (TARGETS GCPollNative DESTINATION bin) diff --git a/tests/src/JIT/Methodical/gc_poll/GCPollNative.cpp b/tests/src/JIT/Methodical/gc_poll/GCPollNative.cpp new file mode 100644 index 000000000000..e3e5382f7f45 --- /dev/null +++ b/tests/src/JIT/Methodical/gc_poll/GCPollNative.cpp @@ -0,0 +1,36 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#include +#include +#include + +#if defined(_MSC_VER) + #define STDMETHODCALLTYPE __stdcall + #define EXPORT(type) extern "C" type __declspec(dllexport) +#else + #define STDMETHODCALLTYPE __attribute__((stdcall)) + #define EXPORT(type) extern "C" __attribute__((visibility("default"))) type +#endif + +namespace +{ + std::atomic _n = 0; + + template + T NextUInt(T t) + { + return (T)((++_n) + t); + } +} + +EXPORT(uint32_t) STDMETHODCALLTYPE NextUInt32(uint32_t t) +{ + return NextUInt(t); +} + +EXPORT(uint64_t) STDMETHODCALLTYPE NextUInt64(uint64_t t) +{ + return NextUInt(t); +} \ No newline at end of file diff --git a/tests/src/JIT/Methodical/gc_poll/InsertGCPoll.cs b/tests/src/JIT/Methodical/gc_poll/InsertGCPoll.cs new file mode 100644 index 000000000000..37bd0763dac3 --- /dev/null +++ b/tests/src/JIT/Methodical/gc_poll/InsertGCPoll.cs @@ -0,0 +1,108 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.InteropServices; + +static class GCPollNative +{ + // Simple function that can be marked as SuppressGCTransition which will + // result in a GCPoll insertion. + [DllImport(nameof(GCPollNative))] + [SuppressGCTransition] + public static extern uint NextUInt32(uint n); + + // Simple function that can be marked as SuppressGCTransition which will + // result in a GCPoll insertion. + [DllImport(nameof(GCPollNative))] + [SuppressGCTransition] + public static extern ulong NextUInt64(ulong n); +} + +class InsertGCPoll +{ + private static int PropNextInt32 => (int)GCPollNative.NextUInt32(0); + private static long PropNextInt64 => (long)GCPollNative.NextUInt64(0); + + private static void AccessAsProperty32() + { + int a = PropNextInt32; + int b = PropNextInt32; + DisplayValues(a, b); + } + + private static void AccessAsProperty64() + { + long a = PropNextInt64; + long b = PropNextInt64; + DisplayValues(a, b); + } + + private static void DisplayValues(T a, T b) + { + Console.WriteLine($"{a} {b}"); + } + + private static void BranchOnProperty32() + { + if (-1 == PropNextInt32) + { + Console.WriteLine(""); + } + } + + private static void BranchOnProperty64() + { + if (-1 == PropNextInt64) + { + Console.WriteLine(""); + } + } + + private static void CompoundStatementBranchOnProperty() + { + if (-1 == (PropNextInt64 + PropNextInt32 - PropNextInt64 + PropNextInt64 - PropNextInt32)) + { + Console.WriteLine(""); + } + } + + private static void LoopOn32() + { + uint i = 0; + for (int j = 0; j < 10 || i < 32; ++j) + { + i += GCPollNative.NextUInt32(1); + } + } + + private static void LoopOn64() + { + ulong i = 0; + for (int j = 0; j < 10 || i < 32; ++j) + { + i += GCPollNative.NextUInt64(1); + } + } + + public static int Main() + { + try + { + AccessAsProperty32(); + AccessAsProperty64(); + BranchOnProperty32(); + BranchOnProperty64(); + CompoundStatementBranchOnProperty(); + LoopOn32(); + LoopOn64(); + } + catch (Exception e) + { + Console.WriteLine(e.ToString()); + return 101; + } + return 100; + } +} \ No newline at end of file diff --git a/tests/src/JIT/Methodical/gc_poll/InsertGCPoll.csproj b/tests/src/JIT/Methodical/gc_poll/InsertGCPoll.csproj new file mode 100644 index 000000000000..06d24781fe68 --- /dev/null +++ b/tests/src/JIT/Methodical/gc_poll/InsertGCPoll.csproj @@ -0,0 +1,15 @@ + + + Exe + true + + + True + + + + + + + + \ No newline at end of file From 6f491f8297752d391c9f85459e78a50f467c09ce Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Mon, 28 Oct 2019 20:49:57 -0700 Subject: [PATCH 6/6] Test build breaks on non-Windows platforms. --- .../JIT/Methodical/gc_poll/GCPollNative.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/src/JIT/Methodical/gc_poll/GCPollNative.cpp b/tests/src/JIT/Methodical/gc_poll/GCPollNative.cpp index e3e5382f7f45..256b267ba2a7 100644 --- a/tests/src/JIT/Methodical/gc_poll/GCPollNative.cpp +++ b/tests/src/JIT/Methodical/gc_poll/GCPollNative.cpp @@ -7,16 +7,24 @@ #include #if defined(_MSC_VER) - #define STDMETHODCALLTYPE __stdcall - #define EXPORT(type) extern "C" type __declspec(dllexport) + +#define STDMETHODCALLTYPE __stdcall +#define EXPORT(type) extern "C" type __declspec(dllexport) + +#else // !defined(_MSC_VER) + +#ifdef __i386__ +#define STDMETHODCALLTYPE __attribute__((stdcall)) #else - #define STDMETHODCALLTYPE __attribute__((stdcall)) - #define EXPORT(type) extern "C" __attribute__((visibility("default"))) type +#define STDMETHODCALLTYPE #endif +#define EXPORT(type) extern "C" __attribute__((visibility("default"))) type + +#endif // defined(_MSC_VER) namespace { - std::atomic _n = 0; + std::atomic _n{ 0 }; template T NextUInt(T t)