Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix ARM/ARM64 hijacking in tail calls #16039

Merged
merged 7 commits into from
Feb 14, 2018
Merged

Fix ARM/ARM64 hijacking in tail calls #16039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ca5371b
Fix ARM/ARM64 hijacking in tail calls
janvorli Jan 26, 2018
91642b7
Reflect PR feedback
janvorli Jan 26, 2018
176abd4
Fix formatting issues and reflect PR feedback
janvorli Jan 29, 2018
9fe28cc
Reflect more PR feedback
janvorli Jan 29, 2018
77f5aba
Move the hasTailCall detection as per PR feedback
janvorli Feb 1, 2018
8b91fcb
Use _TARGET_ARMARCH_ in JIT
janvorli Feb 1, 2018
91b5c6d
Enable the tailcall hijacking test for ARM64
janvorli Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/gcdump/gcdumpnonx86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,11 @@ size_t GCDump::DumpGCTable(PTR_CBYTE gcInfoBlock,
? "<none>"
: GetRegName(hdrdecoder.GetStackBaseRegister()));

#ifdef _TARGET_AMD64_
gcPrintf("Wants Report Only Leaf: %u\n", hdrdecoder.WantsReportOnlyLeaf());

#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
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
Expand Down
4 changes: 4 additions & 0 deletions src/gcinfo/gcinfodumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,10 @@ PORTABILITY_ASSERT("GcInfoDumper::EnumerateStateChanges is not implemented on th
(GcInfoDecoderFlags)( DECODE_SECURITY_OBJECT
| DECODE_CODE_LENGTH
| DECODE_VARARG
#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
| DECODE_HAS_TAILCALLS
#endif // _TARGET_ARM_ || _TARGET_ARM64_

| DECODE_INTERRUPTIBILITY),
offset);

Expand Down
22 changes: 21 additions & 1 deletion src/gcinfo/gcinfoencoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +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;

Expand Down Expand Up @@ -752,10 +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 )
Expand Down Expand Up @@ -1010,9 +1021,14 @@ 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) &&
#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.
Expand All @@ -1034,7 +1050,11 @@ 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);

