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

Plain struct not copied correctly when passed to method under certain conditions #89774

Closed
odyssjii opened this issue Aug 1, 2023 · 23 comments
Closed
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Milestone

Comments

@odyssjii
Copy link

odyssjii commented Aug 1, 2023

Description

When passing a struct to a method on a class, if there is a second parameter of interface type and an instance of a struct implementing that interface is passed in as the second parameter, then if stepping into the record constructor of the struct and then stepping into the method using a debugger via VS Code or Visual Studio for Mac two of the fields of the first parameter are set to null.

This change in the value is "real" and not a visual bug in the debugger, attempting to access or read that value will confirm this.

Note that the bug was detected whilst running code without a debugger, when inexplicable null values were detected deeper in the call stack. So under certain conditions it happens even without stepping in the debugger, but this is a reliable repro.

Screenshot 2023-08-01 at 14 18 33 Screenshot 2023-08-01 at 14 19 08

Reproduction Steps

To produce the bug: place breakpoint on the first line, step into the constructor of the record, then step into the method Z::Foo, inspect the method argument obj and X::D and X::E have been set to null.

The second line which passes new B() instead of new A() does not have this issue.

Also placing breakpoint within Z::Foo and running directly to that point (do not step into record constructor) will NOT produce the bug.

Not related to using record struct, same problem with a regular struct.

new Z().Foo(new X(8, "hello", "world", "cool", "stuff", "baby"), new A());

new Z().Foo(new X(8, "hello", "world", "cool", "stuff", "baby"), new B());

sealed class Z
{
    public void Foo(X obj, IA service)
    {
        Console.WriteLine(string.Join(" ", obj.A, obj.B, obj.C, obj.D, obj.E, obj.F));
    }
}

public readonly record struct X(uint A, string B, string C, string D, string E, string F);

interface IA
{
    bool Bar();
}

readonly struct A : IA
{
    public readonly bool Bar() => true;
}

class B : IA
{
    public bool Bar() => true;
}

Expected behavior

When an instance of a plain struct is passed to a method, it is expected that the copy of the struct should arrive whole and intact -- i.e. some fields should not suddenly go missing or be altered.

Actual behavior

Under certain conditions, when passing an instance of a struct to method, some of the fields of the struct are missing in the copy of the instance accessible from within the method.

Regression?

No response

Known Workarounds

No response

Configuration

.net version: 7.0.304
OS: macOS Ventura 13.4.1
Arch: x64

VS Code and Visual Studio for Mac both exhibit the same behaviour.

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 1, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 1, 2023
@teo-tsirpanis teo-tsirpanis added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 1, 2023
@ghost
Copy link

ghost commented Aug 1, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When passing a struct to a method on a class, if there is a second parameter of interface type and an instance of a struct implementing that interface is passed in as the second parameter, then if stepping into the record constructor of the struct and then stepping into the method using a debugger via VS Code or Visual Studio for Mac two of the fields of the first parameter are set to null.

This change in the value is "real" and not a visual bug in the debugger, attempting to access or read that value will confirm this.

Note that the bug was detected whilst running code without a debugger, when inexplicable null values were detected deeper in the call stack. So under certain conditions it happens even without stepping in the debugger, but this is a reliable repro.

Screenshot 2023-08-01 at 14 18 33 Screenshot 2023-08-01 at 14 19 08

Reproduction Steps

To produce the bug: place breakpoint on the first line, step into the constructor of the record, then step into the method Z::Foo, inspect the method argument obj and X::D and X::E have been set to null.

The second line which passes new B() instead of new A() does not have this issue.

Also placing breakpoint within Z::Foo and running directly to that point (do not step into record constructor) will NOT produce the bug.

Not related to using record struct, same problem with a regular struct.

new Z().Foo(new X(8, "hello", "world", "cool", "stuff", "baby"), new A());

new Z().Foo(new X(8, "hello", "world", "cool", "stuff", "baby"), new B());

