diff options
author | Eugene Rozenfeld <erozen@microsoft.com> | 2018-04-12 16:05:44 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-04-12 16:05:44 -0700 |
commit | c29b30bc4c7bf976db6fcf2c44e352920ace63aa (patch) | |
tree | 581f76fd62c4a10bd52c9775c4b4f6c53e21f1d5 | |
parent | 6ac136e313826833b7fd32f0c8968297e8d7458e (diff) | |
download | coreclr-c29b30bc4c7bf976db6fcf2c44e352920ace63aa.tar.gz coreclr-c29b30bc4c7bf976db6fcf2c44e352920ace63aa.tar.bz2 coreclr-c29b30bc4c7bf976db6fcf2c44e352920ace63aa.zip |
Fix for 17398. (#17501)
When enumerating live gc registers, if we are not on the active stack frame,
we need to report callee-save gc registers that are live before the call.
The reason is that the liveness of gc registers may change across a call
to a method that does not return. In this case the instruction after the call
may be a jump target and a register that didn't have a live gc pointer before
the call may have a live gc pointer after the jump. To make sure we report the
registers that have live gc pointers before the call we subtract 1 from curOffs.
-rw-r--r-- | src/vm/eetwain.cpp | 42 | ||||
-rw-r--r-- | tests/src/Regressions/coreclr/GitHub_17398/test17398.cs | 84 | ||||
-rw-r--r-- | tests/src/Regressions/coreclr/GitHub_17398/test17398.csproj | 41 |
3 files changed, 156 insertions, 11 deletions
diff --git a/src/vm/eetwain.cpp b/src/vm/eetwain.cpp index 1fb332ecd4..511a635509 100644 --- a/src/vm/eetwain.cpp +++ b/src/vm/eetwain.cpp @@ -2404,14 +2404,16 @@ FINISHED: Returns size of things pushed on the stack for ESP frames Arguments: - table - The pointer table - curOffs - The current code offset - info - Incoming arg used to determine if there's a frame, and to save results + table - The pointer table + curOffsRegs - The current code offset that should be used for reporting registers + curOffsArgs - The current code offset that should be used for reporting args + info - Incoming arg used to determine if there's a frame, and to save results */ static -unsigned scanArgRegTableI(PTR_CBYTE table, - unsigned curOffs, +unsigned scanArgRegTableI(PTR_CBYTE table, + unsigned curOffsRegs, + unsigned curOffsArgs, hdrInfo * info) { CONTRACTL { @@ -2433,6 +2435,10 @@ unsigned scanArgRegTableI(PTR_CBYTE table, bool isThis = false; bool iptr = false; + // The comment before the call to scanArgRegTableI in EnumGCRefs + // describes why curOffsRegs can be smaller than curOffsArgs. + _ASSERTE(curOffsRegs <= curOffsArgs); + #if VERIFY_GC_TABLES _ASSERTE(*castto(table, unsigned short *)++ == 0xBABE); #endif @@ -2506,7 +2512,7 @@ unsigned scanArgRegTableI(PTR_CBYTE table, /* Have we reached the instruction we're looking for? */ - while (ptrOffs <= curOffs) + while (ptrOffs <= curOffsArgs) { unsigned val; @@ -2543,10 +2549,14 @@ unsigned scanArgRegTableI(PTR_CBYTE table, regNum reg; ptrOffs += (val ) & 0x7; - if (ptrOffs > curOffs) { + if (ptrOffs > curOffsArgs) { iptr = isThis = false; goto REPORT_REFS; } + else if (ptrOffs > curOffsRegs) { + iptr = isThis = false; + continue; + } reg = (regNum)((val >> 3) & 0x7); regMask = 1 << reg; // EAX,ECX,EDX,EBX,---,EBP,ESI,EDI @@ -2606,7 +2616,7 @@ unsigned scanArgRegTableI(PTR_CBYTE table, /* A small argument encoding */ ptrOffs += (val & 0x07); - if (ptrOffs > curOffs) { + if (ptrOffs > curOffsArgs) { iptr = isThis = false; goto REPORT_REFS; } @@ -2736,7 +2746,7 @@ unsigned scanArgRegTableI(PTR_CBYTE table, /* non-ptr arg push */ _ASSERTE(!hasPartialArgInfo); ptrOffs += (val & 0x07); - if (ptrOffs > curOffs) { + if (ptrOffs > curOffsArgs) { iptr = isThis = false; goto REPORT_REFS; } @@ -2859,6 +2869,7 @@ unsigned GetPushedArgSize(hdrInfo * info, PTR_CBYTE table, DWORD curOffs) { sz = scanArgRegTableI(skipToArgReg(*info, table), curOffs, + curOffs, info); } else @@ -4436,7 +4447,15 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pContext, if (info.interruptible) { - pushedSize = scanArgRegTableI(skipToArgReg(info, table), curOffs, &info); + // If we are not on the active stack frame, we need to report gc registers + // that are live before the call. The reason is that the liveness of gc registers + // may change across a call to a method that does not return. In this case the instruction + // after the call may be a jump target and a register that didn't have a live gc pointer + // before the call may have a live gc pointer after the jump. To make sure we report the + // registers that have live gc pointers before the call we subtract 1 from curOffs. + unsigned curOffsRegs = (flags & ActiveStackFrame) != 0 ? curOffs : curOffs - 1; + + pushedSize = scanArgRegTableI(skipToArgReg(info, table), curOffsRegs, curOffs, &info); RegMask regs = info.regMaskResult; RegMask iregs = info.iregMaskResult; @@ -5342,7 +5361,7 @@ OBJECTREF EECodeManager::GetInstance( PREGDISPLAY pContext, if (info.interruptible) { - stackDepth = scanArgRegTableI(skipToArgReg(info, table), (unsigned)relOffset, &info); + stackDepth = scanArgRegTableI(skipToArgReg(info, table), relOffset, relOffset, &info); } else { @@ -6047,6 +6066,7 @@ TADDR EECodeManager::GetAmbientSP(PREGDISPLAY pContext, { baseSP += scanArgRegTableI(skipToArgReg(stateBuf->hdrInfoBody, table), dwRelOffset, + dwRelOffset, &stateBuf->hdrInfoBody); } else diff --git a/tests/src/Regressions/coreclr/GitHub_17398/test17398.cs b/tests/src/Regressions/coreclr/GitHub_17398/test17398.cs new file mode 100644 index 0000000000..a9cc4b9980 --- /dev/null +++ b/tests/src/Regressions/coreclr/GitHub_17398/test17398.cs @@ -0,0 +1,84 @@ +// 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.CompilerServices; + +// Repro case for CoreCLR 17398 + +class X +{ + static int v; + + string s; + + public override string ToString() => s; + + [MethodImpl(MethodImplOptions.NoInlining)] + X(int x) + { + s = "String" + x; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void F() { } + + public static void T0(object o, int x) + { + GC.Collect(2); + throw new Exception(o.ToString()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void Test() + { + object x1 = new X(1); + object x2 = new X(2); + + if (v == 1) + { + // Generate enough pressure here to + // kill ESI so in linear flow it is dead + // at the call to T0 + int w = v; + int x = v; + int y = v; + int z = v; + + // Unbounded loop here forces fully interruptible GC + for (int i = 0; i < v; i++) + { + w++; + } + + T0(x2, w + x + y + z); + } + + // Encourage x1 to be in callee save (ESI) + F(); + + if (v == 2) + { + T0(x1, 0); + } + } + + public static int Main() + { + v = 1; + int r = 0; + + try + { + Test(); + } + catch (Exception e) + { + Console.WriteLine(e.Message); + r = 100; + } + + return r; + } +} diff --git a/tests/src/Regressions/coreclr/GitHub_17398/test17398.csproj b/tests/src/Regressions/coreclr/GitHub_17398/test17398.csproj new file mode 100644 index 0000000000..53d0e2ed21 --- /dev/null +++ b/tests/src/Regressions/coreclr/GitHub_17398/test17398.csproj @@ -0,0 +1,41 @@ +<?xml version="1.0" encoding="utf-8"?> +<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> + <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> + <PropertyGroup> + <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration> + <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform> + <SchemaVersion>2.0</SchemaVersion> + <ProjectGuid>{E55A6F8B-B9E3-45CE-88F4-22AE70F606CB}</ProjectGuid> + <OutputType>Exe</OutputType> + <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids> + <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir> + <AllowUnsafeBlocks>true</AllowUnsafeBlocks> + <CLRTestKind>BuildAndRun</CLRTestKind> + </PropertyGroup> + <!-- Default configurations to help VS understand the configurations --> + <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "> + </PropertyGroup> + <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "> + </PropertyGroup> + <ItemGroup> + <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies"> + <Visible>False</Visible> + </CodeAnalysisDependentAssemblyPaths> + </ItemGroup> + <PropertyGroup> + <DebugType></DebugType> + <Optimize>True</Optimize> + </PropertyGroup> + <ItemGroup> + <Compile Include="test17398.cs" /> + </ItemGroup> + <ItemGroup> + <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" /> + </ItemGroup> + <ItemGroup> + <ProjectReference Include="../../../Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" /> + </ItemGroup> + <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" /> + <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "> + </PropertyGroup> +</Project>
\ No newline at end of file |