Expand Down
9 changes: 9 additions & 0 deletions src/inc/eetwain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,10 @@ virtual bool UnwindStackFrame(PREGDISPLAY pContext,
virtual bool IsGcSafe(EECodeInfo *pCodeInfo,
DWORD dwRelOffset) = 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)
/*
Locates the end of the last interruptible region in the given code range.
Expand Down Expand Up @@ -470,6 +474,11 @@ virtual
bool IsGcSafe( EECodeInfo *pCodeInfo,
DWORD dwRelOffset);

#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
virtual
bool HasTailCalls(EECodeInfo *pCodeInfo);
#endif // _TARGET_ARM_ || _TARGET_ARM64_

#if defined(_TARGET_AMD64_) && defined(_DEBUG)
/*
Locates the end of the last interruptible region in the given code range.
Expand Down
16 changes: 15 additions & 1 deletion src/inc/gcinfodecoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ 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,
#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
DECODE_HAS_TAILCALLS = 0x4000,
#endif // _TARGET_ARM_ || _TARGET_ARM64_
};

enum GcInfoHeaderFlags
Expand All @@ -217,7 +220,11 @@ 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,
#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
GC_INFO_HAS_TAILCALLS = 0x80,
#endif // _TARGET_AMD64_
GC_INFO_HAS_EDIT_AND_CONTINUE_PRESERVED_SLOTS = 0x100,
GC_INFO_REVERSE_PINVOKE_FRAME = 0x200,

Expand Down Expand Up @@ -520,6 +527,9 @@ class GcInfoDecoder
bool HasMethodTableGenericsInstContext();
bool GetIsVarArg();
bool WantsReportOnlyLeaf();
#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should put the bool WantsReportOnlyLeaf(); above this also under #ifdef

bool HasTailCalls();
#endif // _TARGET_ARM_ || _TARGET_ARM64_
ReturnKind GetReturnKind();
UINT32 GetCodeLength();
UINT32 GetStackBaseRegister();
Expand All @@ -540,7 +550,11 @@ class GcInfoDecoder
bool m_IsVarArg;
bool m_GenericSecretParamIsMD;
bool m_GenericSecretParamIsMT;
#ifdef _TARGET_AMD64_
bool m_WantsReportOnlyLeaf;
#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
bool m_HasTailCalls;
#endif // _TARGET_AMD64_
INT32 m_SecurityObjectStackSlot;
INT32 m_GSCookieStackSlot;
INT32 m_ReversePInvokeFrameStackSlot;
Expand Down
11 changes: 10 additions & 1 deletion src/inc/gcinfoencoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@
hasPSPSymStackSlot,
hasGenericsInstContextStackSlot,
hasStackBaseregister,
wantsReportOnlyLeaf,
wantsReportOnlyLeaf (AMD64 use only),
hasTailCalls (ARM/ARM64 only)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comma is missed.

hasSizeOfEditAndContinuePreservedArea
hasReversePInvokeFrame,
- ReturnKind (Fat: 4 bits)
Expand Down Expand Up @@ -436,10 +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 );
Expand Down Expand Up @@ -491,7 +496,11 @@ class GcInfoEncoder
GcInfoArrayList<LifetimeTransition, 64> m_LifetimeTransitions;

bool m_IsVarArg;
#if defined(_TARGET_AMD64_)
bool m_WantsReportOnlyLeaf;
#elif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
bool m_HasTailCalls;
#endif // _TARGET_AMD64_
Copy link

@sandreenko sandreenko Feb 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong comment for #endif, it should be #endif defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) according to https://github.com/dotnet/coreclr/blob/master/Documentation/coding-guidelines/clr-jit-coding-conventions.md#75-commenting-ifdefs, but it is not jit sources, so maybe we do not care.

and defined(_TARGET_ARM_) || defined(_TARGET_ARM64_) == _TARGET_ARMARCH_

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the coding convention doc actually doesn't cover the #elif pattern and in coreclr runtime, we use the pattern I've used (the comment on #endif matching the #if and not the #elif comment)

INT32 m_SecurityObjectStackSlot;
INT32 m_GSCookieStackSlot;
UINT32 m_GSCookieValidRangeStart;
Expand Down
7 changes: 7 additions & 0 deletions src/jit/codegencommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ CodeGen::CodeGen(Compiler* theCompiler) : CodeGenInterface(theCompiler)
/* Assume that we not fully interruptible */

genInterruptible = false;
#ifdef _TARGET_ARMARCH_
hasTailCalls = false;
#endif // _TARGET_ARMARCH_
#ifdef DEBUG
genInterruptibleUsed = false;
genCurDispOffset = (unsigned)-1;
Expand Down Expand Up @@ -9668,6 +9671,10 @@ void CodeGen::genFnEpilog(BasicBlock* block)

if (jmpEpilog)
{
#ifdef _TARGET_ARMARCH_
hasTailCalls = true;
#endif // _TARGET_ARMARCH_

noway_assert(block->bbJumpKind == BBJ_RETURN);
noway_assert(block->bbTreeList != nullptr);

Expand Down
15 changes: 15 additions & 0 deletions src/jit/codegeninterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,23 @@ class CodeGenInterface
m_cgInterruptible = value;
}

#ifdef _TARGET_ARMARCH_
__declspec(property(get = getHasTailCalls, put = setHasTailCalls)) bool hasTailCalls;
bool getHasTailCalls()
{
return m_cgHasTailCalls;
}
void setHasTailCalls(bool value)
{
m_cgHasTailCalls = value;
}
#endif // _TARGET_ARMARCH_

private:
bool m_cgInterruptible;
#ifdef _TARGET_ARMARCH_
bool m_cgHasTailCalls;
#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.
Expand Down
12 changes: 12 additions & 0 deletions src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7148,6 +7148,18 @@ class Compiler
codeGen->setInterruptible(value);
}

