Skip to content

Commit 3e2a45c

Browse files
jakobbotschjakobbotsch
authored
Fix too narrow loads while unspilling (#66675)
Unspilling could produce too narrow loads for normalize-on-load variables when encountering a narrowly typed LCL_VAR node. This could result in subsequent uses of the same local using a truncated value. Fix #66624
1 parent d2fa88e commit 3e2a45c

File tree

4 files changed

+140
-15
lines changed

4 files changed

+140
-15
lines changed

‎src/coreclr/jit/codegenlinear.cpp

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1213,22 +1213,15 @@ void CodeGen::genUnspillRegIfNeeded(GenTree* tree)
12131213
// TODO-Cleanup: The following code could probably be further merged and cleaned up.
12141214
#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
12151215
// Load local variable from its home location.
1216-
// In most cases the tree type will indicate the correct type to use for the load.
1217-
// However, if it is NOT a normalizeOnLoad lclVar (i.e. NOT a small int that always gets
1218-
// widened when loaded into a register), and its size is not the same as the actual register type
1219-
// of the lclVar, then we need to change the type of the tree node when loading.
1220-
// This situation happens due to "optimizations" that avoid a cast and
1221-
// simply retype the node when using long type lclVar as an int.
1222-
// While loading the int in that case would work for this use of the lclVar, if it is
1223-
// later used as a long, we will have incorrectly truncated the long.
1224-
// In the normalizeOnLoad case ins_Load will return an appropriate sign- or zero-
1225-
// extending load.
1226-
var_types lclActualType = varDsc->GetActualRegisterType();
1227-
assert(lclActualType != TYP_UNDEF);
1228-
if (spillType != lclActualType && !varTypeIsGC(spillType) && !varDsc->lvNormalizeOnLoad())
1216+
// Never allow truncating the locals here, otherwise a subsequent
1217+
// use of the local with a wider type would see the truncated
1218+
// value. We do allow wider loads as those can be efficient even
1219+
// when unaligned and might be smaller encoding wise (on xarch).
1220+
var_types lclLoadType = varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : varDsc->GetActualRegisterType();
1221+
assert(lclLoadType != TYP_UNDEF);
1222+
if (genTypeSize(spillType) < genTypeSize(lclLoadType))
12291223
{
1230-
assert(!varTypeIsGC(varDsc));
1231-
spillType = lclActualType;
1224+
spillType = lclLoadType;
12321225
}
12331226
#elif defined(TARGET_ARM)
12341227
// No normalizing for ARM

‎src/tests/JIT/Regression/JitBlue/Runtime_66624/Runtime_66624.cs

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
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+
4+
// Generated by Fuzzlyn v1.5 on 2022-03-14 21:20:55
5+
// Run on X64 Windows
6+
// Seed: 16520696696442011600
7+
// Reduced from 677.9 KiB to 2.4 KiB in 00:04:01
8+
// Debug: Outputs 53474
9+
// Release: Outputs 226
10+
public class C0
11+
{
12+
public byte F0;
13+
public sbyte F3;
14+
public byte F4;
15+
public byte F5;
16+
public uint F6;
17+
public C0(byte f4, byte f5, uint f6)
18+
{
19+
F4 = f4;
20+
F5 = f5;
21+
F6 = f6;
22+
}
23+
}
24+
25+
public class C1
26+
{
27+
public C0 F4;
28+
public C1(C0 f4)
29+
{
30+
F4 = f4;
31+
}
32+
}
33+
34+
public struct S0
35+
{
36+
public sbyte F0;
37+
public C0 F2;
38+
public bool F4;
39+
public ulong F5;
40+
public uint F6;
41+
public bool F7;
42+
public S0(sbyte f0, C0 f1, C0 f2, bool f4, ulong f5, uint f6, bool f7) : this()
43+
{
44+
F0 = f0;
45+
F2 = f2;
46+
F4 = f4;
47+
F5 = f5;
48+
F6 = f6;
49+
F7 = f7;
50+
}
51+
}
52+
53+
public class Runtime_66624
54+
{
55+
public static IRuntime s_rt;
56+
public static C0 s_1 = new C0(0, 0, 0);
57+
public static C1[][] s_3 = new C1[][] { new C1[] { new C1(new C0(0, 0, 0)) } };
58+
public static int Main()
59+
{
60+
CollectibleALC alc = new CollectibleALC();
61+
System.Reflection.Assembly asm = alc.LoadFromAssemblyPath(System.Reflection.Assembly.GetExecutingAssembly().Location);
62+
System.Reflection.MethodInfo mi = asm.GetType(typeof(Runtime_66624).FullName).GetMethod(nameof(MainInner));
63+
System.Type runtimeTy = asm.GetType(typeof(Runtime).FullName);
64+
return (int)mi.Invoke(null, new object[] { System.Activator.CreateInstance(runtimeTy) });
65+
}
66+
67+
public static int MainInner(IRuntime rt)
68+
{
69+
s_rt = rt;
70+
return M10(53474);
71+
}
72+
73+
public static int M10(ushort arg0)
74+
{
75+
var vr1 = new S0[] { new S0(0, new C0(0, 0, 0), new C0(0, 0, 0), false, 0, 0, false) };
76+
S0 var1 = new S0(s_1.F3, new C0(0, 0, s_3[0][0].F4.F6), new C0(0, 0, 0), true, 0, 0, false);
77+
try
78+
{
79+
byte vr5 = var1.F2.F0;
80+
}
81+
finally
82+
{
83+
var1 = new S0(0, new C0(0, 0, 0), new C0(0, 1, 0), true, 0, 0, true);
84+
}
85+
86+
if ((byte)arg0 < 1)
87+
{
88+
C0[] var4 = new C0[] { new C0(0, 0, 0) };
89+
}
90+
91+
s_rt.WriteLine("c_4102", arg0);
92+
return ((Runtime)s_rt).Result;
93+
}
94+
}
95+
96+
public interface IRuntime
97+
{
98+
void WriteLine<T>(string site, T value);
99+
}
100+
101+
public class Runtime : IRuntime
102+
{
103+
public int Result;
104+
105+
public void WriteLine<T>(string site, T value)
106+
{
107+
Result = (ushort)(object)value == 53474 ? 100 : -1;
108+
}
109+
}
110+
111+
public class CollectibleALC : System.Runtime.Loader.AssemblyLoadContext
112+
{
113+
public CollectibleALC() : base(true)
114+
{
115+
}
116+
}

‎src/tests/JIT/Regression/JitBlue/Runtime_66624/Runtime_66624.csproj

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<OutputType>Exe</OutputType>
4+
<Optimize>True</Optimize>
5+
<CLRTestPriority>1</CLRTestPriority>
6+
</PropertyGroup>
7+
<ItemGroup>
8+
<Compile Include="$(MSBuildProjectName).cs" />
9+
</ItemGroup>
10+
</Project>

‎src/tests/issues.targets

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1265,6 +1265,9 @@
12651265
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_64883/Runtime_64883/*">
12661266
<Issue>https://github.com/dotnet/runtimelab/issues/155: Collectible assemblies</Issue>
12671267
</ExcludeList>
1268+
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_66624/Runtime_66624/*">
1269+
<Issue>https://github.com/dotnet/runtimelab/issues/155: Collectible assemblies</Issue>
1270+
</ExcludeList>
12681271
<ExcludeList Include="$(XunitTestBinBase)/Loader/ContextualReflection/ContextualReflection/*">
12691272
<Issue>https://github.com/dotnet/runtimelab/issues/165</Issue>
12701273
</ExcludeList>
@@ -3431,6 +3434,9 @@
34313434
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_64883/Runtime_64883/*">
34323435
<Issue>Loads an assembly from file</Issue>
34333436
</ExcludeList>
3437+
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_66624/Runtime_66624/*">
3438+
<Issue>Loads an assembly from file</Issue>
3439+
</ExcludeList>
34343440
<ExcludeList Include = "$(XunitTestBinBase)/tracing/eventcounter/incrementingeventcounter/**">
34353441
<Issue>System.Threading.Thread.UnsafeStart not supported</Issue>
34363442
</ExcludeList>

0 commit comments

Comments
 (0)