From ca5371bd48d60000da7cf053ad427a995e024842 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 26 Jan 2018 20:13:19 +0100 Subject: [PATCH 1/7] Fix ARM/ARM64 hijacking in tail calls This change fixes an issue that can happen when a function that has tail calls is hijacked. There are two potential issues: 1. When a function that tail calls another one is hijacked, the LR may be stored at a different location in the stack frame of the tail call target. So just by performing tail call, the hijacked location becomes invalid and unhijacking would corrupt stack by writing to that location. 2. There is a small window after the caller pops LR from the stack in its epilog and before the tail called function pushes LR in its prolog when the hijacked return address would not be not on the stack and so we would not be able to unhijack. The fix is to prevent hijacking of functions that contain tail calls. --- src/gcdump/gcdumpnonx86.cpp | 3 ++- src/gcinfo/gcinfodumper.cpp | 3 ++- src/gcinfo/gcinfoencoder.cpp | 9 ++++++++- src/inc/eetwain.h | 5 +++++ src/inc/gcinfo.h | 2 +- src/inc/gcinfodecoder.h | 9 +++++++-- src/inc/gcinfoencoder.h | 3 +++ src/inc/gcinfotypes.h | 1 + src/jit/codegencommon.cpp | 1 + src/jit/codegeninterface.h | 11 +++++++++++ src/jit/compiler.h | 10 ++++++++++ src/jit/flowgraph.cpp | 8 ++++++++ src/jit/gcencode.cpp | 14 ++++++++++++++ src/vm/eetwain.cpp | 17 +++++++++++++++++ src/vm/gcinfodecoder.cpp | 21 ++++++++++++++++++++- src/vm/stackwalk.cpp | 11 +++++++++++ src/vm/stackwalk.h | 1 + src/vm/threadsuspend.cpp | 15 +++++++++++++++ 18 files changed, 137 insertions(+), 7 deletions(-) diff --git a/src/gcdump/gcdumpnonx86.cpp b/src/gcdump/gcdumpnonx86.cpp index ca8cc75792b0..7f0c0015f921 100644 --- a/src/gcdump/gcdumpnonx86.cpp +++ b/src/gcdump/gcdumpnonx86.cpp @@ -434,7 +434,8 @@ size_t GCDump::DumpGCTable(PTR_CBYTE gcInfoBlock, : GetRegName(hdrdecoder.GetStackBaseRegister())); gcPrintf("Wants Report Only Leaf: %u\n", hdrdecoder.WantsReportOnlyLeaf()); - + gcPrintf("Has tailcalls: %u\n", hdrdecoder.HasTailCalls()); + #ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA gcPrintf("Size of parameter area: %x\n", hdrdecoder.GetSizeOfStackParameterArea()); #endif diff --git a/src/gcinfo/gcinfodumper.cpp b/src/gcinfo/gcinfodumper.cpp index 3bd2f5cb6153..454bea2056c4 100644 --- a/src/gcinfo/gcinfodumper.cpp +++ b/src/gcinfo/gcinfodumper.cpp @@ -650,7 +650,8 @@ PORTABILITY_ASSERT("GcInfoDumper::EnumerateStateChanges is not implemented on th (GcInfoDecoderFlags)( DECODE_SECURITY_OBJECT | DECODE_CODE_LENGTH | DECODE_VARARG - | DECODE_INTERRUPTIBILITY), + | DECODE_INTERRUPTIBILITY + | DECODE_HAS_TAILCALLS), offset); fNewInterruptible = decoder1.IsInterruptible(); diff --git a/src/gcinfo/gcinfoencoder.cpp b/src/gcinfo/gcinfoencoder.cpp index 15d6ed9ff93d..9e049feaf3c7 100644 --- a/src/gcinfo/gcinfoencoder.cpp +++ b/src/gcinfo/gcinfoencoder.cpp @@ -488,6 +488,7 @@ GcInfoEncoder::GcInfoEncoder( m_SizeOfEditAndContinuePreservedArea = NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA; m_ReversePInvokeFrameSlot = NO_REVERSE_PINVOKE_FRAME; m_WantsReportOnlyLeaf = false; + m_HasTailCalls = false; m_IsVarArg = false; m_pLastInterruptibleRange = NULL; @@ -757,6 +758,11 @@ void GcInfoEncoder::SetWantsReportOnlyLeaf() m_WantsReportOnlyLeaf = true; } +void GcInfoEncoder::SetHasTailCalls() +{ + m_HasTailCalls = true; +} + #ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA void GcInfoEncoder::SetSizeOfStackOutgoingAndScratchArea( UINT32 size ) { @@ -1013,7 +1019,7 @@ void GcInfoEncoder::Build() !hasContextParamType && !m_WantsReportOnlyLeaf && (m_InterruptibleRanges.Count() == 0) && !hasReversePInvokeFrame && ((m_StackBaseRegister == NO_STACK_BASE_REGISTER) || (NORMALIZE_STACK_BASE_REGISTER(m_StackBaseRegister) == 0))) && (m_SizeOfEditAndContinuePreservedArea == NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) && - !IsStructReturnKind(m_ReturnKind); + !IsStructReturnKind(m_ReturnKind) && !m_HasTailCalls; // All new code is generated for the latest GCINFO_VERSION. // So, always encode RetunrKind and encode ReversePInvokeFrameSlot where applicable. @@ -1037,6 +1043,7 @@ void GcInfoEncoder::Build() GCINFO_WRITE(m_Info1, (m_WantsReportOnlyLeaf ? 1 : 0), 1, FlagsSize); GCINFO_WRITE(m_Info1, ((m_SizeOfEditAndContinuePreservedArea != NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) ? 1 : 0), 1, FlagsSize); GCINFO_WRITE(m_Info1, (hasReversePInvokeFrame ? 1 : 0), 1, FlagsSize); + GCINFO_WRITE(m_Info1, (m_HasTailCalls ? 1 : 0), 1, FlagsSize); GCINFO_WRITE(m_Info1, m_ReturnKind, SIZE_OF_RETURN_KIND_IN_FAT_HEADER, RetKindSize); } diff --git a/src/inc/eetwain.h b/src/inc/eetwain.h index 97a742b576d9..305cbbfd449c 100644 --- a/src/inc/eetwain.h +++ b/src/inc/eetwain.h @@ -214,6 +214,8 @@ virtual bool UnwindStackFrame(PREGDISPLAY pContext, virtual bool IsGcSafe(EECodeInfo *pCodeInfo, DWORD dwRelOffset) = 0; +virtual bool HasTailCalls(EECodeInfo *pCodeInfo) = 0; + #if defined(_TARGET_AMD64_) && defined(_DEBUG) /* Locates the end of the last interruptible region in the given code range. @@ -470,6 +472,9 @@ virtual bool IsGcSafe( EECodeInfo *pCodeInfo, DWORD dwRelOffset); +virtual +bool HasTailCalls(EECodeInfo *pCodeInfo); + #if defined(_TARGET_AMD64_) && defined(_DEBUG) /* Locates the end of the last interruptible region in the given code range. diff --git a/src/inc/gcinfo.h b/src/inc/gcinfo.h index 901f2cfc0762..83b7307dfce4 100644 --- a/src/inc/gcinfo.h +++ b/src/inc/gcinfo.h @@ -34,7 +34,7 @@ const unsigned this_OFFSET_FLAG = 0x2; // the offset is "this" // The current GCInfo Version //----------------------------------------------------------------------------- -#define GCINFO_VERSION 2 +#define GCINFO_VERSION 3 #define MIN_GCINFO_VERSION_WITH_RETURN_KIND 2 #define MIN_GCINFO_VERSION_WITH_REV_PINVOKE_FRAME 2 diff --git a/src/inc/gcinfodecoder.h b/src/inc/gcinfodecoder.h index fccf67a625b5..ccfb142d67a1 100644 --- a/src/inc/gcinfodecoder.h +++ b/src/inc/gcinfodecoder.h @@ -202,7 +202,8 @@ enum GcInfoDecoderFlags DECODE_PROLOG_LENGTH = 0x400, // length of the prolog (used to avoid reporting generics context) DECODE_EDIT_AND_CONTINUE = 0x800, DECODE_REVERSE_PINVOKE_VAR = 0x1000, - DECODE_RETURN_KIND = 0x2000 + DECODE_RETURN_KIND = 0x2000, + DECODE_HAS_TAILCALLS = 0x4000 }; enum GcInfoHeaderFlags @@ -220,9 +221,11 @@ enum GcInfoHeaderFlags GC_INFO_WANTS_REPORT_ONLY_LEAF = 0x80, GC_INFO_HAS_EDIT_AND_CONTINUE_PRESERVED_SLOTS = 0x100, GC_INFO_REVERSE_PINVOKE_FRAME = 0x200, + GC_INFO_HAS_TAILCALLS = 0x400, GC_INFO_FLAGS_BIT_SIZE_VERSION_1 = 9, - GC_INFO_FLAGS_BIT_SIZE = 10, + GC_INFO_FLAGS_BIT_SIZE_VERSION_2 = 10, + GC_INFO_FLAGS_BIT_SIZE = 11, }; class BitStreamReader @@ -520,6 +523,7 @@ class GcInfoDecoder bool HasMethodTableGenericsInstContext(); bool GetIsVarArg(); bool WantsReportOnlyLeaf(); + bool HasTailCalls(); ReturnKind GetReturnKind(); UINT32 GetCodeLength(); UINT32 GetStackBaseRegister(); @@ -538,6 +542,7 @@ class GcInfoDecoder // Pre-decoded information bool m_IsInterruptible; bool m_IsVarArg; + bool m_HasTailCalls; bool m_GenericSecretParamIsMD; bool m_GenericSecretParamIsMT; bool m_WantsReportOnlyLeaf; diff --git a/src/inc/gcinfoencoder.h b/src/inc/gcinfoencoder.h index d09f43058f42..092d4576b452 100644 --- a/src/inc/gcinfoencoder.h +++ b/src/inc/gcinfoencoder.h @@ -441,6 +441,8 @@ class GcInfoEncoder // Called only by RyuJIT (not JIT64) void SetWantsReportOnlyLeaf(); + void SetHasTailCalls(); + #ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA void SetSizeOfStackOutgoingAndScratchArea( UINT32 size ); #endif // FIXED_STACK_PARAMETER_SCRATCH_AREA @@ -491,6 +493,7 @@ class GcInfoEncoder GcInfoArrayList m_LifetimeTransitions; bool m_IsVarArg; + bool m_HasTailCalls; bool m_WantsReportOnlyLeaf; INT32 m_SecurityObjectStackSlot; INT32 m_GSCookieStackSlot; diff --git a/src/inc/gcinfotypes.h b/src/inc/gcinfotypes.h index c802d97ec689..5b051143842f 100644 --- a/src/inc/gcinfotypes.h +++ b/src/inc/gcinfotypes.h @@ -442,6 +442,7 @@ struct InfoHdrSmall { unsigned char genericsContext : 1;//4 [1] function reports a generics context parameter is present unsigned char genericsContextIsMethodDesc : 1;//4[2] unsigned char returnKind : 2; // 4 [4] Available GcInfo v2 onwards, previously undefined + unsigned char hasTailCalls : 1; // 4 [5] unsigned short argCount; // 5,6 in bytes unsigned int frameSize; // 7,8,9,10 in bytes unsigned int untrackedCnt; // 11,12,13,14 diff --git a/src/jit/codegencommon.cpp b/src/jit/codegencommon.cpp index e0046d2279fb..043ca8dc04b7 100644 --- a/src/jit/codegencommon.cpp +++ b/src/jit/codegencommon.cpp @@ -180,6 +180,7 @@ CodeGen::CodeGen(Compiler* theCompiler) : CodeGenInterface(theCompiler) /* Assume that we not fully interruptible */ genInterruptible = false; + hasTailCalls = false; #ifdef DEBUG genInterruptibleUsed = false; genCurDispOffset = (unsigned)-1; diff --git a/src/jit/codegeninterface.h b/src/jit/codegeninterface.h index 5c84a4d2a8b5..2d8c2944cd8e 100644 --- a/src/jit/codegeninterface.h +++ b/src/jit/codegeninterface.h @@ -408,8 +408,19 @@ class CodeGenInterface m_cgInterruptible = value; } + __declspec(property(get = getHasTailCalls, put = setHasTailCalls)) bool hasTailCalls; + bool getHasTailCalls() + { + return m_cgHasTailCalls; + } + void setHasTailCalls(bool value) + { + m_cgHasTailCalls = value; + } + private: bool m_cgInterruptible; + bool m_cgHasTailCalls; // The following will be set to true if we've determined that we need to // generate a full-blown pointer register map for the current method. diff --git a/src/jit/compiler.h b/src/jit/compiler.h index a97530c6c12a..e307d90d5df3 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -7148,6 +7148,16 @@ class Compiler codeGen->setInterruptible(value); } + __declspec(property(get = getHasTailCalls, put = setHasTailCalls)) bool hasTailCalls; + bool getHasTailCalls() + { + return codeGen->hasTailCalls; + } + void setHasTailCalls(bool value) + { + codeGen->setHasTailCalls(value); + } + #if DOUBLE_ALIGN const bool genDoubleAlign() { diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index 4a5e03279a24..2b3b8ad15b76 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -18969,6 +18969,14 @@ void Compiler::fgSetBlockOrder() // JIT64 does. genInterruptible = true; } + + GenTreePtr tailCall = nullptr; + bool fastTailCallsOnly = true; + bool tailCallsConvertibleToLoopOnly = false; + if (!hasTailCalls && block->endsWithTailCall(this, fastTailCallsOnly, tailCallsConvertibleToLoopOnly, &tailCall)) + { + hasTailCalls = true; + } #endif // !JIT32_GCENCODER #endif // FEATURE_FASTTAILCALL diff --git a/src/jit/gcencode.cpp b/src/jit/gcencode.cpp index 6140773f36be..3c7dce9d987d 100644 --- a/src/jit/gcencode.cpp +++ b/src/jit/gcencode.cpp @@ -3917,6 +3917,15 @@ class GcInfoEncoderWithLogging } } + void SetHasTailCalls() + { + m_gcInfoEncoder->SetHasTailCalls(); + if (m_doLogging) + { + printf("Set HasTailCalls.\n"); + } + } + void SetSizeOfStackOutgoingAndScratchArea(UINT32 size) { m_gcInfoEncoder->SetSizeOfStackOutgoingAndScratchArea(size); @@ -4057,6 +4066,11 @@ void GCInfo::gcInfoBlockHdrSave(GcInfoEncoder* gcInfoEncoder, unsigned methodSiz } #endif // FEATURE_EH_FUNCLETS + if (compiler->codeGen->hasTailCalls) + { + gcInfoEncoderWithLog->SetHasTailCalls(); + } + #if FEATURE_FIXED_OUT_ARGS // outgoing stack area size gcInfoEncoderWithLog->SetSizeOfStackOutgoingAndScratchArea(compiler->lvaOutgoingArgSpaceSize); diff --git a/src/vm/eetwain.cpp b/src/vm/eetwain.cpp index 9fe9670e6ea4..18fdf253e970 100644 --- a/src/vm/eetwain.cpp +++ b/src/vm/eetwain.cpp @@ -1451,6 +1451,23 @@ bool EECodeManager::IsGcSafe( EECodeInfo *pCodeInfo, return gcInfoDecoder.IsInterruptible(); } +bool EECodeManager::HasTailCalls( EECodeInfo *pCodeInfo) +{ + CONTRACTL { + NOTHROW; + GC_NOTRIGGER; + } CONTRACTL_END; + + GCInfoToken gcInfoToken = pCodeInfo->GetGCInfoToken(); + + GcInfoDecoder gcInfoDecoder( + gcInfoToken, + DECODE_HAS_TAILCALLS, + 0 + ); + + return gcInfoDecoder.HasTailCalls(); +} #if defined(_TARGET_AMD64_) && defined(_DEBUG) diff --git a/src/vm/gcinfodecoder.cpp b/src/vm/gcinfodecoder.cpp index 558247c9bf9f..ffeae645e1ae 100644 --- a/src/vm/gcinfodecoder.cpp +++ b/src/vm/gcinfodecoder.cpp @@ -113,7 +113,19 @@ GcInfoDecoder::GcInfoDecoder( } else { - int numFlagBits = (m_Version == 1) ? GC_INFO_FLAGS_BIT_SIZE_VERSION_1 : GC_INFO_FLAGS_BIT_SIZE; + int numFlagBits; + switch (m_Version) + { + case 1: + numFlagBits = GC_INFO_FLAGS_BIT_SIZE_VERSION_1; + break; + case 2: + numFlagBits = GC_INFO_FLAGS_BIT_SIZE_VERSION_2; + break; + default: + numFlagBits = GC_INFO_FLAGS_BIT_SIZE; + break; + } headerFlags = (GcInfoHeaderFlags) m_Reader.Read(numFlagBits); } @@ -126,6 +138,7 @@ GcInfoDecoder::GcInfoDecoder( m_GenericSecretParamIsMT = (headerFlags & GC_INFO_HAS_GENERICS_INST_CONTEXT_MASK) == GC_INFO_HAS_GENERICS_INST_CONTEXT_MT; int hasStackBaseRegister = headerFlags & GC_INFO_HAS_STACK_BASE_REGISTER; m_WantsReportOnlyLeaf = ((headerFlags & GC_INFO_WANTS_REPORT_ONLY_LEAF) != 0); + m_HasTailCalls = ((headerFlags & GC_INFO_HAS_TAILCALLS) != 0); int hasSizeOfEditAndContinuePreservedArea = headerFlags & GC_INFO_HAS_EDIT_AND_CONTINUE_PRESERVED_SLOTS; int hasReversePInvokeFrame = false; @@ -346,6 +359,12 @@ bool GcInfoDecoder::IsInterruptible() return m_IsInterruptible; } +bool GcInfoDecoder::HasTailCalls() +{ + _ASSERTE( m_Flags & DECODE_HAS_TAILCALLS ); + return m_HasTailCalls; +} + bool GcInfoDecoder::HasMethodDescGenericsInstContext() { _ASSERTE( m_Flags & DECODE_GENERICS_INST_CONTEXT ); diff --git a/src/vm/stackwalk.cpp b/src/vm/stackwalk.cpp index 13e25d5c9d4d..315cfb10ce2a 100644 --- a/src/vm/stackwalk.cpp +++ b/src/vm/stackwalk.cpp @@ -353,6 +353,17 @@ bool CrawlFrame::IsGcSafe() return GetCodeManager()->IsGcSafe(&codeInfo, GetRelOffset()); } +bool CrawlFrame::HasTailCalls() +{ + CONTRACTL { + NOTHROW; + GC_NOTRIGGER; + SUPPORTS_DAC; + } CONTRACTL_END; + + return GetCodeManager()->HasTailCalls(&codeInfo); +} + inline void CrawlFrame::GotoNextFrame() { CONTRACTL { diff --git a/src/vm/stackwalk.h b/src/vm/stackwalk.h index 8dd8f1b93fab..db99bb2d8706 100644 --- a/src/vm/stackwalk.h +++ b/src/vm/stackwalk.h @@ -310,6 +310,7 @@ class CrawlFrame */ bool IsGcSafe(); + bool HasTailCalls(); PREGDISPLAY GetRegisterSet() { diff --git a/src/vm/threadsuspend.cpp b/src/vm/threadsuspend.cpp index 7ac50e3b00ae..1dc2489e2f36 100644 --- a/src/vm/threadsuspend.cpp +++ b/src/vm/threadsuspend.cpp @@ -6109,6 +6109,7 @@ struct ExecutionState IJitManager *m_pJitManager; METHODTOKEN m_MethodToken; BOOL m_IsInterruptible; // is this code interruptible? + BOOL m_HasTailCalls; // does this code perform tail calls? ExecutionState() : m_FirstPass(TRUE) {LIMITED_METHOD_CONTRACT; } }; @@ -6229,6 +6230,7 @@ StackWalkAction SWCB_GetExecutionState(CrawlFrame *pCF, VOID *pData) pES->m_IsInterruptible = pCF->IsGcSafe(); pES->m_RelOffset = pCF->GetRelOffset(); pES->m_pJitManager = pCF->GetJitManager(); + pES->m_HasTailCalls = pCF->HasTailCalls(); STRESS_LOG3(LF_SYNC, LL_INFO1000, "Stopped in Jitted code at pc = %p sp = %p fullyInt=%d\n", GetControlPC(pCF->GetRegisterSet()), GetRegdisplaySP(pCF->GetRegisterSet()), pES->m_IsInterruptible); @@ -6304,6 +6306,19 @@ StackWalkAction SWCB_GetExecutionState(CrawlFrame *pCF, VOID *pData) // For this, we may need JIT support to do so. notJittedCase = true; } + else if (pES->m_HasTailCalls) + { + // Do not hijack functions that have tail calls, since there are two problems: + // 1. When a function that tail calls another one is hijacked, the LR may be + // stored at a different location in the stack frame of the tail call target. + // So just by performing tail call, the hijacked location becomes invalid and + // unhijacking would corrupt stack by writing to that location. + // 2. There is a small window after the caller pops LR from the stack in its + // epilog and before the tail called function pushes LR in its prolog when + // the hijacked return address would not be not on the stack and so we would + // not be able to unhijack. + notJittedCase = true; + } else { // This is the case of IP being inside the method body and LR is From 91642b767e1f8ee4729960ead245cdaf22fb87f6 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 26 Jan 2018 21:56:10 +0100 Subject: [PATCH 2/7] Reflect PR feedback --- src/gcdump/gcdumpnonx86.cpp | 4 +++- src/gcinfo/gcinfodumper.cpp | 7 ++++-- src/gcinfo/gcinfoencoder.cpp | 21 +++++++++++++---- src/inc/eetwain.h | 8 +++++-- src/inc/gcinfo.h | 2 +- src/inc/gcinfodecoder.h | 19 +++++++++++---- src/inc/gcinfoencoder.h | 12 +++++++--- src/inc/gcinfotypes.h | 1 - src/jit/codegencommon.cpp | 2 ++ src/jit/codegeninterface.h | 4 ++++ src/jit/compiler.h | 2 ++ src/jit/flowgraph.cpp | 3 +++ src/jit/gcencode.cpp | 11 +++++++-- src/jit/legacynonjit/CMakeLists.txt | 2 +- src/vm/eetwain.cpp | 2 ++ src/vm/gcinfodecoder.cpp | 36 ++++++++++++++--------------- src/vm/stackwalk.cpp | 2 ++ src/vm/stackwalk.h | 2 ++ src/vm/threadsuspend.cpp | 4 ++++ 19 files changed, 103 insertions(+), 41 deletions(-) diff --git a/src/gcdump/gcdumpnonx86.cpp b/src/gcdump/gcdumpnonx86.cpp index 7f0c0015f921..09f7ffdbe380 100644 --- a/src/gcdump/gcdumpnonx86.cpp +++ b/src/gcdump/gcdumpnonx86.cpp @@ -433,9 +433,11 @@ size_t GCDump::DumpGCTable(PTR_CBYTE gcInfoBlock, ? "" : GetRegName(hdrdecoder.GetStackBaseRegister())); +#ifdef _TARGET_AMD64_ gcPrintf("Wants Report Only Leaf: %u\n", hdrdecoder.WantsReportOnlyLeaf()); +#else // _TARGET_AMD64_ gcPrintf("Has tailcalls: %u\n", hdrdecoder.HasTailCalls()); - +#endif // _TARGET_AMD64_ #ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA gcPrintf("Size of parameter area: %x\n", hdrdecoder.GetSizeOfStackParameterArea()); #endif diff --git a/src/gcinfo/gcinfodumper.cpp b/src/gcinfo/gcinfodumper.cpp index 454bea2056c4..a9a096f8108b 100644 --- a/src/gcinfo/gcinfodumper.cpp +++ b/src/gcinfo/gcinfodumper.cpp @@ -650,8 +650,11 @@ PORTABILITY_ASSERT("GcInfoDumper::EnumerateStateChanges is not implemented on th (GcInfoDecoderFlags)( DECODE_SECURITY_OBJECT | DECODE_CODE_LENGTH | DECODE_VARARG - | DECODE_INTERRUPTIBILITY - | DECODE_HAS_TAILCALLS), +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) + | DECODE_HAS_TAILCALLS +#endif // _TARGET_ARM_ || _TARGET_ARM64_ + + | DECODE_INTERRUPTIBILITY), offset); fNewInterruptible = decoder1.IsInterruptible(); diff --git a/src/gcinfo/gcinfoencoder.cpp b/src/gcinfo/gcinfoencoder.cpp index 9e049feaf3c7..9e07d1b69f14 100644 --- a/src/gcinfo/gcinfoencoder.cpp +++ b/src/gcinfo/gcinfoencoder.cpp @@ -487,8 +487,11 @@ GcInfoEncoder::GcInfoEncoder( m_StackBaseRegister = NO_STACK_BASE_REGISTER; m_SizeOfEditAndContinuePreservedArea = NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA; m_ReversePInvokeFrameSlot = NO_REVERSE_PINVOKE_FRAME; +#ifdef _TARGET_AMD64_ m_WantsReportOnlyLeaf = false; +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) m_HasTailCalls = false; +#endif // _TARGET_AMD64_ m_IsVarArg = false; m_pLastInterruptibleRange = NULL; @@ -753,15 +756,17 @@ void GcInfoEncoder::SetSizeOfEditAndContinuePreservedArea( UINT32 slots ) m_SizeOfEditAndContinuePreservedArea = slots; } +#ifdef _TARGET_AMD64_ void GcInfoEncoder::SetWantsReportOnlyLeaf() { m_WantsReportOnlyLeaf = true; } - +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) void GcInfoEncoder::SetHasTailCalls() { m_HasTailCalls = true; } +#endif // _TARGET_AMD64_ #ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA void GcInfoEncoder::SetSizeOfStackOutgoingAndScratchArea( UINT32 size ) @@ -1016,10 +1021,15 @@ void GcInfoEncoder::Build() UINT32 hasReversePInvokeFrame = (m_ReversePInvokeFrameSlot != NO_REVERSE_PINVOKE_FRAME); BOOL slimHeader = (!m_IsVarArg && !hasSecurityObject && !hasGSCookie && (m_PSPSymStackSlot == NO_PSP_SYM) && - !hasContextParamType && !m_WantsReportOnlyLeaf && (m_InterruptibleRanges.Count() == 0) && !hasReversePInvokeFrame && + !hasContextParamType && (m_InterruptibleRanges.Count() == 0) && !hasReversePInvokeFrame && ((m_StackBaseRegister == NO_STACK_BASE_REGISTER) || (NORMALIZE_STACK_BASE_REGISTER(m_StackBaseRegister) == 0))) && (m_SizeOfEditAndContinuePreservedArea == NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) && - !IsStructReturnKind(m_ReturnKind) && !m_HasTailCalls; +#ifdef _TARGET_AMD64_ + !m_WantsReportOnlyLeaf && +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) + !m_HasTailCalls && +#endif // _TARGET_AMD64_ + !IsStructReturnKind(m_ReturnKind); // All new code is generated for the latest GCINFO_VERSION. // So, always encode RetunrKind and encode ReversePInvokeFrameSlot where applicable. @@ -1040,10 +1050,13 @@ void GcInfoEncoder::Build() GCINFO_WRITE(m_Info1, ((m_PSPSymStackSlot != NO_PSP_SYM) ? 1 : 0), 1, FlagsSize); GCINFO_WRITE(m_Info1, m_contextParamType, 2, FlagsSize); GCINFO_WRITE(m_Info1, ((m_StackBaseRegister != NO_STACK_BASE_REGISTER) ? 1 : 0), 1, FlagsSize); +#ifdef _TARGET_AMD64_ GCINFO_WRITE(m_Info1, (m_WantsReportOnlyLeaf ? 1 : 0), 1, FlagsSize); +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) + GCINFO_WRITE(m_Info1, (m_HasTailCalls ? 1 : 0), 1, FlagsSize); +#endif // _TARGET_AMD64_ GCINFO_WRITE(m_Info1, ((m_SizeOfEditAndContinuePreservedArea != NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) ? 1 : 0), 1, FlagsSize); GCINFO_WRITE(m_Info1, (hasReversePInvokeFrame ? 1 : 0), 1, FlagsSize); - GCINFO_WRITE(m_Info1, (m_HasTailCalls ? 1 : 0), 1, FlagsSize); GCINFO_WRITE(m_Info1, m_ReturnKind, SIZE_OF_RETURN_KIND_IN_FAT_HEADER, RetKindSize); } diff --git a/src/inc/eetwain.h b/src/inc/eetwain.h index 305cbbfd449c..9597e30e4444 100644 --- a/src/inc/eetwain.h +++ b/src/inc/eetwain.h @@ -214,7 +214,9 @@ virtual bool UnwindStackFrame(PREGDISPLAY pContext, virtual bool IsGcSafe(EECodeInfo *pCodeInfo, DWORD dwRelOffset) = 0; -virtual bool HasTailCalls(EECodeInfo *pCodeInfo) = 0; +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +virtual bool HasTailCalls(EECodeInfo *pCodeInfo) = 0; +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #if defined(_TARGET_AMD64_) && defined(_DEBUG) /* @@ -472,8 +474,10 @@ virtual bool IsGcSafe( EECodeInfo *pCodeInfo, DWORD dwRelOffset); +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) virtual -bool HasTailCalls(EECodeInfo *pCodeInfo); +bool HasTailCalls(EECodeInfo *pCodeInfo); +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #if defined(_TARGET_AMD64_) && defined(_DEBUG) /* diff --git a/src/inc/gcinfo.h b/src/inc/gcinfo.h index 83b7307dfce4..901f2cfc0762 100644 --- a/src/inc/gcinfo.h +++ b/src/inc/gcinfo.h @@ -34,7 +34,7 @@ const unsigned this_OFFSET_FLAG = 0x2; // the offset is "this" // The current GCInfo Version //----------------------------------------------------------------------------- -#define GCINFO_VERSION 3 +#define GCINFO_VERSION 2 #define MIN_GCINFO_VERSION_WITH_RETURN_KIND 2 #define MIN_GCINFO_VERSION_WITH_REV_PINVOKE_FRAME 2 diff --git a/src/inc/gcinfodecoder.h b/src/inc/gcinfodecoder.h index ccfb142d67a1..750563d2ce82 100644 --- a/src/inc/gcinfodecoder.h +++ b/src/inc/gcinfodecoder.h @@ -203,7 +203,9 @@ enum GcInfoDecoderFlags DECODE_EDIT_AND_CONTINUE = 0x800, DECODE_REVERSE_PINVOKE_VAR = 0x1000, DECODE_RETURN_KIND = 0x2000, - DECODE_HAS_TAILCALLS = 0x4000 +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) + DECODE_HAS_TAILCALLS = 0x4000, +#endif // _TARGET_ARM_ || _TARGET_ARM64_ }; enum GcInfoHeaderFlags @@ -218,14 +220,16 @@ enum GcInfoHeaderFlags GC_INFO_HAS_GENERICS_INST_CONTEXT_MD = 0x20, GC_INFO_HAS_GENERICS_INST_CONTEXT_THIS = 0x30, GC_INFO_HAS_STACK_BASE_REGISTER = 0x40, +#ifdef _TARGET_AMD64_ GC_INFO_WANTS_REPORT_ONLY_LEAF = 0x80, +#else // _TARGET_AMD64_ + GC_INFO_HAS_TAILCALLS = 0x80, +#endif // _TARGET_AMD64_ GC_INFO_HAS_EDIT_AND_CONTINUE_PRESERVED_SLOTS = 0x100, GC_INFO_REVERSE_PINVOKE_FRAME = 0x200, - GC_INFO_HAS_TAILCALLS = 0x400, GC_INFO_FLAGS_BIT_SIZE_VERSION_1 = 9, - GC_INFO_FLAGS_BIT_SIZE_VERSION_2 = 10, - GC_INFO_FLAGS_BIT_SIZE = 11, + GC_INFO_FLAGS_BIT_SIZE = 10, }; class BitStreamReader @@ -523,7 +527,9 @@ class GcInfoDecoder bool HasMethodTableGenericsInstContext(); bool GetIsVarArg(); bool WantsReportOnlyLeaf(); +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) bool HasTailCalls(); +#endif // _TARGET_ARM_ || _TARGET_ARM64_ ReturnKind GetReturnKind(); UINT32 GetCodeLength(); UINT32 GetStackBaseRegister(); @@ -542,10 +548,13 @@ class GcInfoDecoder // Pre-decoded information bool m_IsInterruptible; bool m_IsVarArg; - bool m_HasTailCalls; bool m_GenericSecretParamIsMD; bool m_GenericSecretParamIsMT; +#ifdef _TARGET_AMD64_ bool m_WantsReportOnlyLeaf; +#else // _TARGET_AMD64_ + bool m_HasTailCalls; +#endif // _TARGET_AMD64_ INT32 m_SecurityObjectStackSlot; INT32 m_GSCookieStackSlot; INT32 m_ReversePInvokeFrameStackSlot; diff --git a/src/inc/gcinfoencoder.h b/src/inc/gcinfoencoder.h index 092d4576b452..6b095dca4650 100644 --- a/src/inc/gcinfoencoder.h +++ b/src/inc/gcinfoencoder.h @@ -27,7 +27,8 @@ hasPSPSymStackSlot, hasGenericsInstContextStackSlot, hasStackBaseregister, - wantsReportOnlyLeaf, + wantsReportOnlyLeaf (AMD64 JIT64 use only), + hasTailCalls (ARM/ARM64 only) hasSizeOfEditAndContinuePreservedArea hasReversePInvokeFrame, - ReturnKind (Fat: 4 bits) @@ -436,12 +437,14 @@ class GcInfoEncoder // Number of slots preserved during EnC remap void SetSizeOfEditAndContinuePreservedArea( UINT32 size ); +#ifdef _TARGET_AMD64_ // Used to only report a frame once for the leaf function/funclet // instead of once for each live function/funclet on the stack. // Called only by RyuJIT (not JIT64) void SetWantsReportOnlyLeaf(); - +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) void SetHasTailCalls(); +#endif // _TARGET_AMD64_ #ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA void SetSizeOfStackOutgoingAndScratchArea( UINT32 size ); @@ -493,8 +496,11 @@ class GcInfoEncoder GcInfoArrayList m_LifetimeTransitions; bool m_IsVarArg; - bool m_HasTailCalls; +#if defined(_TARGET_AMD64_) bool m_WantsReportOnlyLeaf; +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) + bool m_HasTailCalls; +#endif // _TARGET_AMD64_ INT32 m_SecurityObjectStackSlot; INT32 m_GSCookieStackSlot; UINT32 m_GSCookieValidRangeStart; diff --git a/src/inc/gcinfotypes.h b/src/inc/gcinfotypes.h index 5b051143842f..c802d97ec689 100644 --- a/src/inc/gcinfotypes.h +++ b/src/inc/gcinfotypes.h @@ -442,7 +442,6 @@ struct InfoHdrSmall { unsigned char genericsContext : 1;//4 [1] function reports a generics context parameter is present unsigned char genericsContextIsMethodDesc : 1;//4[2] unsigned char returnKind : 2; // 4 [4] Available GcInfo v2 onwards, previously undefined - unsigned char hasTailCalls : 1; // 4 [5] unsigned short argCount; // 5,6 in bytes unsigned int frameSize; // 7,8,9,10 in bytes unsigned int untrackedCnt; // 11,12,13,14 diff --git a/src/jit/codegencommon.cpp b/src/jit/codegencommon.cpp index 043ca8dc04b7..0d2b809bc595 100644 --- a/src/jit/codegencommon.cpp +++ b/src/jit/codegencommon.cpp @@ -180,7 +180,9 @@ CodeGen::CodeGen(Compiler* theCompiler) : CodeGenInterface(theCompiler) /* Assume that we not fully interruptible */ genInterruptible = false; +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) hasTailCalls = false; +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #ifdef DEBUG genInterruptibleUsed = false; genCurDispOffset = (unsigned)-1; diff --git a/src/jit/codegeninterface.h b/src/jit/codegeninterface.h index 2d8c2944cd8e..f07729d2df93 100644 --- a/src/jit/codegeninterface.h +++ b/src/jit/codegeninterface.h @@ -408,6 +408,7 @@ class CodeGenInterface m_cgInterruptible = value; } +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) __declspec(property(get = getHasTailCalls, put = setHasTailCalls)) bool hasTailCalls; bool getHasTailCalls() { @@ -417,10 +418,13 @@ class CodeGenInterface { m_cgHasTailCalls = value; } +#endif // _TARGET_ARM_ || _TARGET_ARM64_ private: bool m_cgInterruptible; +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) bool m_cgHasTailCalls; +#endif // _TARGET_ARM_ || _TARGET_ARM64_ // The following will be set to true if we've determined that we need to // generate a full-blown pointer register map for the current method. diff --git a/src/jit/compiler.h b/src/jit/compiler.h index e307d90d5df3..ebc85f233006 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -7148,6 +7148,7 @@ class Compiler codeGen->setInterruptible(value); } +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) __declspec(property(get = getHasTailCalls, put = setHasTailCalls)) bool hasTailCalls; bool getHasTailCalls() { @@ -7157,6 +7158,7 @@ class Compiler { codeGen->setHasTailCalls(value); } +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #if DOUBLE_ALIGN const bool genDoubleAlign() diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index 2b3b8ad15b76..6e405509f49f 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -18970,6 +18970,7 @@ void Compiler::fgSetBlockOrder() genInterruptible = true; } +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) GenTreePtr tailCall = nullptr; bool fastTailCallsOnly = true; bool tailCallsConvertibleToLoopOnly = false; @@ -18977,6 +18978,8 @@ void Compiler::fgSetBlockOrder() { hasTailCalls = true; } +#endif // _TARGET_ARM_ || _TARGET_ARM64_ + #endif // !JIT32_GCENCODER #endif // FEATURE_FASTTAILCALL diff --git a/src/jit/gcencode.cpp b/src/jit/gcencode.cpp index 3c7dce9d987d..af007631fda6 100644 --- a/src/jit/gcencode.cpp +++ b/src/jit/gcencode.cpp @@ -3908,6 +3908,7 @@ class GcInfoEncoderWithLogging } } +#ifdef _TARGET_AMD64_ void SetWantsReportOnlyLeaf() { m_gcInfoEncoder->SetWantsReportOnlyLeaf(); @@ -3916,7 +3917,7 @@ class GcInfoEncoderWithLogging printf("Set WantsReportOnlyLeaf.\n"); } } - +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) void SetHasTailCalls() { m_gcInfoEncoder->SetHasTailCalls(); @@ -3925,6 +3926,7 @@ class GcInfoEncoderWithLogging printf("Set HasTailCalls.\n"); } } +#endif // _TARGET_AMD64_ void SetSizeOfStackOutgoingAndScratchArea(UINT32 size) { @@ -4059,18 +4061,23 @@ void GCInfo::gcInfoBlockHdrSave(GcInfoEncoder* gcInfoEncoder, unsigned methodSiz #endif // !_TARGET_AMD64_ } +#ifdef _TARGET_AMD64_ if (compiler->ehAnyFunclets()) { // Set this to avoid double-reporting the parent frame (unlike JIT64) gcInfoEncoderWithLog->SetWantsReportOnlyLeaf(); } +#endif // _TARGET_AMD64_ + #endif // FEATURE_EH_FUNCLETS +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) if (compiler->codeGen->hasTailCalls) { gcInfoEncoderWithLog->SetHasTailCalls(); } - +#endif // _TARGET_ARM_ || _TARGET_ARM64_ + #if FEATURE_FIXED_OUT_ARGS // outgoing stack area size gcInfoEncoderWithLog->SetSizeOfStackOutgoingAndScratchArea(compiler->lvaOutgoingArgSpaceSize); diff --git a/src/jit/legacynonjit/CMakeLists.txt b/src/jit/legacynonjit/CMakeLists.txt index b36800d329cd..9e04174dd22a 100644 --- a/src/jit/legacynonjit/CMakeLists.txt +++ b/src/jit/legacynonjit/CMakeLists.txt @@ -36,7 +36,7 @@ set_property(TARGET legacynonjit APPEND_STRING PROPERTY LINK_DEPENDS ${JIT_EXPOR set(RYUJIT_LINK_LIBRARIES utilcodestaticnohost - gcinfo + gcinfo_arm ) if(CLR_CMAKE_PLATFORM_UNIX) diff --git a/src/vm/eetwain.cpp b/src/vm/eetwain.cpp index 18fdf253e970..bed8a9392686 100644 --- a/src/vm/eetwain.cpp +++ b/src/vm/eetwain.cpp @@ -1451,6 +1451,7 @@ bool EECodeManager::IsGcSafe( EECodeInfo *pCodeInfo, return gcInfoDecoder.IsInterruptible(); } +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) bool EECodeManager::HasTailCalls( EECodeInfo *pCodeInfo) { CONTRACTL { @@ -1468,6 +1469,7 @@ bool EECodeManager::HasTailCalls( EECodeInfo *pCodeInfo) return gcInfoDecoder.HasTailCalls(); } +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #if defined(_TARGET_AMD64_) && defined(_DEBUG) diff --git a/src/vm/gcinfodecoder.cpp b/src/vm/gcinfodecoder.cpp index ffeae645e1ae..ab393d190c7b 100644 --- a/src/vm/gcinfodecoder.cpp +++ b/src/vm/gcinfodecoder.cpp @@ -113,19 +113,7 @@ GcInfoDecoder::GcInfoDecoder( } else { - int numFlagBits; - switch (m_Version) - { - case 1: - numFlagBits = GC_INFO_FLAGS_BIT_SIZE_VERSION_1; - break; - case 2: - numFlagBits = GC_INFO_FLAGS_BIT_SIZE_VERSION_2; - break; - default: - numFlagBits = GC_INFO_FLAGS_BIT_SIZE; - break; - } + int numFlagBits = (m_Version == 1) ? GC_INFO_FLAGS_BIT_SIZE_VERSION_1 : GC_INFO_FLAGS_BIT_SIZE; headerFlags = (GcInfoHeaderFlags) m_Reader.Read(numFlagBits); } @@ -137,8 +125,11 @@ GcInfoDecoder::GcInfoDecoder( m_GenericSecretParamIsMD = (headerFlags & GC_INFO_HAS_GENERICS_INST_CONTEXT_MASK) == GC_INFO_HAS_GENERICS_INST_CONTEXT_MD; m_GenericSecretParamIsMT = (headerFlags & GC_INFO_HAS_GENERICS_INST_CONTEXT_MASK) == GC_INFO_HAS_GENERICS_INST_CONTEXT_MT; int hasStackBaseRegister = headerFlags & GC_INFO_HAS_STACK_BASE_REGISTER; +#ifdef _TARGET_AMD64_ m_WantsReportOnlyLeaf = ((headerFlags & GC_INFO_WANTS_REPORT_ONLY_LEAF) != 0); +#else // _TARGET_AMD64_ m_HasTailCalls = ((headerFlags & GC_INFO_HAS_TAILCALLS) != 0); +#endif // _TARGET_AMD64_ int hasSizeOfEditAndContinuePreservedArea = headerFlags & GC_INFO_HAS_EDIT_AND_CONTINUE_PRESERVED_SLOTS; int hasReversePInvokeFrame = false; @@ -359,12 +350,6 @@ bool GcInfoDecoder::IsInterruptible() return m_IsInterruptible; } -bool GcInfoDecoder::HasTailCalls() -{ - _ASSERTE( m_Flags & DECODE_HAS_TAILCALLS ); - return m_HasTailCalls; -} - bool GcInfoDecoder::HasMethodDescGenericsInstContext() { _ASSERTE( m_Flags & DECODE_GENERICS_INST_CONTEXT ); @@ -546,9 +531,22 @@ bool GcInfoDecoder::GetIsVarArg() return m_IsVarArg; } +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +bool GcInfoDecoder::HasTailCalls() +{ + _ASSERTE( m_Flags & DECODE_HAS_TAILCALLS ); + return m_HasTailCalls; +} +#endif // _TARGET_ARM_ || _TARGET_ARM64_ + bool GcInfoDecoder::WantsReportOnlyLeaf() { + // Only AMD64 with JIT64 can return false here. +#ifdef _TARGET_AMD64_ return m_WantsReportOnlyLeaf; +#else + return true; +#endif } UINT32 GcInfoDecoder::GetCodeLength() diff --git a/src/vm/stackwalk.cpp b/src/vm/stackwalk.cpp index 315cfb10ce2a..8dd1d2824003 100644 --- a/src/vm/stackwalk.cpp +++ b/src/vm/stackwalk.cpp @@ -353,6 +353,7 @@ bool CrawlFrame::IsGcSafe() return GetCodeManager()->IsGcSafe(&codeInfo, GetRelOffset()); } +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) bool CrawlFrame::HasTailCalls() { CONTRACTL { @@ -363,6 +364,7 @@ bool CrawlFrame::HasTailCalls() return GetCodeManager()->HasTailCalls(&codeInfo); } +#endif // _TARGET_ARM_ || _TARGET_ARM64_ inline void CrawlFrame::GotoNextFrame() { diff --git a/src/vm/stackwalk.h b/src/vm/stackwalk.h index db99bb2d8706..e587aa6b685f 100644 --- a/src/vm/stackwalk.h +++ b/src/vm/stackwalk.h @@ -310,7 +310,9 @@ class CrawlFrame */ bool IsGcSafe(); +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) bool HasTailCalls(); +#endif // _TARGET_ARM_ || _TARGET_ARM64_ PREGDISPLAY GetRegisterSet() { diff --git a/src/vm/threadsuspend.cpp b/src/vm/threadsuspend.cpp index 1dc2489e2f36..e3dc093b28a2 100644 --- a/src/vm/threadsuspend.cpp +++ b/src/vm/threadsuspend.cpp @@ -6109,7 +6109,9 @@ struct ExecutionState IJitManager *m_pJitManager; METHODTOKEN m_MethodToken; BOOL m_IsInterruptible; // is this code interruptible? +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) BOOL m_HasTailCalls; // does this code perform tail calls? +#endif // _TARGET_ARM_ || _TARGET_ARM64_ ExecutionState() : m_FirstPass(TRUE) {LIMITED_METHOD_CONTRACT; } }; @@ -6230,7 +6232,9 @@ StackWalkAction SWCB_GetExecutionState(CrawlFrame *pCF, VOID *pData) pES->m_IsInterruptible = pCF->IsGcSafe(); pES->m_RelOffset = pCF->GetRelOffset(); pES->m_pJitManager = pCF->GetJitManager(); +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) pES->m_HasTailCalls = pCF->HasTailCalls(); +#endif // _TARGET_ARM_ || _TARGET_ARM64_ STRESS_LOG3(LF_SYNC, LL_INFO1000, "Stopped in Jitted code at pc = %p sp = %p fullyInt=%d\n", GetControlPC(pCF->GetRegisterSet()), GetRegdisplaySP(pCF->GetRegisterSet()), pES->m_IsInterruptible); From 176abd4418f8d70aea9e6a2ae0bf98c1091443d6 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 29 Jan 2018 15:26:53 +0100 Subject: [PATCH 3/7] Fix formatting issues and reflect PR feedback --- src/jit/codegencommon.cpp | 2 +- src/jit/codegeninterface.h | 4 ++-- src/jit/compiler.h | 2 +- src/jit/flowgraph.cpp | 11 ++++++----- src/jit/gcencode.cpp | 2 +- src/vm/eetwain.cpp | 2 +- src/vm/stackwalk.cpp | 2 +- src/vm/stackwalk.h | 2 +- src/vm/threadsuspend.cpp | 8 +------- 9 files changed, 15 insertions(+), 20 deletions(-) diff --git a/src/jit/codegencommon.cpp b/src/jit/codegencommon.cpp index 0d2b809bc595..a80140427de5 100644 --- a/src/jit/codegencommon.cpp +++ b/src/jit/codegencommon.cpp @@ -182,7 +182,7 @@ CodeGen::CodeGen(Compiler* theCompiler) : CodeGenInterface(theCompiler) genInterruptible = false; #if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) hasTailCalls = false; -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #ifdef DEBUG genInterruptibleUsed = false; genCurDispOffset = (unsigned)-1; diff --git a/src/jit/codegeninterface.h b/src/jit/codegeninterface.h index f07729d2df93..60e713b52846 100644 --- a/src/jit/codegeninterface.h +++ b/src/jit/codegeninterface.h @@ -418,13 +418,13 @@ class CodeGenInterface { m_cgHasTailCalls = value; } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ private: bool m_cgInterruptible; #if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) bool m_cgHasTailCalls; -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ // The following will be set to true if we've determined that we need to // generate a full-blown pointer register map for the current method. diff --git a/src/jit/compiler.h b/src/jit/compiler.h index ebc85f233006..9fb9c103e635 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -7158,7 +7158,7 @@ class Compiler { codeGen->setHasTailCalls(value); } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #if DOUBLE_ALIGN const bool genDoubleAlign() diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index 6e405509f49f..cbd8ee32f7ed 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -18971,14 +18971,15 @@ void Compiler::fgSetBlockOrder() } #if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) - GenTreePtr tailCall = nullptr; - bool fastTailCallsOnly = true; - bool tailCallsConvertibleToLoopOnly = false; - if (!hasTailCalls && block->endsWithTailCall(this, fastTailCallsOnly, tailCallsConvertibleToLoopOnly, &tailCall)) + GenTreePtr tailCall = nullptr; + bool fastTailCallsOnly = true; + bool tailCallsConvertibleToLoopOnly = false; + if (!hasTailCalls && + block->endsWithTailCall(this, fastTailCallsOnly, tailCallsConvertibleToLoopOnly, &tailCall)) { hasTailCalls = true; } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #endif // !JIT32_GCENCODER #endif // FEATURE_FASTTAILCALL diff --git a/src/jit/gcencode.cpp b/src/jit/gcencode.cpp index af007631fda6..f1caf157aa8d 100644 --- a/src/jit/gcencode.cpp +++ b/src/jit/gcencode.cpp @@ -4076,7 +4076,7 @@ void GCInfo::gcInfoBlockHdrSave(GcInfoEncoder* gcInfoEncoder, unsigned methodSiz { gcInfoEncoderWithLog->SetHasTailCalls(); } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #if FEATURE_FIXED_OUT_ARGS // outgoing stack area size diff --git a/src/vm/eetwain.cpp b/src/vm/eetwain.cpp index bed8a9392686..ed76facde3ab 100644 --- a/src/vm/eetwain.cpp +++ b/src/vm/eetwain.cpp @@ -1469,7 +1469,7 @@ bool EECodeManager::HasTailCalls( EECodeInfo *pCodeInfo) return gcInfoDecoder.HasTailCalls(); } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ #if defined(_TARGET_AMD64_) && defined(_DEBUG) diff --git a/src/vm/stackwalk.cpp b/src/vm/stackwalk.cpp index 8dd1d2824003..d324e411d7dd 100644 --- a/src/vm/stackwalk.cpp +++ b/src/vm/stackwalk.cpp @@ -364,7 +364,7 @@ bool CrawlFrame::HasTailCalls() return GetCodeManager()->HasTailCalls(&codeInfo); } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ inline void CrawlFrame::GotoNextFrame() { diff --git a/src/vm/stackwalk.h b/src/vm/stackwalk.h index e587aa6b685f..a30f51407d4f 100644 --- a/src/vm/stackwalk.h +++ b/src/vm/stackwalk.h @@ -312,7 +312,7 @@ class CrawlFrame #if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) bool HasTailCalls(); -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARM_ || _TARGET_ARM64_ PREGDISPLAY GetRegisterSet() { diff --git a/src/vm/threadsuspend.cpp b/src/vm/threadsuspend.cpp index e3dc093b28a2..e5fc68abab94 100644 --- a/src/vm/threadsuspend.cpp +++ b/src/vm/threadsuspend.cpp @@ -6109,9 +6109,6 @@ struct ExecutionState IJitManager *m_pJitManager; METHODTOKEN m_MethodToken; BOOL m_IsInterruptible; // is this code interruptible? -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) - BOOL m_HasTailCalls; // does this code perform tail calls? -#endif // _TARGET_ARM_ || _TARGET_ARM64_ ExecutionState() : m_FirstPass(TRUE) {LIMITED_METHOD_CONTRACT; } }; @@ -6232,9 +6229,6 @@ StackWalkAction SWCB_GetExecutionState(CrawlFrame *pCF, VOID *pData) pES->m_IsInterruptible = pCF->IsGcSafe(); pES->m_RelOffset = pCF->GetRelOffset(); pES->m_pJitManager = pCF->GetJitManager(); -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) - pES->m_HasTailCalls = pCF->HasTailCalls(); -#endif // _TARGET_ARM_ || _TARGET_ARM64_ STRESS_LOG3(LF_SYNC, LL_INFO1000, "Stopped in Jitted code at pc = %p sp = %p fullyInt=%d\n", GetControlPC(pCF->GetRegisterSet()), GetRegdisplaySP(pCF->GetRegisterSet()), pES->m_IsInterruptible); @@ -6310,7 +6304,7 @@ StackWalkAction SWCB_GetExecutionState(CrawlFrame *pCF, VOID *pData) // For this, we may need JIT support to do so. notJittedCase = true; } - else if (pES->m_HasTailCalls) + else if (pCF->HasTailCalls()) { // Do not hijack functions that have tail calls, since there are two problems: // 1. When a function that tail calls another one is hijacked, the LR may be From 9fe28ccea2e32b80daead840ef56ddff87e279ff Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 29 Jan 2018 16:52:54 +0100 Subject: [PATCH 4/7] Reflect more PR feedback --- src/gcdump/gcdumpnonx86.cpp | 2 +- src/inc/gcinfodecoder.h | 4 ++-- src/jit/codegeninterface.h | 2 +- src/vm/gcinfodecoder.cpp | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/gcdump/gcdumpnonx86.cpp b/src/gcdump/gcdumpnonx86.cpp index 09f7ffdbe380..7a7ebfe36be1 100644 --- a/src/gcdump/gcdumpnonx86.cpp +++ b/src/gcdump/gcdumpnonx86.cpp @@ -435,7 +435,7 @@ size_t GCDump::DumpGCTable(PTR_CBYTE gcInfoBlock, #ifdef _TARGET_AMD64_ gcPrintf("Wants Report Only Leaf: %u\n", hdrdecoder.WantsReportOnlyLeaf()); -#else // _TARGET_AMD64_ +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) gcPrintf("Has tailcalls: %u\n", hdrdecoder.HasTailCalls()); #endif // _TARGET_AMD64_ #ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA diff --git a/src/inc/gcinfodecoder.h b/src/inc/gcinfodecoder.h index 750563d2ce82..ad693e1414f3 100644 --- a/src/inc/gcinfodecoder.h +++ b/src/inc/gcinfodecoder.h @@ -222,7 +222,7 @@ enum GcInfoHeaderFlags GC_INFO_HAS_STACK_BASE_REGISTER = 0x40, #ifdef _TARGET_AMD64_ GC_INFO_WANTS_REPORT_ONLY_LEAF = 0x80, -#else // _TARGET_AMD64_ +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) GC_INFO_HAS_TAILCALLS = 0x80, #endif // _TARGET_AMD64_ GC_INFO_HAS_EDIT_AND_CONTINUE_PRESERVED_SLOTS = 0x100, @@ -552,7 +552,7 @@ class GcInfoDecoder bool m_GenericSecretParamIsMT; #ifdef _TARGET_AMD64_ bool m_WantsReportOnlyLeaf; -#else // _TARGET_AMD64_ +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) bool m_HasTailCalls; #endif // _TARGET_AMD64_ INT32 m_SecurityObjectStackSlot; diff --git a/src/jit/codegeninterface.h b/src/jit/codegeninterface.h index 60e713b52846..e64af64e62ce 100644 --- a/src/jit/codegeninterface.h +++ b/src/jit/codegeninterface.h @@ -417,7 +417,7 @@ class CodeGenInterface void setHasTailCalls(bool value) { m_cgHasTailCalls = value; - } + } #endif // _TARGET_ARM_ || _TARGET_ARM64_ private: diff --git a/src/vm/gcinfodecoder.cpp b/src/vm/gcinfodecoder.cpp index ab393d190c7b..ee3a19409d07 100644 --- a/src/vm/gcinfodecoder.cpp +++ b/src/vm/gcinfodecoder.cpp @@ -127,7 +127,7 @@ GcInfoDecoder::GcInfoDecoder( int hasStackBaseRegister = headerFlags & GC_INFO_HAS_STACK_BASE_REGISTER; #ifdef _TARGET_AMD64_ m_WantsReportOnlyLeaf = ((headerFlags & GC_INFO_WANTS_REPORT_ONLY_LEAF) != 0); -#else // _TARGET_AMD64_ +#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) m_HasTailCalls = ((headerFlags & GC_INFO_HAS_TAILCALLS) != 0); #endif // _TARGET_AMD64_ int hasSizeOfEditAndContinuePreservedArea = headerFlags & GC_INFO_HAS_EDIT_AND_CONTINUE_PRESERVED_SLOTS; From 77f5aba16e5603ea04417af10bd17a00c4b24e43 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 1 Feb 2018 14:54:18 +0100 Subject: [PATCH 5/7] Move the hasTailCall detection as per PR feedback Move the hasTailCall detection into the CodeGen::genFnEpilog and also let it cover the JMP IL instruction case, since its behavior is equivalent w.r.t. the hijacking. --- src/jit/codegencommon.cpp | 4 ++++ src/jit/flowgraph.cpp | 12 ------------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/jit/codegencommon.cpp b/src/jit/codegencommon.cpp index a80140427de5..6b8383da9bb9 100644 --- a/src/jit/codegencommon.cpp +++ b/src/jit/codegencommon.cpp @@ -9671,6 +9671,10 @@ void CodeGen::genFnEpilog(BasicBlock* block) if (jmpEpilog) { +#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) + hasTailCalls = true; +#endif // _TARGET_ARM_ || _TARGET_ARM64_ + noway_assert(block->bbJumpKind == BBJ_RETURN); noway_assert(block->bbTreeList != nullptr); diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index cbd8ee32f7ed..4a5e03279a24 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -18969,18 +18969,6 @@ void Compiler::fgSetBlockOrder() // JIT64 does. genInterruptible = true; } - -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) - GenTreePtr tailCall = nullptr; - bool fastTailCallsOnly = true; - bool tailCallsConvertibleToLoopOnly = false; - if (!hasTailCalls && - block->endsWithTailCall(this, fastTailCallsOnly, tailCallsConvertibleToLoopOnly, &tailCall)) - { - hasTailCalls = true; - } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ - #endif // !JIT32_GCENCODER #endif // FEATURE_FASTTAILCALL From 8b91fcb45fb8e2ea54d9880f753d0c75fc490144 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 1 Feb 2018 15:05:41 +0100 Subject: [PATCH 6/7] Use _TARGET_ARMARCH_ in JIT Instead of defined(_TARGET_ARM_) || defined(_TARGET_ARM64_), use defined(_TARGET_ARMARCH_). --- src/jit/codegencommon.cpp | 8 ++++---- src/jit/codegeninterface.h | 8 ++++---- src/jit/compiler.h | 4 ++-- src/jit/gcencode.cpp | 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/jit/codegencommon.cpp b/src/jit/codegencommon.cpp index 6b8383da9bb9..184579a48a8f 100644 --- a/src/jit/codegencommon.cpp +++ b/src/jit/codegencommon.cpp @@ -180,9 +180,9 @@ CodeGen::CodeGen(Compiler* theCompiler) : CodeGenInterface(theCompiler) /* Assume that we not fully interruptible */ genInterruptible = false; -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +#ifdef _TARGET_ARMARCH_ hasTailCalls = false; -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARMARCH_ #ifdef DEBUG genInterruptibleUsed = false; genCurDispOffset = (unsigned)-1; @@ -9671,9 +9671,9 @@ void CodeGen::genFnEpilog(BasicBlock* block) if (jmpEpilog) { -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +#ifdef _TARGET_ARMARCH_ hasTailCalls = true; -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARMARCH_ noway_assert(block->bbJumpKind == BBJ_RETURN); noway_assert(block->bbTreeList != nullptr); diff --git a/src/jit/codegeninterface.h b/src/jit/codegeninterface.h index e64af64e62ce..9a0261f19eba 100644 --- a/src/jit/codegeninterface.h +++ b/src/jit/codegeninterface.h @@ -408,7 +408,7 @@ class CodeGenInterface m_cgInterruptible = value; } -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +#ifdef _TARGET_ARMARCH_ __declspec(property(get = getHasTailCalls, put = setHasTailCalls)) bool hasTailCalls; bool getHasTailCalls() { @@ -418,13 +418,13 @@ class CodeGenInterface { m_cgHasTailCalls = value; } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARMARCH_ private: bool m_cgInterruptible; -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +#ifdef _TARGET_ARMARCH_ bool m_cgHasTailCalls; -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARMARCH_ // The following will be set to true if we've determined that we need to // generate a full-blown pointer register map for the current method. diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 9fb9c103e635..389773bd231a 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -7148,7 +7148,7 @@ class Compiler codeGen->setInterruptible(value); } -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +#ifdef _TARGET_ARMARCH_ __declspec(property(get = getHasTailCalls, put = setHasTailCalls)) bool hasTailCalls; bool getHasTailCalls() { @@ -7158,7 +7158,7 @@ class Compiler { codeGen->setHasTailCalls(value); } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARMARCH_ #if DOUBLE_ALIGN const bool genDoubleAlign() diff --git a/src/jit/gcencode.cpp b/src/jit/gcencode.cpp index f1caf157aa8d..8da52118acd2 100644 --- a/src/jit/gcencode.cpp +++ b/src/jit/gcencode.cpp @@ -3917,7 +3917,7 @@ class GcInfoEncoderWithLogging printf("Set WantsReportOnlyLeaf.\n"); } } -#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +#elif defined(_TARGET_ARMARCH_) void SetHasTailCalls() { m_gcInfoEncoder->SetHasTailCalls(); @@ -4071,12 +4071,12 @@ void GCInfo::gcInfoBlockHdrSave(GcInfoEncoder* gcInfoEncoder, unsigned methodSiz #endif // FEATURE_EH_FUNCLETS -#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) +#ifdef _TARGET_ARMARCH_ if (compiler->codeGen->hasTailCalls) { gcInfoEncoderWithLog->SetHasTailCalls(); } -#endif // _TARGET_ARM_ || _TARGET_ARM64_ +#endif // _TARGET_ARMARCH_ #if FEATURE_FIXED_OUT_ARGS // outgoing stack area size From 91b5c6d003ff6cd2cb3326489dc76d09a9b8d5df Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 13 Feb 2018 21:57:48 +0100 Subject: [PATCH 7/7] Enable the tailcall hijacking test for ARM64 The test JIT/Methodical/tailcall_v4/hijacking should be passing now on ARM64. --- src/inc/gcinfoencoder.h | 2 +- tests/arm64/Tests.lst | 2 +- tests/testsFailingOnArm64.txt | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/inc/gcinfoencoder.h b/src/inc/gcinfoencoder.h index 6b095dca4650..b1236ffeeb4a 100644 --- a/src/inc/gcinfoencoder.h +++ b/src/inc/gcinfoencoder.h @@ -27,7 +27,7 @@ hasPSPSymStackSlot, hasGenericsInstContextStackSlot, hasStackBaseregister, - wantsReportOnlyLeaf (AMD64 JIT64 use only), + wantsReportOnlyLeaf (AMD64 use only), hasTailCalls (ARM/ARM64 only) hasSizeOfEditAndContinuePreservedArea hasReversePInvokeFrame, diff --git a/tests/arm64/Tests.lst b/tests/arm64/Tests.lst index a6ed50bf5e09..2c93e499dab4 100644 --- a/tests/arm64/Tests.lst +++ b/tests/arm64/Tests.lst @@ -58817,7 +58817,7 @@ RelativePath=JIT\Methodical\tailcall_v4\hijacking\hijacking.cmd WorkingDir=JIT\Methodical\tailcall_v4\hijacking Expected=0 MaxAllowedDurationSeconds=600 -Categories=EXPECTED_FAIL;4122;LONG_RUNNING +Categories=EXPECTED_PASS;LONG_RUNNING HostStyle=0 [smallFrame.cmd_7667] diff --git a/tests/testsFailingOnArm64.txt b/tests/testsFailingOnArm64.txt index c124cb60a7fa..3b85ff6b370b 100644 --- a/tests/testsFailingOnArm64.txt +++ b/tests/testsFailingOnArm64.txt @@ -5,4 +5,3 @@ JIT/jit64/opt/cse/HugeField1/HugeField1.sh JIT/jit64/opt/cse/HugeField2/HugeField2.sh CoreMangLib/cti/system/string/StringFormat1/StringFormat1.sh CoreMangLib/cti/system/string/StringFormat2/StringFormat2.sh -JIT/Methodical/tailcall_v4/hijacking/hijacking.sh