sealed class Z
{
    public void Foo(X obj, IA service)
    {
        Console.WriteLine(string.Join(" ", obj.A, obj.B, obj.C, obj.D, obj.E, obj.F));
    }
}

public readonly record struct X(uint A, string B, string C, string D, string E, string F);

interface IA
{
    bool Bar();
}

readonly struct A : IA
{
    public readonly bool Bar() => true;
}

class B : IA
{
    public bool Bar() => true;
}

Expected behavior

When an instance of a plain struct is passed to a method, it is expected that the copy of the struct should arrive whole and intact -- i.e. some fields should not suddenly go missing or be altered.

Actual behavior

Under certain conditions, when passing an instance of a struct to method, some of the fields of the struct are missing in the copy of the instance accessible from within the method.

Regression?

No response

Known Workarounds

No response

Configuration

.net version: 7.0.304
OS: macOS Ventura 13.4.1
Arch: x64

VS Code and Visual Studio for Mac both exhibit the same behaviour.

Other information

No response

Author: odyssjii
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@filipnavara
Copy link
Member

I tried it on SDKs 7.0.306 and 8.0.100-preview.7. It didn't repro on my macOS machine in either x64 or arm64 builds.

@AndyAyersMS
Copy link
Member

@odyssjii was this with a debug or release build?

@odyssjii
Copy link
Author

odyssjii commented Aug 2, 2023

@AndyAyersMS This is with debug build.

Here is repro in VS Code on a different machine also running macOS:

image

dotnet version: 7.0.304
MSBuild version 17.6.3+07e294721 for .NET
macOS Ventura 13.4.1

Note that both of the machines are Apple computers with Intel CPUs.

@jakobbotsch
Copy link
Member

jakobbotsch commented Aug 2, 2023

Do I understand correctly that this only repros under the debugger and requires stepping in a very specific pattern?

Note that the bug was detected whilst running code without a debugger, when inexplicable null values were detected deeper in the call stack. So under certain conditions it happens even without stepping in the debugger, but this is a reliable repro.

Do you happen to have an example that doesn't require attaching and stepping with a debugger? It sounds likely these could be two separate issues.

@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Aug 2, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 2, 2023
@odyssjii
Copy link
Author

odyssjii commented Aug 3, 2023

Hi @jakobbotsch ! I will try to construct a repro that does not require the stepping in with a debugger. It is not as straightforward though, I still don't know under what condition that happens.

@jakobbotsch
Copy link
Member

Thanks @odyssjii! If you are able to reproduce it without a debugger it would definitely indicate some sort of codegen bug. Sadly I don't have an Intel Mac and it seems like VSCode/OmniSharp does not support debugging with an SDK running under Rosetta, so I cannot try it while stepping.