#ifdef _TARGET_ARMARCH_
__declspec(property(get = getHasTailCalls, put = setHasTailCalls)) bool hasTailCalls;
bool getHasTailCalls()
{
return codeGen->hasTailCalls;
}
void setHasTailCalls(bool value)
{
codeGen->setHasTailCalls(value);
}
#endif // _TARGET_ARMARCH_

#if DOUBLE_ALIGN
const bool genDoubleAlign()
{
Expand Down
21 changes: 21 additions & 0 deletions src/jit/gcencode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3908,6 +3908,7 @@ class GcInfoEncoderWithLogging
}
}

#ifdef _TARGET_AMD64_
void SetWantsReportOnlyLeaf()
{
m_gcInfoEncoder->SetWantsReportOnlyLeaf();
Expand All @@ -3916,6 +3917,16 @@ class GcInfoEncoderWithLogging
printf("Set WantsReportOnlyLeaf.\n");
}
}
#elif defined(_TARGET_ARMARCH_)
void SetHasTailCalls()
{
m_gcInfoEncoder->SetHasTailCalls();
if (m_doLogging)
{
printf("Set HasTailCalls.\n");
}
}
#endif // _TARGET_AMD64_

void SetSizeOfStackOutgoingAndScratchArea(UINT32 size)
{
Expand Down Expand Up @@ -4050,13 +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

#ifdef _TARGET_ARMARCH_
if (compiler->codeGen->hasTailCalls)
{
gcInfoEncoderWithLog->SetHasTailCalls();
}
#endif // _TARGET_ARMARCH_

#if FEATURE_FIXED_OUT_ARGS
// outgoing stack area size
gcInfoEncoderWithLog->SetSizeOfStackOutgoingAndScratchArea(compiler->lvaOutgoingArgSpaceSize);
Expand Down
2 changes: 1 addition & 1 deletion src/jit/legacynonjit/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ set_property(TARGET legacynonjit APPEND_STRING PROPERTY LINK_DEPENDS ${JIT_EXPOR

set(RYUJIT_LINK_LIBRARIES
utilcodestaticnohost
gcinfo
gcinfo_arm
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BruceForstall this is a bug in the legacynonjit CMakeLists.txt. I've hit it when making my changes that are ARM / ARM64 specific and was getting missing symbols due to the fact that x86 version of the gcinfo was being linked in.

)

if(CLR_CMAKE_PLATFORM_UNIX)
Expand Down
19 changes: 19 additions & 0 deletions src/vm/eetwain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1451,6 +1451,25 @@ bool EECodeManager::IsGcSafe( EECodeInfo *pCodeInfo,
return gcInfoDecoder.IsInterruptible();
}

#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
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();
}
#endif // _TARGET_ARM_ || _TARGET_ARM64_

#if defined(_TARGET_AMD64_) && defined(_DEBUG)

Expand Down
17 changes: 17 additions & 0 deletions src/vm/gcinfodecoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +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);
#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;

int hasReversePInvokeFrame = false;
Expand Down Expand Up @@ -527,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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you just want to put the whole function under #ifdef _TARGET_AMD64_?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have intentionally kept it on the GCInfoDecoder and just always return true for non-amd64 platforms. The places in the code that call this method would need to have ifdefs around parts of conditions otherwise and it seemed less readable.

#ifdef _TARGET_AMD64_
return m_WantsReportOnlyLeaf;
#else
return true;
#endif
}

UINT32 GcInfoDecoder::GetCodeLength()
Expand Down
13 changes: 13 additions & 0 deletions src/vm/stackwalk.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,19 @@ bool CrawlFrame::IsGcSafe()
return GetCodeManager()->IsGcSafe(&codeInfo, GetRelOffset());
}

#if defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
bool CrawlFrame::HasTailCalls()
{
CONTRACTL {
NOTHROW;
GC_NOTRIGGER;
SUPPORTS_DAC;
} CONTRACTL_END;

return GetCodeManager()->HasTailCalls(&codeInfo);
}
#endif // _TARGET_ARM_ || _TARGET_ARM64_

inline void CrawlFrame::GotoNextFrame()
{
CONTRACTL {
Expand Down
Loading