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

Allow stack-only types in stackalloc #33951

Merged
merged 3 commits into from Mar 15, 2019

Conversation

@jcouv
Copy link
Member

jcouv commented Mar 7, 2019

Customer scenario

In 16.0 preview3, we accidentally disallowed stack-only types in stackalloc. This PR restores the previous behavior.

Bugs this fixes

Fixes #33945

Workarounds, if any

None

Risk, Performance impact

Low. We're just skipping an unnecessary check for stackallocated arrays.

Is this a regression from a previous update?

Yes, this bug was introduced in 16.0 preview3.

Root cause analysis

While fixing an issue related to arrays, we factored some code too aggressively. The check that prevents restricted types in array types started applying to stackalloc arrays too.

How was the bug found?

Reported by customer.


Filled https://devdiv.visualstudio.com/DevDiv/_workitems/edit/815087/ for ask-mode approval.

@jcouv jcouv added the Area-Compilers label Mar 7, 2019

@jcouv jcouv added this to the 16.0 milestone Mar 7, 2019

@jcouv jcouv self-assigned this Mar 7, 2019

@jcouv jcouv requested a review from dotnet/roslyn-compiler as a code owner Mar 7, 2019

_ = z2;
_ = z3;
_ = z4;
}

This comment has been minimized.

@jaredpar

jaredpar Mar 7, 2019

Member

Suggestion for other tests here:

ref struct RefG<T> { public T field; } 

Then add the following cases into the matrix

RefG<int>
RefG<string>
``` #Closed
// var x2 = stackalloc RefG<string>[10];
Diagnostic(ErrorCode.ERR_ManagedAddr, "RefG<string>").WithArguments("RefG<string>").WithLocation(10, 29),
// (11,29): error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('RefG<int>')
// var x3 = stackalloc RefG<int>[10];

This comment has been minimized.

@jaredpar

jaredpar Mar 7, 2019

Member

Heads up @JoeRobich, @agocke: this will cause a test failure when we merge to master as this will stop giving an error.

This comment has been minimized.

@jcouv

jcouv Mar 19, 2019

Author Member

Here's the commit to resolve that conflict in merge to master:
7fa3083

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Mar 7, 2019

@dotnet/roslyn-compiler for second review for 16.0. Thanks

@cston

cston approved these changes Mar 7, 2019

@agocke

agocke approved these changes Mar 7, 2019

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Mar 7, 2019

var x1 = stackalloc RefS[10];
var x2 = stackalloc RefG<string>[10];
var x3 = stackalloc RefG<int>[10];
var x4 = stackalloc System.TypedReference[10];

This comment has been minimized.

@VSadov

VSadov Mar 7, 2019

Member

TypedReference has a byref field in it (logically, similar to Span<T>). I do not think stackallocs are reported to GC.
It could result in a GC hole.

This comment has been minimized.

@VSadov

VSadov Mar 7, 2019

Member

ArgIterator is probably ok. Also has a ref, but I think it always points to unmovable locations.

This comment has been minimized.

@VSadov

VSadov Mar 7, 2019

Member

I wonder what happens with Span<int> - do we get an error because it is not unmanaged? #Resolved

This comment has been minimized.

@jcouv

jcouv Mar 7, 2019

Author Member

Correct. This will change once this gets merged to master (which has the "unmanaged generic struct" feature). Jared pointed out that the test expectations will need updated during the merge.


In reply to: 263615857 [](ancestors = 263615857)

This comment has been minimized.

@VSadov

VSadov Mar 7, 2019

Member

Just being ref-like should not prevent stackalloc, but some of them are not technically "unmanaged" since they may contain embedded references to movable heap. Are we able to sort out such distinction by looking at metadata?

This comment has been minimized.

@VSadov

VSadov Mar 8, 2019

Member

Interestingly we currently explicitly assume that TypedReference is unmanaged. For the purpose of taking a pointer to a variable of such type it seems ok. - They cannot be on the heap, so cannot move...

We may need a slightly different criteria for stackalloc, since now the concern is what variables themselves may refer to.
It might be a problem only for a few "magic" types.

This comment has been minimized.

@jcouv

jcouv Mar 8, 2019

Author Member

From discussion with Jared, Andy and David: you should be able to stackalloc any unmanaged type. (matches spec)
Span is managed, because it has a dummy object field in the various ref assemblies.
TypedReference currently does not have such a dummy object field. Filed corefx issue dotnet/corefx#35873


In reply to: 263623732 [](ancestors = 263623732)

This comment has been minimized.

@VSadov

VSadov Mar 8, 2019

Member

I think the unmanagedness of TypedReference is hardcoded in the compiler.

TypedReference* has always been legal. If corefx adds a dummy object field and if compiler starts looking at it, you will break that.

@agocke
Copy link
Contributor

agocke left a comment

Vlad's comments caused me concern. Did TypedReference work in previous builds?

@agocke

agocke approved these changes Mar 8, 2019

Copy link
Contributor

agocke left a comment

Sounds like CoreFX will mark TypedReference as having an object reference on their end to prevent this in the future

LGTM

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Mar 8, 2019

@agocke Yes, the old compiler (16.0 preview2 or older) does allow stackalloc TypedReference. From our discussion, this is an issue with the definition of that type.

Good to approve this PR? Never mind, race condition ;-) Thanks

@jcouv jcouv force-pushed the jcouv:stackalloc-bug branch from 59c7cd5 to c2fb53f Mar 8, 2019

@VSadov

This comment has been minimized.

Copy link
Member

VSadov commented Mar 8, 2019

Since we allowed stackalloc with TypedReference before, then LGTM.

However we should consider disallowing it.

And we may need to do it explicitly. Adding a field on FX side, does not seem like a solution since:

  1. we do not look inside TypedReference (and couple other types)
  2. we treat it as unmanaged for the purposes of taking a pointer. Changing that would be breaking.
@VSadov

VSadov approved these changes Mar 8, 2019

Copy link
Member

VSadov left a comment

LGTM.
consider a tracking issue for specialcasing TypedReference and the like

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Mar 8, 2019

Filed rolsyn issue as well: #33965

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Mar 15, 2019

@jinujoseph @jaredpar I see that Doug marked the corresponding VSTS issue as "Division Triage: Approved".

Just want to confirm I'm ok to merge this and other issue in same situation (#34016) into dev16.0 branch. I'll wait for one of you to update the yellow label on both issues. Thanks

@jcouv jcouv modified the milestones: 16.0, 16.0.P4 Mar 15, 2019

@jcouv jcouv merged commit 6d65656 into dotnet:dev16.0 Mar 15, 2019

1 check passed

license/cla All CLA requirements met.
Details

@jcouv jcouv deleted the jcouv:stackalloc-bug branch Mar 15, 2019

@jcouv jcouv referenced this pull request Mar 19, 2019

Merged

Merge dev16.0 to master #34146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.