Skip to content
This repository was archived by the owner on Sep 25, 2026. It is now read-only.

Intentionally skip switch instructions during branch removal as the code doesn't handle it yet (throws NIE). #2889

Merged
vitek-karas merged 1 commit into from
Jul 12, 2022
Merged

Intentionally skip switch instructions during branch removal as the code doesn't handle it yet (throws NIE). #2889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,10 @@ bool RemoveConditions ()
var opcode = instr.OpCode;

if (opcode.FlowControl == FlowControl.Cond_Branch) {
// No support for removing branches from switch instruction
if (opcode == OpCodes.Switch)
continue;

if (opcode.StackBehaviourPop == StackBehaviour.Pop1_pop1) {
if (!GetOperandsConstantValues (i, out left, out right))
continue;
Expand Down Expand Up @@ -892,7 +896,7 @@ bool RemoveConditions ()
}

// Common pattern generated by C# compiler in debug mode
if (i > 3 && GetConstantValue (FoldedInstructions[i - 3], out operand) && operand is int opint2 && IsPairedStlocLdloc (FoldedInstructions[i - 2], FoldedInstructions[i - 1])) {
if (i >= 3 && GetConstantValue (FoldedInstructions[i - 3], out operand) && operand is int opint2 && IsPairedStlocLdloc (FoldedInstructions[i - 2], FoldedInstructions[i - 1])) {

@vitek-karas vitek-karas Jul 12, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the case where the switch is the first thing in a method body - in which case the logic here would not kick in (now switch is disabled above, but if it's not it would not be recognized due to this).

@agocke agocke Jul 12, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm having trouble visualizing the kind of code this is matching. Could you write a comment with an example?

@sbomer sbomer Jul 12, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it is a bug not just for switch statements but for any branching instruction we consider, right? I wonder if we can write a test for this fix where the compiler doesn't emit a nop as the first instruction.

@agocke I think it's for cases where the compiler emits code like this:

call bool C::get_ConstantProperty()
stloc.0
ldloc.0
brfalse.s IL_xxxx // or a switch

@vitek-karas vitek-karas Jul 12, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly - for some reason Roslyn always put the stloc/ldloc pair even in optimized build - in debug build there are actually two pairs like that one after another. The code would not handle the debug build at all right now, but that's for when we actually support switch.

if (IsJumpTargetRange (i - 2, i))
continue;

Expand Down
9 changes: 1 addition & 8 deletions test/Mono.Linker.Tests.Cases/UnreachableBlock/ComplexConditionsOptimized.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ static int KnownInteger {
[ExpectBodyModified]
public static void Test ()
{
// https://github.com/dotnet/linker/issues/2872
// Uncomment the call to make the test crash the linker
//Helper ();
switch (KnownInteger) {
case 0:
Unreached ();
Expand All @@ -44,17 +41,13 @@ public static void Test ()
}
}

// https://github.com/dotnet/linker/issues/2872
// https://github.com/dotnet/linker/issues/2888
// Should be removed
[Kept]
static void Unreached () { }

[Kept]
static void Reached () { }

// https://github.com/dotnet/linker/issues/2872
//[Kept]
static void Helper () { }
}
}
}