@dotnet/jit-contrib Anyone with an Intel Mac who wants to see if they are able to repro it? I've looked at the x64 debug codegen on my Mac and I do not see anything suspicious. I've also tried reproducing it when stepping on Ubuntu x64 (which should be the exact same codegen since it's the same ABI), but haven't had luck.

@jakobbotsch jakobbotsch modified the milestones: 8.0.0, 9.0.0 Aug 7, 2023
@jakobbotsch
Copy link
Member

@tommcdon Can the diagnostics team take a look at this? It seems to be related to stepping, and unfortunately I don't have an x64 Mac to take a look.

@tommcdon
Copy link
Member

tommcdon commented Aug 7, 2023

@jakobbotsch jakobbotsch added area-Diagnostics-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 9, 2023
@jakobbotsch jakobbotsch removed their assignment Aug 9, 2023
@ghost
Copy link

ghost commented Aug 9, 2023

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When passing a struct to a method on a class, if there is a second parameter of interface type and an instance of a struct implementing that interface is passed in as the second parameter, then if stepping into the record constructor of the struct and then stepping into the method using a debugger via VS Code or Visual Studio for Mac two of the fields of the first parameter are set to null.

This change in the value is "real" and not a visual bug in the debugger, attempting to access or read that value will confirm this.

Note that the bug was detected whilst running code without a debugger, when inexplicable null values were detected deeper in the call stack. So under certain conditions it happens even without stepping in the debugger, but this is a reliable repro.

Screenshot 2023-08-01 at 14 18 33 Screenshot 2023-08-01 at 14 19 08

Reproduction Steps

To produce the bug: place breakpoint on the first line, step into the constructor of the record, then step into the method Z::Foo, inspect the method argument obj and X::D and X::E have been set to null.

The second line which passes new B() instead of new A() does not have this issue.

Also placing breakpoint within Z::Foo and running directly to that point (do not step into record constructor) will NOT produce the bug.

Not related to using record struct, same problem with a regular struct.

new Z().Foo(new X(8, "hello", "world", "cool", "stuff", "baby"), new A());

new Z().Foo(new X(8, "hello", "world", "cool", "stuff", "baby"), new B());

sealed class Z
{
    public void Foo(X obj, IA service)
    {
        Console.WriteLine(string.Join(" ", obj.A, obj.B, obj.C, obj.D, obj.E, obj.F));
    }
}

public readonly record struct X(uint A, string B, string C, string D, string E, string F);

interface IA
{
    bool Bar();
}

readonly struct A : IA
{
    public readonly bool Bar() => true;
}

class B : IA
{
    public bool Bar() => true;
}

Expected behavior

When an instance of a plain struct is passed to a method, it is expected that the copy of the struct should arrive whole and intact -- i.e. some fields should not suddenly go missing or be altered.

Actual behavior

Under certain conditions, when passing an instance of a struct to method, some of the fields of the struct are missing in the copy of the instance accessible from within the method.

Regression?

No response

Known Workarounds

No response

Configuration

.net version: 7.0.304
OS: macOS Ventura 13.4.1
Arch: x64

VS Code and Visual Studio for Mac both exhibit the same behaviour.

Other information

No response

Author: odyssjii
Assignees: jakobbotsch
Labels:

area-Diagnostics-coreclr

Milestone: 8.0.0

@tommcdon
Copy link
Member

tommcdon commented Aug 9, 2023

Note that the bug was detected whilst running code without a debugger

@jakobbotsch would mind explaining why this issue was assigned to the debugger team? The customer provided a repro with the debugger attached but it seems it reproduces without the debugger as well per the comment above.

@tommcdon
Copy link
Member

tommcdon commented Aug 9, 2023

Hi @odyssjii, would you mind trying COMPlus_EnableAVX=0 to see if the issue no longer reproduces? I tested the repro on an early Spring 2013 x64 mac and it didn't repro. It's my understanding that model does not support AVX and so I am suspecting this is related to AVX code generation. Testing with the suggested env variable on an AVX-enabled mac would help confirm.

@jakobbotsch
Copy link
Member

@jakobbotsch would mind explaining why this issue was assigned to the debugger team? The customer provided a repro with the debugger attached but it seems it reproduces without the debugger as well per the comment above.

I have looked at the codegen and I see nothing wrong with it. If there is a possible codegen issue then I think it is a separate issue -- I would gladly look at that once an example is provided.

Note that it is not just about the debugger being attached -- the debugger also needs to be used to step in a specific pattern, as far as I understand.

@tommcdon
Copy link
Member

tommcdon commented Aug 9, 2023

the debugger also needs to be used to step in a specific pattern, as far as I understand.

@odyssjii would you mind confirming the above statement? From what I understand is that this bug reproduces without the debugger attached and that the debugger-based stepping repro was merely a reliable way of reproducing it.

@tommcdon
Copy link
Member

tommcdon commented Aug 9, 2023

Per offline conv with @jakobbotsch, moving this issue back to codegen assuming that the issue reproduces without the debugger and moving to the 9.0 milestone.
We will treat the debugger issue as a separate bug. If the issue does not reproduce with COMPlus_EnableAVX=0 then the debugger version of the repro is likely related to #78991, otherwise we will create a new bug tracking the debugger side of the problem.

@tommcdon tommcdon added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 9, 2023
@ghost
Copy link

ghost commented Aug 9, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When passing a struct to a method on a class, if there is a second parameter of interface type and an instance of a struct implementing that interface is passed in as the second parameter, then if stepping into the record constructor of the struct and then stepping into the method using a debugger via VS Code or Visual Studio for Mac two of the fields of the first parameter are set to null.

This change in the value is "real" and not a visual bug in the debugger, attempting to access or read that value will confirm this.

Note that the bug was detected whilst running code without a debugger, when inexplicable null values were detected deeper in the call stack. So under certain conditions it happens even without stepping in the debugger, but this is a reliable repro.

Screenshot 2023-08-01 at 14 18 33 Screenshot 2023-08-01 at 14 19 08

Reproduction Steps

To produce the bug: place breakpoint on the first line, step into the constructor of the record, then step into the method Z::Foo, inspect the method argument obj and X::D and X::E have been set to null.

The second line which passes new B() instead of new A() does not have this issue.

Also placing breakpoint within Z::Foo and running directly to that point (do not step into record constructor) will NOT produce the bug.

Not related to using record struct, same problem with a regular struct.

new Z().Foo(new X(8, "hello", "world", "cool", "stuff", "baby"), new A());

new Z().Foo(new X(8, "hello", "world", "cool", "stuff", "baby"), new B());

sealed class Z
{
    public void Foo(X obj, IA service)
    {
        Console.WriteLine(string.Join(" ", obj.A, obj.B, obj.C, obj.D, obj.E, obj.F));
    }
}

public readonly record struct X(uint A, string B, string C, string D, string E, string F);

interface IA
{
    bool Bar();
}

readonly struct A : IA
{
    public readonly bool Bar() => true;
}

class B : IA
{
    public bool Bar() => true;
}

Expected behavior

When an instance of a plain struct is passed to a method, it is expected that the copy of the struct should arrive whole and intact -- i.e. some fields should not suddenly go missing or be altered.

Actual behavior

Under certain conditions, when passing an instance of a struct to method, some of the fields of the struct are missing in the copy of the instance accessible from within the method.

Regression?

No response

Known Workarounds

No response

Configuration

.net version: 7.0.304
OS: macOS Ventura 13.4.1
Arch: x64

VS Code and Visual Studio for Mac both exhibit the same behaviour.

Other information

No response

Author: odyssjii
Assignees: -
Labels:

area-CodeGen-coreclr, area-Diagnostics-coreclr

Milestone: 8.0.0

@jakobbotsch
Copy link
Member

I'm going to move this to 9.0 and mark it as needing author action since there is nothing actionable from the codegen side at this point. Will happily take another look once a repro case that doesn't involve the debugger/stepping is provided.

@jakobbotsch jakobbotsch modified the milestones: 8.0.0, 9.0.0 Aug 9, 2023
@jakobbotsch jakobbotsch added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 9, 2023
@ghost
Copy link

ghost commented Aug 9, 2023

This issue has been marked needs-author-action and may be missing some important information.

@mikem8361
Copy link
Member

It doesn't repro if COMPlus_EnableAVX=0. If does also repro on .NET 8 even with the AVX PR ##89705 changes.

@mikem8361 mikem8361 removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 10, 2023
@jakobbotsch
Copy link
Member

jakobbotsch commented Aug 10, 2023

Ok, so the summary is that the repro case posted is a duplicate of #78991. We still need to determine whether there is a codegen issue, but we need a repro case that doesn't involve stepping or more details to do that.

@jakobbotsch jakobbotsch added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 10, 2023
@ghost
Copy link

ghost commented Aug 10, 2023

This issue has been marked needs-author-action and may be missing some important information.

@ghost ghost added the no-recent-activity label Aug 25, 2023
@ghost
Copy link

ghost commented Aug 25, 2023

This issue has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented Sep 8, 2023

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

@ghost ghost closed this as completed Sep 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 8, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Development

No branches or pull requests

8 participants