Skip to content

Commit c29b30b

Browse files
erozenfelderozenfeld
authored
Fix for 17398. (dotnet#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.
1 parent 6ac136e commit c29b30b

File tree

3 files changed

+156
-11
lines changed

3 files changed

+156
-11
lines changed

‎src/vm/eetwain.cpp

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2404,14 +2404,16 @@ unsigned scanArgRegTable(PTR_CBYTE table,
24042404
Returns size of things pushed on the stack for ESP frames
24052405
24062406
Arguments:
2407-
table - The pointer table
2408-
curOffs - The current code offset
2409-
info - Incoming arg used to determine if there's a frame, and to save results
2407+
table - The pointer table
2408+
curOffsRegs - The current code offset that should be used for reporting registers
2409+
curOffsArgs - The current code offset that should be used for reporting args
2410+
info - Incoming arg used to determine if there's a frame, and to save results
24102411
*/
24112412

24122413
static
2413-
unsigned scanArgRegTableI(PTR_CBYTE table,
2414-
unsigned curOffs,
2414+
unsigned scanArgRegTableI(PTR_CBYTE table,
2415+
unsigned curOffsRegs,
2416+
unsigned curOffsArgs,
24152417
hdrInfo * info)
24162418
{
24172419
CONTRACTL {
@@ -2433,6 +2435,10 @@ unsigned scanArgRegTableI(PTR_CBYTE table,
24332435
bool isThis = false;
24342436
bool iptr = false;
24352437

2438+
// The comment before the call to scanArgRegTableI in EnumGCRefs
2439+
// describes why curOffsRegs can be smaller than curOffsArgs.
2440+
_ASSERTE(curOffsRegs <= curOffsArgs);
2441+
24362442
#if VERIFY_GC_TABLES
24372443
_ASSERTE(*castto(table, unsigned short *)++ == 0xBABE);
24382444
#endif
@@ -2506,7 +2512,7 @@ unsigned scanArgRegTableI(PTR_CBYTE table,
25062512

25072513
/* Have we reached the instruction we're looking for? */
25082514

2509-
while (ptrOffs <= curOffs)
2515+
while (ptrOffs <= curOffsArgs)
25102516
{
25112517
unsigned val;
25122518

@@ -2543,10 +2549,14 @@ unsigned scanArgRegTableI(PTR_CBYTE table,
25432549
regNum reg;
25442550

25452551
ptrOffs += (val ) & 0x7;
2546-
if (ptrOffs > curOffs) {
2552+
if (ptrOffs > curOffsArgs) {
25472553
iptr = isThis = false;
25482554
goto REPORT_REFS;
25492555
}
2556+
else if (ptrOffs > curOffsRegs) {
2557+
iptr = isThis = false;
2558+
continue;
2559+
}
25502560

25512561
reg = (regNum)((val >> 3) & 0x7);
25522562
regMask = 1 << reg; // EAX,ECX,EDX,EBX,---,EBP,ESI,EDI
@@ -2606,7 +2616,7 @@ unsigned scanArgRegTableI(PTR_CBYTE table,
26062616
/* A small argument encoding */
26072617

26082618
ptrOffs += (val & 0x07);
2609-
if (ptrOffs > curOffs) {
2619+
if (ptrOffs > curOffsArgs) {
26102620
iptr = isThis = false;
26112621
goto REPORT_REFS;
26122622
}
@@ -2736,7 +2746,7 @@ unsigned scanArgRegTableI(PTR_CBYTE table,
27362746
/* non-ptr arg push */
27372747
_ASSERTE(!hasPartialArgInfo);
27382748
ptrOffs += (val & 0x07);
2739-
if (ptrOffs > curOffs) {
2749+
if (ptrOffs > curOffsArgs) {
27402750
iptr = isThis = false;
27412751
goto REPORT_REFS;
27422752
}
@@ -2858,6 +2868,7 @@ unsigned GetPushedArgSize(hdrInfo * info, PTR_CBYTE table, DWORD curOffs)
28582868
if (info->interruptible)
28592869
{
28602870
sz = scanArgRegTableI(skipToArgReg(*info, table),
2871+
curOffs,
28612872
curOffs,
28622873
info);
28632874
}
@@ -4436,7 +4447,15 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pContext,
44364447

44374448
if (info.interruptible)
44384449
{
4439-
pushedSize = scanArgRegTableI(skipToArgReg(info, table), curOffs, &info);
4450+
// If we are not on the active stack frame, we need to report gc registers
4451+
// that are live before the call. The reason is that the liveness of gc registers
4452+
// may change across a call to a method that does not return. In this case the instruction
4453+
// after the call may be a jump target and a register that didn't have a live gc pointer
4454+
// before the call may have a live gc pointer after the jump. To make sure we report the
4455+
// registers that have live gc pointers before the call we subtract 1 from curOffs.
4456+
unsigned curOffsRegs = (flags & ActiveStackFrame) != 0 ? curOffs : curOffs - 1;
4457+
4458+
pushedSize = scanArgRegTableI(skipToArgReg(info, table), curOffsRegs, curOffs, &info);
44404459

44414460
RegMask regs = info.regMaskResult;
44424461
RegMask iregs = info.iregMaskResult;
@@ -5342,7 +5361,7 @@ OBJECTREF EECodeManager::GetInstance( PREGDISPLAY pContext,
53425361

53435362
if (info.interruptible)
53445363
{
5345-
stackDepth = scanArgRegTableI(skipToArgReg(info, table), (unsigned)relOffset, &info);
5364+
stackDepth = scanArgRegTableI(skipToArgReg(info, table), relOffset, relOffset, &info);
53465365
}
53475366
else
53485367
{
@@ -6046,6 +6065,7 @@ TADDR EECodeManager::GetAmbientSP(PREGDISPLAY pContext,
60466065
if (stateBuf->hdrInfoBody.interruptible)
60476066
{
60486067
baseSP += scanArgRegTableI(skipToArgReg(stateBuf->hdrInfoBody, table),
6068+
dwRelOffset,
60496069
dwRelOffset,
60506070
&stateBuf->hdrInfoBody);
60516071
}

‎tests/src/Regressions/coreclr/GitHub_17398/test17398.cs

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
using System.Runtime.CompilerServices;
7+
8+
// Repro case for CoreCLR 17398
9+
10+
class X
11+
{
12+
static int v;
13+
14+
string s;
15+
16+
public override string ToString() => s;
17+
18+
[MethodImpl(MethodImplOptions.NoInlining)]
19+
X(int x)
20+
{
21+
s = "String" + x;
22+
}
23+
24+
[MethodImpl(MethodImplOptions.NoInlining)]
25+
static void F() { }
26+
27+
public static void T0(object o, int x)
28+
{
29+
GC.Collect(2);
30+
throw new Exception(o.ToString());
31+
}
32+
33+
[MethodImpl(MethodImplOptions.NoInlining)]
34+
public static void Test()
35+
{
36+
object x1 = new X(1);
37+
object x2 = new X(2);
38+
39+
if (v == 1)
40+
{
41+
// Generate enough pressure here to
42+
// kill ESI so in linear flow it is dead
43+
// at the call to T0
44+
int w = v;
45+
int x = v;
46+
int y = v;
47+
int z = v;
48+
49+
// Unbounded loop here forces fully interruptible GC
50+
for (int i = 0; i < v; i++)
51+
{
52+
w++;
53+
}
54+
55+
T0(x2, w + x + y + z);
56+
}
57+
58+
// Encourage x1 to be in callee save (ESI)
59+
F();
60+
61+
if (v == 2)
62+
{
63+
T0(x1, 0);
64+
}
65+
}
66+
67+
public static int Main()
68+
{
69+
v = 1;
70+
int r = 0;
71+
72+
try
73+
{
74+
Test();
75+
}
76+
catch (Exception e)
77+
{
78+
Console.WriteLine(e.Message);
79+
r = 100;
80+
}
81+
82+
return r;
83+
}
84+
}

‎tests/src/Regressions/coreclr/GitHub_17398/test17398.csproj

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
3+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
4+
<PropertyGroup>
5+
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
6+
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
7+
<SchemaVersion>2.0</SchemaVersion>
8+
<ProjectGuid>{E55A6F8B-B9E3-45CE-88F4-22AE70F606CB}</ProjectGuid>
9+
<OutputType>Exe</OutputType>
10+
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
11+
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
12+
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
13+
<CLRTestKind>BuildAndRun</CLRTestKind>
14+
</PropertyGroup>
15+
<!-- Default configurations to help VS understand the configurations -->
16+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
17+
</PropertyGroup>
18+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
19+
</PropertyGroup>
20+
<ItemGroup>
21+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
22+
<Visible>False</Visible>
23+
</CodeAnalysisDependentAssemblyPaths>
24+
</ItemGroup>
25+
<PropertyGroup>
26+
<DebugType></DebugType>
27+
<Optimize>True</Optimize>
28+
</PropertyGroup>
29+
<ItemGroup>
30+
<Compile Include="test17398.cs" />
31+
</ItemGroup>
32+
<ItemGroup>
33+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
34+
</ItemGroup>
35+
<ItemGroup>
36+
<ProjectReference Include="../../../Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
37+
</ItemGroup>
38+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
39+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
40+
</PropertyGroup>
41+
</Project>

0 commit comments

Comments
 (0)