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

Bad codegen for unused ref int local in Release builds #53113

Closed
jkotas opened this issue May 3, 2021 · 5 comments · Fixed by #56790
Closed

Bad codegen for unused ref int local in Release builds #53113

jkotas opened this issue May 3, 2021 · 5 comments · Fixed by #56790
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug
Milestone

Comments

@jkotas
Copy link
Member

jkotas commented May 3, 2021

Version Used: .NET Core 6.0.100-preview.2.21155.3

Steps to Reproduce:

  1. Add <AllowUnsafeBlocks>true</AllowUnsafeBlocks> to .csproj and replace the Program.cs in the default console app with:
using System;

unsafe
{
    ref int x = ref *(int*)0;
    Console.WriteLine("Hello World!");
}
  1. dotnet run
  2. dotnet run -c Release

Expected Behavior:

Debug and Release builds have the same behavior

Actual Behavior:

Debug prints "Hello world"
Release fails with System.NullReferenceException

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 3, 2021
@jkotas
Copy link
Member Author

jkotas commented May 3, 2021

The problem is that the release codegen has a superfluous ldind.i4 instruction:

  IL_0000:  ldc.i4.0
  IL_0001:  conv.i
  IL_0002:  ldind.i4 <- This ldind.i4 is superfluous and causing the bug
  IL_0003:  pop
  IL_0004:  ldstr      "Hello World!"
  IL_0009:  call       void [System.Console]System.Console::WriteLine(string)
  IL_000e:  ret

@NN---
Copy link

NN--- commented May 6, 2021

Dereferencing null pointer is a bad idea no matter what compiler generates :)

The effect of applying the unary * operator to a null pointer is implementation-defined. In particular, there is no guarantee that this operation throws a System.NullReferenceException.

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/unsafe-code

@jkotas
Copy link
Member Author

jkotas commented May 6, 2021

I have used null pointer in the repro just to make it minimal. The situation where I have run into this issue was more complex interop code and the pointer was actually non-null.

Regardless, the IL produced by the compiler in Release builds is wrong.

@jaredpar jaredpar added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 13, 2021
@jaredpar jaredpar added this to the 17.0 milestone Jul 13, 2021
@333fred
Copy link
Member

333fred commented Jul 13, 2021

@jkotas while I agree that release and debug should certainly have the same behavior, I come to the opposite conclusion as to what that behavior should be: both should be null referencing, and the debug code is missing a ldind instruction, not the other way around. As your repro demonstrates, a pointer indirection instruction is potentially side-effecting, and it is not valid for the C# compiler to eliminate that pointer dereference even if the result is unused. Debug is doing something odd because of the ref that I need to dig into, but the simpler example of int i = *(int*)0; will null-ref in both debug and release correctly (well, as correctly as "it actually dereferences the 0 and the platform defines what will happen in that case").

@jkotas
Copy link
Member Author

jkotas commented Jul 13, 2021

Debug is doing something odd because of the ref

I think that Debug works correctly. When you are assigning one ref int to another ref int, there should not ever be a dereference of the value. If you disagree, what should be the spec for when assigning one ref int to another ref int dereferences the value?

int i = *(int*)0;

This one is not assigning one ref int to another ref int. It is expected that it dereferences the value.

@jaredpar jaredpar assigned chsienki and unassigned 333fred Aug 3, 2021
@jaredpar jaredpar added the Test Test failures in roslyn-CI label Aug 3, 2021
@jaredpar jaredpar removed the Test Test failures in roslyn-CI label Sep 27, 2021
@333fred 333fred assigned 333fred and unassigned chsienki Sep 27, 2021
333fred added a commit to 333fred/roslyn that referenced this issue Sep 28, 2021
Fixes dotnet#53113. Previously, when we did stack local optimization, we'd erase the assignment (including the context of the assignment being by-ref), leaving just the pointer indirection. This is incorrect, and resulted in us generating a ldind where we shouldn't. This fixes the bug by tracking whether we're currently optimizing a stack-scheduled assignment, and seeing through the pointer indirection in that case.
@333fred 333fred added the 4 - In Review A fix for the issue is submitted for review. label Sep 28, 2021
333fred added a commit that referenced this issue Oct 3, 2021
)

* See through ref pointer indirection when optimizing stack locals

Fixes #53113. Previously, when we did stack local optimization, we'd erase the assignment (including the context of the assignment being by-ref), leaving just the pointer indirection. This is incorrect, and resulted in us generating a ldind where we shouldn't. This fixes the bug by tracking whether a pointer indirection operator is referring to a location in initial binding, and only emitting the address when that is true.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants