Skip to content

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Sep 15, 2022

The first commit reverts commit 76bd0d9.

Spec update for the relaxation: dotnet/csharplang#6453
Some context on EE logic: 02a3854

Note: I've built the runtime libraries locally with this change and some suppressions (as expected).

@jcouv jcouv self-assigned this Sep 15, 2022
@ghost ghost added the Area-Compilers label Sep 15, 2022
@jcouv jcouv marked this pull request as ready for review September 16, 2022 22:24
@jcouv jcouv requested a review from a team as a code owner September 16, 2022 22:24
@RikkiGibson RikkiGibson self-assigned this Sep 19, 2022
Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

approving so we can ship .NET 7. Not thrilled about being able to do var ptr = &managed; but not Managed* ptr = &managed; :)

InExpressionTree = 1 << 30,

/// <summary>
/// The current context allows pointer types to managed types.
Copy link
Member

@RikkiGibson RikkiGibson Sep 19, 2022

Choose a reason for hiding this comment

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

consider indicating that UnsafeRegion is also needed, since absence of that flag means no pointer types are allowed. #Resolved

@jcouv jcouv requested a review from cston September 19, 2022 22:30
// var x = &s; // 1
Diagnostic(ErrorCode.WRN_ManagedAddr, "&s").WithArguments("S2").WithLocation(11, 17)
);
var verifier = CompileAndVerify(comp, verify: Verification.Skipped);
Copy link
Contributor

@cston cston Sep 19, 2022

Choose a reason for hiding this comment

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

);

Can we run this code (using IncludeExpectedOutput(...) to run conditionally)?

Same question for the next several tests. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't yet found out which unsafe operations are in fact safe from the JIT's perspective...
Since the IL is small and straightforward, I think it's sufficient.

{
unsafe void M()
{
var x1 = stackalloc S[0]; // 1
Copy link
Contributor

@cston cston Sep 19, 2022

Choose a reason for hiding this comment

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

// 1

Consider removing comments // 1 and // 4. #Resolved

// C** y3 = stackalloc C*[0]; // 4
Diagnostic(ErrorCode.ERR_ManagedAddr, "C*").WithArguments("C").WithLocation(21, 29)
);
}
Copy link
Contributor

@cston cston Sep 19, 2022

Choose a reason for hiding this comment

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

}

Are we testing implicit stack alloc?

ref struct R { ref int F; }
var a = stackalloc[] { new R() };
``` #Resolved

IL_0003: ret
}
");
}
Copy link
Contributor

@cston cston Sep 20, 2022

Choose a reason for hiding this comment

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

}

Are we testing assignment?

ref struct R { ref int F; }
unsafe static void Assign(IntPtr* p, ref R r)
{
    *(R*)p = r;
}
``` #Resolved

@Neme12
Copy link
Contributor

Neme12 commented Sep 13, 2023

Given the change that pointers to managed structs are now allowed and considered a warning instead of an error, could someone pls explain why the warning exists - i.e. why are pointers to managed structs considered unsafe (or "unsafe" unsafe) and why was it originally an error? What issues can I run into if I use pointers to managed structs?

There is no documentation for the new warning and I can't find any info on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants