From 49bdca02d01001ea364d1cef728b9ad3027ef744 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Tue, 28 Jul 2026 22:14:15 +0200 Subject: [PATCH] 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 --- ...eScalarReplacementOfAggregates.Expected.cs | 26 ++++++++++----- ...AggressiveScalarReplacementOfAggregates.cs | 32 +++++++++++++------ .../Transforms/TransformDisplayClassUsage.cs | 30 +++++++++++++++++ 3 files changed, 70 insertions(+), 18 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.Expected.cs b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.Expected.cs index c6d1f556e..33ec7b0b0 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.Expected.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.Expected.cs @@ -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 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); + } } } diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.cs b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.cs index 5173011b3..ba985d019 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.cs @@ -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 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); + } } } diff --git a/ICSharpCode.Decompiler/IL/Transforms/TransformDisplayClassUsage.cs b/ICSharpCode.Decompiler/IL/Transforms/TransformDisplayClassUsage.cs index d10fe6388..1183f4f8b 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/TransformDisplayClassUsage.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/TransformDisplayClassUsage.cs @@ -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 } } + /// + /// 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. + /// + 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)