Skip to content

Commit

Permalink
Fix too narrow loads while unspilling (#66675)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jakobbotsch authored Mar 19, 2022
1 parent d2fa88e commit 3e2a45c
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 15 deletions.
23 changes: 8 additions & 15 deletions src/coreclr/jit/codegenlinear.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1213,22 +1213,15 @@ void CodeGen::genUnspillRegIfNeeded(GenTree* tree)
// TODO-Cleanup: The following code could probably be further merged and cleaned up.
#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
// Load local variable from its home location.
// In most cases the tree type will indicate the correct type to use for the load.
// However, if it is NOT a normalizeOnLoad lclVar (i.e. NOT a small int that always gets
// widened when loaded into a register), and its size is not the same as the actual register type
// of the lclVar, then we need to change the type of the tree node when loading.
// This situation happens due to "optimizations" that avoid a cast and
// simply retype the node when using long type lclVar as an int.
// While loading the int in that case would work for this use of the lclVar, if it is
// later used as a long, we will have incorrectly truncated the long.
// In the normalizeOnLoad case ins_Load will return an appropriate sign- or zero-
// extending load.
var_types lclActualType = varDsc->GetActualRegisterType();
assert(lclActualType != TYP_UNDEF);
if (spillType != lclActualType && !varTypeIsGC(spillType) && !varDsc->lvNormalizeOnLoad())
// Never allow truncating the locals here, otherwise a subsequent
// use of the local with a wider type would see the truncated
// value. We do allow wider loads as those can be efficient even
// when unaligned and might be smaller encoding wise (on xarch).
var_types lclLoadType = varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : varDsc->GetActualRegisterType();
assert(lclLoadType != TYP_UNDEF);
if (genTypeSize(spillType) < genTypeSize(lclLoadType))
{
assert(!varTypeIsGC(varDsc));
spillType = lclActualType;
spillType = lclLoadType;
}
#elif defined(TARGET_ARM)
// No normalizing for ARM
Expand Down
116 changes: 116 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_66624/Runtime_66624.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Generated by Fuzzlyn v1.5 on 2022-03-14 21:20:55
// Run on X64 Windows
// Seed: 16520696696442011600
// Reduced from 677.9 KiB to 2.4 KiB in 00:04:01
// Debug: Outputs 53474
// Release: Outputs 226
public class C0
{
public byte F0;
public sbyte F3;
public byte F4;
public byte F5;
public uint F6;
public C0(byte f4, byte f5, uint f6)
{
F4 = f4;
F5 = f5;
F6 = f6;
}
}

public class C1
{
public C0 F4;
public C1(C0 f4)
{
F4 = f4;
}
}

public struct S0
{
public sbyte F0;
public C0 F2;
public bool F4;
public ulong F5;
public uint F6;
public bool F7;
public S0(sbyte f0, C0 f1, C0 f2, bool f4, ulong f5, uint f6, bool f7) : this()
{
F0 = f0;
F2 = f2;
F4 = f4;
F5 = f5;
F6 = f6;
F7 = f7;
}
}

public class Runtime_66624
{
public static IRuntime s_rt;
public static C0 s_1 = new C0(0, 0, 0);
public static C1[][] s_3 = new C1[][] { new C1[] { new C1(new C0(0, 0, 0)) } };
public static int Main()
{
CollectibleALC alc = new CollectibleALC();
System.Reflection.Assembly asm = alc.LoadFromAssemblyPath(System.Reflection.Assembly.GetExecutingAssembly().Location);
System.Reflection.MethodInfo mi = asm.GetType(typeof(Runtime_66624).FullName).GetMethod(nameof(MainInner));
System.Type runtimeTy = asm.GetType(typeof(Runtime).FullName);
return (int)mi.Invoke(null, new object[] { System.Activator.CreateInstance(runtimeTy) });
}

public static int MainInner(IRuntime rt)
{
s_rt = rt;
return M10(53474);
}

public static int M10(ushort arg0)
{
var vr1 = new S0[] { new S0(0, new C0(0, 0, 0), new C0(0, 0, 0), false, 0, 0, false) };
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);
try
{
byte vr5 = var1.F2.F0;
}
finally
{
var1 = new S0(0, new C0(0, 0, 0), new C0(0, 1, 0), true, 0, 0, true);
}

if ((byte)arg0 < 1)
{
C0[] var4 = new C0[] { new C0(0, 0, 0) };
}

s_rt.WriteLine("c_4102", arg0);
return ((Runtime)s_rt).Result;
}
}

public interface IRuntime
{
void WriteLine<T>(string site, T value);
}

public class Runtime : IRuntime
{
public int Result;

public void WriteLine<T>(string site, T value)
{
Result = (ushort)(object)value == 53474 ? 100 : -1;
}
}

public class CollectibleALC : System.Runtime.Loader.AssemblyLoadContext
{
public CollectibleALC() : base(true)
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
<CLRTestPriority>1</CLRTestPriority>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
6 changes: 6 additions & 0 deletions src/tests/issues.targets
Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,9 @@
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_64883/Runtime_64883/*">
<Issue>https://github.com/dotnet/runtimelab/issues/155: Collectible assemblies</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_66624/Runtime_66624/*">
<Issue>https://github.com/dotnet/runtimelab/issues/155: Collectible assemblies</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/Loader/ContextualReflection/ContextualReflection/*">
<Issue>https://github.com/dotnet/runtimelab/issues/165</Issue>
</ExcludeList>
Expand Down Expand Up @@ -3431,6 +3434,9 @@
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_64883/Runtime_64883/*">
<Issue>Loads an assembly from file</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_66624/Runtime_66624/*">
<Issue>Loads an assembly from file</Issue>
</ExcludeList>
<ExcludeList Include = "$(XunitTestBinBase)/tracing/eventcounter/incrementingeventcounter/**">
<Issue>System.Threading.Thread.UnsafeStart not supported</Issue>
</ExcludeList>
Expand Down

0 comments on commit 3e2a45c

Please sign in to comment.