Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Reject struct promotion of parameters when -GS checks are enabled #17329

Merged
merged 1 commit into from
Mar 31, 2018

Conversation

briansull
Copy link

This causes bad codegen when we introduce shadow copies for them.

@briansull
Copy link
Author

Introduced with #16220

@briansull
Copy link
Author

@AndyAyersMS PTAL
@dotnet/jit-contrib PTAL

@briansull
Copy link
Author

I will try to work up a repro case for this issue.

@AndyAyersMS
Copy link
Member

Can you verify we still get the same codegen for the span-stackalloc cases in #16197?

Seeing running diffs would be valuable. Likely there should be no diffs in core and framework assemblies. Perhaps a few in tests if we have some IL tests that have unsafe value classes.

as we could introduce shadow copies of them.
Add new test case: GitHub_17329.cs
@briansull briansull force-pushed the struct-gscheck-fix branch from c4fff19 to 6b07baf Compare March 31, 2018 01:27
@briansull
Copy link
Author

briansull commented Mar 31, 2018

I saw a handful of diffs on the desktop. '
It loosk like IL_STUB's sometimes have an unsafe buffer.
The diffs I saw were typically just in the locals variable names used in the header.
Often the generated code was exactly the same.

@briansull
Copy link
Author

briansull commented Mar 31, 2018

One such diff occurs in
; Assembly listing for method System.Reflection.Associates:AssignAssociates(struct,int,ref,ref,byref,byref,byref,byref,byref,byref,byref,byref)

where local 4 is an unsafe-buffer
; V15 loc3 [V15 ] ( 8, 11.24) struct (80) [rsp+0x20] do-not-enreg[XSF] must-init addr-exposed ld-addr-op ptr unsafe-buffer

That struct is a MetadataEnumResult

which is defined:

internal unsafe struct MetadataEnumResult

So it looks like there are two ways to mark a struct as being an unsafe-buffer

Either use the full Attribute name like I did in my test case:
[UnsafeValueTypeAttribute]
struct DangerousBuffer

or by simply using the C# keyword unsafe in your struct definition as is done for MetadataEnumResult:

internal unsafe struct MetadataEnumResult

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

@briansull briansull merged commit 42ae2c0 into dotnet:master Mar 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants