Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Branch removal can't remove all branches in compiler generated state machine #3087

Open
vitek-karas opened this issue Oct 27, 2022 · 2 comments

Comments

@vitek-karas
Copy link
Member

Take for example this code:

static IEnumerable<int> TestBranchWithYieldBefore ()
{
    if (AlwaysFalse) { // Property which returns constant false
        yield return 1;
        RemovedMethod ();  // This is unreachable code and should be removed, but it's not
    } else {
        yield return 1;
        UsedMethod ();
    }
}

The problem is that the compiler generated code produces the if (AlwaysFalse) where its body is just some state machine setup because there's a yield there. And so it means that even if that branch is removed, the call to RemovedMethod is still kept because it's part of a different state in the state machine.

private bool MoveNext()
{
	switch (<>1__state)
	{
	default:
		return false;
	case 0:
		<>1__state = -1;
		if (AlwaysFalse) // This is the branch which can be optimized
		{
			<>2__current = 1; // This will be removed...
			<>1__state = 1;
			return true;
		}
		<>2__current = 1;
		<>1__state = 2;
		return true;
	case 1:
		<>1__state = -1;
		RemovedMethod();  // But this is still kept, since there's not constant branch around it
		goto IL_007a;

In order to correctly implement this, the analysis would have to understand at least basics of state machines. Detecting this in a general case is really complicated because the state is stored in a field, and so we would have to be able to prove that the field can only have a certain set of values - which is really hard in multi-threaded environments.

So in theory doable, but pretty complex.

The downside of this issue is that it's hard to diagnose what's happening for the user. If this is used in combination of feature switches then this can lead to unexpected warnings, since code which is technically hidden behind a feature switch is still marked as reachable by the linker.

vitek-karas added a commit to vitek-karas/linker that referenced this issue Oct 27, 2022
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking

This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.

To make the code simple I added a hashset to prevent multiple optimization runs on the same method. Technically this can be guaranteed by the caller, but it's really complex and very fragile.

Added tests.

Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet#3087
@marek-safar
Copy link
Contributor

I think it might not complicate to implement for C# state machine because compiler usually assign the field in single method body only.

Did you find that in some real code somewhere?

@vitek-karas
Copy link
Member Author

I didn't run into this in real code yet. But it's effectively hidden behind #3088.
I agree that if we special case this for C# state machines this is definitely solvable.

vitek-karas added a commit that referenced this issue Nov 1, 2022
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking

This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.

The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method.

This does have one possibly negative impact: the issue described in #2937 is now consistent and happens always.

Added tests.

Note that there's still a whole in analysis of compiler generated code around state machines, see #3087

Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar.

In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).
tlakollo pushed a commit to dotnet/runtime that referenced this issue Nov 4, 2022
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking

This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.

The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method.

This does have one possibly negative impact: the issue described in dotnet/linker#2937 is now consistent and happens always.

Added tests.

Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet/linker#3087

Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar.

In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).

Commit migrated from dotnet/linker@e502e72
agocke pushed a commit to dotnet/runtime that referenced this issue Nov 16, 2022
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking

This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.

The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method.

This does have one possibly negative impact: the issue described in dotnet/linker#2937 is now consistent and happens always.

Added tests.

Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet/linker#3087

Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar.

In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).

Commit migrated from dotnet/linker@e502e72
vitek-karas added a commit to vitek-karas/linker that referenced this issue Dec 13, 2022
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking

This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.

The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method.

This does have one possibly negative impact: the issue described in dotnet#2937 is now consistent and happens always.

Added tests.

Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet#3087

Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar.

In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).
sbomer pushed a commit that referenced this issue Jan 18, 2023
* Fix branch removal in compiler generated code (#3088)

Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking

This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.

The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method.

This does have one possibly negative impact: the issue described in #2937 is now consistent and happens always.

Added tests.

Note that there's still a whole in analysis of compiler generated code around state machines, see #3087

Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar.

In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).

* PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants