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

stackalloc should be disallowed for structs with ref fields #63104

Closed
jkotas opened this issue Jul 31, 2022 · 6 comments · Fixed by #63209
Closed

stackalloc should be disallowed for structs with ref fields #63104

jkotas opened this issue Jul 31, 2022 · 6 comments · Fixed by #63209

Comments

@jkotas
Copy link
Member

jkotas commented Jul 31, 2022

Version Used: Microsoft.Net.Compilers.Toolset 4.4.0-2.22379.18 (current daily build)

Steps to Reproduce:

  1. Build
using System;
using System.Reflection;
using System.Runtime.InteropServices;

unsafe
{
    StructWithRefField* p = stackalloc StructWithRefField[10];

    MyClass o = new();    
    // This has GC hole. The stackallocated array of StructWithRefField is not reported to GC
    p[0].RefField = ref o.ByteField;
}

ref struct StructWithRefField
{
    public ref byte RefField;
}

class MyClass
{
    public byte ByteField;
}

sharplab is crashing on this repro.

Expected Behavior:

Build error. stackalloc should be disallowed for structs with ref fields.

Actual Behavior:

Builds successfully, the program has GC hole at runtime.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 31, 2022
@cston cston added this to the 17.4 milestone Aug 1, 2022
@jcouv jcouv added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 2, 2022
@jcouv
Copy link
Member

jcouv commented Aug 2, 2022

Assigned to @jaredpar to adjust language rules/spec

@cston
Copy link
Member

cston commented Aug 2, 2022

The spec includes the following:

  • A ref struct which has a ref field is never considered unmanaged

This was missed in the implementation.

@cston
Copy link
Member

cston commented Aug 12, 2022

Reopening since the change has been reverted - see #63367.

@cston cston reopened this Aug 12, 2022
@MichalPetryka
Copy link

Is this issue maybe a duplicate of #62528?

@cston
Copy link
Member

cston commented Aug 15, 2022

@MichalPetryka, this appears to be a distinct issue from #62528, although we should test both scenarios with any potential fix. Thanks.

@jcouv
Copy link
Member

jcouv commented Sep 23, 2022

Fixed in #64064

@jcouv jcouv closed this as completed Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants