Browse Source

Fix #2032: avoid propagating mutated SROA fields

Aggressive scalar replacement propagated a display-class field to its
source variable even when the field is mutated after initialization,
aliasing two distinct source-level variables (Test9: thisField and
this). Propagation is now cancelled when the field sees a second store
or its address escapes, but only for propagation targets that cannot
absorb the store: 'this' and variables that are themselves
scalar-replaced display classes. Parameters continue to propagate,
because their remaining uses are already restricted by
ResolveVariableToPropagate and a captured parameter mutated inside a
lambda (DelegateConstruction's Bug951) must keep mapping to the
parameter. Checking CanPropagate first also keeps the guard away from
Mono state-machine fields, whose VariableToDeclare is pre-bound to a
state-machine variable that Propagate(null) would discard.

Re-enables Test9 and adds Test10 covering the escaping-address variant
(Interlocked.Exchange(ref displayClass.thisField, ...)).

Assisted-by: OpenCode:openai/gpt-5.5:OpenCode
Assisted-by: Claude:claude-fable-5:Claude Code
pull/3943/head
Siegfried Pammer 2 months ago committed by Siegfried Pammer
parent
commit
49bdca02d0
  1. 26
      ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.Expected.cs
  2. 32
      ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.cs
  3. 30
      ICSharpCode.Decompiler/IL/Transforms/TransformDisplayClassUsage.cs

26
ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.Expected.cs

@ -1,4 +1,5 @@ @@ -1,4 +1,5 @@
using System;
using System.Threading;
namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly
{
@ -166,13 +167,22 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly @@ -166,13 +167,22 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly
Console.WriteLine("{0} {1}", field1, field2);
}
// public void Test9()
// {
// Program thisField = this;
// int field1 = 1;
// string field2 = "Hello World!";
// thisField = new Program();
// Console.WriteLine("{0} {1}", this, thisField);
// }
public void Test9()
{
Program thisField = this;
int field1 = 1;
string field2 = "Hello World!";
thisField = new Program();
Console.WriteLine("{0} {1}", this, thisField);
}
public void Test10()
{
Program thisField = this;
int field1 = 1;
string field2 = "Hello World!";
Interlocked.Exchange(ref thisField, new Program());
Console.WriteLine("{0} {1}", this, thisField);
}
}
}

32
ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.cs

@ -1,4 +1,5 @@ @@ -1,4 +1,5 @@
using System;
using System.Threading;
namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly
{
@ -180,15 +181,26 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly @@ -180,15 +181,26 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly
Console.WriteLine("{0} {1}", displayClass.field1, displayClass.field2);
}
// public void Test9()
// {
// DisplayClass displayClass = new DisplayClass {
// thisField = this,
// field1 = 1,
// field2 = "Hello World!"
// };
// displayClass.thisField = new Program();
// Console.WriteLine("{0} {1}", this, displayClass.thisField);
// }
public void Test9()
{
DisplayClass displayClass = new DisplayClass {
thisField = this,
field1 = 1,
field2 = "Hello World!"
};
displayClass.thisField = new Program();
Console.WriteLine("{0} {1}", this, displayClass.thisField);
}
public void Test10()
{
DisplayClass displayClass = new DisplayClass {
thisField = this,
field1 = 1,
field2 = "Hello World!"
};
Interlocked.Exchange(ref displayClass.thisField, new Program());
Console.WriteLine("{0} {1}", this, displayClass.thisField);
}
}
}

30
ICSharpCode.Decompiler/IL/Transforms/TransformDisplayClassUsage.cs

@ -227,6 +227,19 @@ namespace ICSharpCode.Decompiler.IL.Transforms @@ -227,6 +227,19 @@ namespace ICSharpCode.Decompiler.IL.Transforms
{
variable = AddVariable(container, null, field);
}
if (variable.CanPropagate && !IsReadOnlyOrInitializerUse(ldflda, variable))
{
// The field is mutated after initialization (stored again, or its address
// escapes). Propagation is only sound if the store can be redirected to
// the propagated variable: possible for parameters (their remaining uses
// are restricted in ResolveVariableToPropagate), but not for 'this' and
// not for a variable that is itself a scalar-replaced display class.
var propagatedVariable = variable.GetOrDeclare();
if (propagatedVariable.IsThis() || displayClasses.ContainsKey(propagatedVariable))
{
variable.Propagate(null);
}
}
container.VariablesToDeclare[keyField] = variable;
return true;
case StObj stobj when stobj.MatchStObj(out var target, out ILInstruction value, out _) && value == use:
@ -245,6 +258,23 @@ namespace ICSharpCode.Decompiler.IL.Transforms @@ -245,6 +258,23 @@ namespace ICSharpCode.Decompiler.IL.Transforms
}
}
/// <summary>
/// True when the given field access is a plain read or one of the recorded
/// initializer stores; false for any other store or for an escaping address.
/// </summary>
static bool IsReadOnlyOrInitializerUse(LdFlda ldflda, VariableToDeclare variable)
{
switch (ldflda.Parent)
{
case LdObj:
return true;
case StObj store when store.Target == ldflda:
return variable.Initializers.Contains(store);
default:
return false;
}
}
private DisplayClass AnalyzeVariable(ILVariable v)
{
switch (v.Kind)

Loading…
Cancel
Save