Skip to content

Commit f826037

Browse files
committed
Protect IsInst against multi-step inlining -- we can only allow Box as the top-level argument, not anywhere within the argument tree.
1 parent 8ad33f1 commit f826037

File tree

2 files changed

+11
-3
lines changed

2 files changed

+11
-3
lines changed

ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -958,6 +958,10 @@ internal virtual bool CanInlineIntoSlot(int childIndex, ILInstruction expression
958958
/// Unlike `CanInlineIntoSlot`, this is not about descendants of the slot, only about
959959
/// whether SetChild(childIndex, newChild) is valid.
960960
/// (i.e. this controls whether FindLoadInNext may return the specified slot as a final result)
961+
///
962+
/// Warning: after newChild is inlined, other nodes may be inlined into newChild's sub-instructions
963+
/// without asking this function again. This means this function is not suitable for protecting
964+
/// a slot from having side effects, use `CanInlineIntoSlot` for that.
961965
/// </summary>
962966
internal virtual bool SatisfiesSlotRestrictionForInlining(int childIndex, ILInstruction newChild)
963967
{

ICSharpCode.Decompiler/IL/Instructions/IsInst.cs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,10 @@ namespace ICSharpCode.Decompiler.IL;
2525

2626
partial class IsInst
2727
{
28-
internal override bool SatisfiesSlotRestrictionForInlining(int childIndex, ILInstruction newChild)
28+
internal override bool CanInlineIntoSlot(int childIndex, ILInstruction newChild)
2929
{
3030
Debug.Assert(childIndex == 0);
31-
Debug.Assert(base.SatisfiesSlotRestrictionForInlining(childIndex, newChild));
31+
Debug.Assert(base.CanInlineIntoSlot(childIndex, newChild));
3232
if (this.Type.IsReferenceType == true)
3333
{
3434
return true; // reference-type isinst is always supported
@@ -37,12 +37,16 @@ internal override bool SatisfiesSlotRestrictionForInlining(int childIndex, ILIns
3737
{
3838
return true; // emulated via "expr is T ? (T)expr : null"
3939
}
40-
else if (newChild is Box box && SemanticHelper.IsPure(box.Argument.Flags))
40+
else if (newChild is Box box && SemanticHelper.IsPure(box.Argument.Flags) && this.Argument.Children.Count == 0)
4141
{
4242
// Also emulated via "expr is T ? (T)expr : null".
4343
// This duplicates the boxing side-effect, but that's harmless as one of the boxes is only
4444
// used in the `expr is T` type test where the object identity can never be observed.
4545
// This appears as part of C# pattern matching, inlining early makes those code patterns easier to detect.
46+
47+
// We can only do this if the Box appears directly top-level in the IsInst, we cannot inline Box instructions
48+
// deeper into our Argument subtree. So restricts to the case were the previous argument has no children
49+
// (which means inlining can only replace the argument, not insert within it).
4650
return true;
4751
}
4852
if (this.Parent is UnboxAny unboxAny && ExpressionBuilder.IsUnboxAnyWithIsInst(unboxAny, this.Type))

0 commit comments

Comments
 (0)