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

Changing constraint from "struct" to "unmanaged" causes stack overflow and VS crash. #31439

Closed
Korporal opened this issue Nov 29, 2018 · 36 comments
Assignees
Labels
Area-Compilers Bug Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner.
Milestone

Comments

@Korporal
Copy link

Korporal commented Nov 29, 2018

I'm working on some code in VS 2017 (v 15.9.2) using v 7.3 of C#. This is a small console app but does some low level memory manipulation. I was refactoring the code earlier and decided that a constraint could be adjusted to unmanaged rather than struct to support some richer operations.

Before this change the code compiles and runs exactly as expected. The code here is basically a generic struct where the T is itself constrained to be struct - changing this constraint to unmanaged is what leads to the failure.

I then decided to edit Program.cs in notepad just to change the constraint and then re-opened the solution in VS. I opened the solution (but not Program.cs) and ran a build, the build soon failed with stack overflow.

I then opened Program.cs again (in VS 2017) and VS simply freezes, stops interacting and responding to mouse etc. Left for long enough it abruptly dies and restarts.

I did explore the unmanaged constraint recently but in a simple test app and probably used an earlier version of C# than 7.3 - I had no issues. So I simply opened the solution and changed each project to C# 7.2 but again as soon as I opened Program.cs VS freezes, same thing happens when I try C# 7.1 and C# 7.0 too.

NOTE: This is not a runtime issue, so don't suspect that the logic itself is involved, this is an IDE/compiler issue.

Thoughts?

@Korporal Korporal changed the title Changing constraint from struct to unmanaged causes stack overflow and VS crash. Changing constraint from "struct" to "unmanaged" causes stack overflow and VS crash. Nov 29, 2018
@tannergooding
Copy link
Member

Do you have a simple repro application that we can use to validate the issue locally?

@Korporal
Copy link
Author

Korporal commented Nov 29, 2018

@tannergooding - That's a pretty fair question, the code however is proprietary and I'm unable to share it. I will however endeavor to create a simple repro, I'm sure I can incrementally copy bits n piece from the original until it happens. The "good" thing here is that it's not intermittent, very consistent.

I'm thinking the stack overflow must be a clue to what's happening - by the way, are there any special diagnostic flags I can add to the build or anything that could generate a log for you guys?

The "pattern" (if I can say that) is a generic struct X<T> where T is itself constrained to struct (making it unmanaged is where it breaks). Then there is another struct Z which contains a private member field of type X<Z>. In this particular code (a proof of concept) both these structs are in Program.cs
It's possible this is the root and the other code in there is not a factor, I'll try to look at this later but it may be possible for someone to repro it using what I describe above.

@jmarolf
Copy link
Contributor

jmarolf commented Nov 29, 2018

@Korporal You can use the Windows Event Viewer to get the callstack of the crash

@Korporal
Copy link
Author

@jmarolf - Ahh OK cool, if I get some time today I'll definitely dig this out and a possible simple repro...

@jmarolf
Copy link
Contributor

jmarolf commented Nov 29, 2018

@Korporal event past events should still be there so if you still have the machine it happened on you can get the original crash info

[jcouv deleted image as it confused multiple folks ;-)
https://user-images.githubusercontent.com/9797472/49233789-9d5ee800-f3ab-11e8-957f-1cbe8bcfbc72.png
]

@tannergooding
Copy link
Member

tannergooding commented Nov 29, 2018

Seems pretty easy to repro. The following hangs sharplab and crashes VS https://sharplab.io/#v2:EYLgHgbALAPgAgBgARwIwG4CwAoHcDMSAzgC4BOArgMYlIAaAPACoB8OSHSA7gBYCmZPkiZIQSCgDsAtgEMJMgOZ8AJjgDeOAL448hUpRpIAWuvacADmQCWANxkkhjIyyQAzK3wA2yrNm3YgA===

Using where T : struct works fine

using System;

public struct X<T>
    where T : unmanaged
{
}

public struct Z
{
    private X<Z> field;
}

@Korporal
Copy link
Author

@tannergooding - Attaboy, you got it - thanks!

@vatsalyaagrawal vatsalyaagrawal added Bug Area-IDE Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner. labels Nov 29, 2018
@vatsalyaagrawal vatsalyaagrawal added this to the 15.9 milestone Nov 29, 2018
@sharwell
Copy link
Member

@jcouv Is this related to the signature help changes we worked on recently?

@jcouv
Copy link
Member

jcouv commented Nov 29, 2018

@sharwell Yes, it is a regression caused by my recent change to signature help.
Gen fixed this issue in dev16 preview2 (PR #30997).
I'll add a test with Tanner's repro code to confirm.

@jcouv jcouv self-assigned this Nov 29, 2018
@jcouv jcouv modified the milestones: 15.9, 16.0.P2 Nov 29, 2018
@Korporal
Copy link
Author

Hi, pleased to see this is readily addressed, very good. May I ask approx when would an update containing this fix expect to be released to the public? Or if this may be several months, is there a workaround by any chance?

@jcouv
Copy link
Member

jcouv commented Nov 30, 2018

I'll first add a test to confirm this is fixed (stack trace looks like previous issue, but still good to validate with a test). I'll do that today.
Then I'll get back to you on availability or mitigation.

@jcouv jcouv modified the milestones: 16.0.P2, 15.9 Nov 30, 2018
@jcouv
Copy link
Member

jcouv commented Dec 1, 2018

Correction. This is not fixed and it is not related to the signature help work I had done recently.
Apologies for not reading the thread carefully enough. I got confused by Jon's screenshot which showed a stacktrace for signature help :-S

The issue at hand is a compiler stack overflow with unmanaged constraint.

@jaredpar Do you know how to query Watson to see the hit count on this?

        [Fact, WorkItem(31439, "https://github.com/dotnet/roslyn/issues/31439")]
        public void Test()
        {
            string source = @"
using System;

public struct X<T>
    where T : unmanaged
{
}

public struct Z
{
    private X<Z> field;
}";
            var comp = CreateCompilation(source);
            comp.VerifyDiagnostics(); // stack overflow
        }

Here's the repeating portion of the stacktrace:

FieldSymbol.Type.get()
BaseTypeAnalysis.DependsOnDefinitelyManagedType(CSharp.Symbols.NamedTypeSymbol type, HashSet<CSharp.Symbol> partialClosure)
BaseTypeAnalysis.IsManagedType(CSharp.Symbols.NamedTypeSymbol type)
NamedTypeSymbol.IsManagedType.get()
SourceMemberContainerTypeSymbol.IsManagedType.get()
TypeSymbolWithAnnotations.IsManagedType.get()
ConstraintsHelper.CheckConstraints(CSharp.Symbol containingSymbol, CSharp.ConversionsBase conversions, CSharp.Symbols.TypeMap substitution, CSharp.Symbols.TypeParameterSymbol typeParameter, CSharp.Symbols.TypeSymbolWithAnnotations typeArgument, Compilation currentCompilation, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> diagnosticsBuilder, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> warningsBuilderOpt, ref PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder, HashSet<CSharp.Symbols.TypeParameterSymbol> ignoreTypeConstraintsDependentOnTypeParametersOpt)
ConstraintsHelper.CheckConstraints(CSharp.Symbol containingSymbol, CSharp.ConversionsBase conversions, CSharp.Symbols.TypeMap substitution, System.Collections.Immutable.ImmutableArray<CSharp.Symbols.TypeParameterSymbol> typeParameters, System.Collections.Immutable.ImmutableArray<CSharp.Symbols.TypeSymbolWithAnnotations> typeArguments, Compilation currentCompilation, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> diagnosticsBuilder, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> warningsBuilderOpt, ref PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder, BitVector skipParameters, HashSet<CSharp.Symbols.TypeParameterSymbol> ignoreTypeConstraintsDependentOnTypeParametersOpt)
ConstraintsHelper.CheckTypeConstraints(CSharp.Symbols.NamedTypeSymbol type, CSharp.ConversionsBase conversions, Compilation currentCompilation, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> diagnosticsBuilder, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> warningsBuilderOpt, ref PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder)
ConstraintsHelper.CheckConstraintsForNonTuple(CSharp.Symbols.NamedTypeSymbol type, CSharp.ConversionsBase conversions, SyntaxNode typeSyntax, SeparatedSyntaxList<CSharp.Syntax.TypeSyntax> typeArgumentsSyntax, Compilation currentCompilation, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved, DiagnosticBag diagnostics)
CSharp.Binder.ConstructNamedType(CSharp.Symbols.NamedTypeSymbol type, SyntaxNode typeSyntax, SeparatedSyntaxList<CSharp.Syntax.TypeSyntax> typeArgumentsSyntax, System.Collections.Immutable.ImmutableArray<CSharp.Symbols.TypeSymbolWithAnnotations> typeArguments, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved, DiagnosticBag diagnostics)
CSharp.Binder.BindGenericSimpleNamespaceOrTypeOrAliasSymbol(CSharp.Syntax.GenericNameSyntax node, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved, CSharp.Symbols.NamespaceOrTypeSymbol qualifierOpt)
CSharp.Binder.BindNamespaceOrTypeOrAliasSymbol(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved, bool suppressUseSiteDiagnostics)
CSharp.Binder.BindTypeOrAlias(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved)
CSharp.Binder.BindType(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved)
SourceMemberFieldSymbolFromDeclarator.GetFieldType(Roslyn.Utilities.ConsList<CSharp.Symbols.FieldSymbol> fieldsBeingBound)
FieldSymbol.Type.get()
BaseTypeAnalysis.DependsOnDefinitelyManagedType(CSharp.Symbols.NamedTypeSymbol type, HashSet<CSharp.Symbol> partialClosure)

@jaredpar
Copy link
Member

jaredpar commented Dec 2, 2018

@jcouv

Do you know how to query Watson to see the hit count on this?

If we can get a bucket ID of the crash it should be possible to query this.

@sharwell
Copy link
Member

sharwell commented Dec 2, 2018

/cc @ivanbasov for help with reliability data

@ivanbasov
Copy link
Contributor

  1. I hope we already fixed it with Fix IndexOutOfRange exception in InvocationExpressionSignatureHelpProvider #30997
    @genlu please let us know if my guess is wrong.
  2. There is a watson issue: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/718094
    The issue was fixed in Dev 16 preview 1.
  3. Based on:
    a. number of hits in the watson issue
    b. version number of the PR (16 preview 1)
    I'm resolving this issue and marking it with Dev 16 Preview 1

@ivanbasov ivanbasov removed this from the 15.9 milestone Dec 3, 2018
@jcouv
Copy link
Member

jcouv commented Dec 14, 2018

@Korporal If all goes well, VS 2019 preview2 (no dates announced).

@Korporal
Copy link
Author

Korporal commented Dec 15, 2018

@jcouv - Hi and thanks. So (just curious) why can't this simply be an update to the compiler and decouple it from a Visual Studio release? I frequntly install updates as we all do and I was under the impression that the compiler is no longer tied to VS releases, indeed we can select the compiler version easily for any project. This is also a rather fundamentql bug, complete failure to process valid C# source.

Since VS 2019 appears to expected in mid 2019 this implies the bug won't have a production ready fix for us to use for another 6 months.

@jcouv
Copy link
Member

jcouv commented Dec 15, 2018

In a project, you don't select a "compiler version", you only select a "language version". There is only one version of the compiler installed with VS.

That said, Roslyn and the compiler are indeed well componentized inside VS, and it is actually possible to install Roslyn in VS as an extension.
But for long complicated reasons it is trouble to expose to end-users, because of an occasional bug in un-install scenario. We don't have enough adoption of Roslyn dailies to justify an onerous fix of that bug, and vice-versa.
The second reason is that VS and Roslyn interact via many APIs, and sometimes there are some lock-step changes required (can't use new version of Roslyn on old version of VS, or vice-versa).

This deployment/adoption/dogfooding issue is tracked by #18783

@Korporal
Copy link
Author

@jcouv Thanks for explaining this but I must admit I'm still confused! Is this bug special in some way? in the sense that a fix cannot be made and released before VS 2019? I can see many bugs (most being quite obscure) listed here but some of these will presumably be fixed in VS 2017, does my question make sense? Once again, thanks, I'm just trying to understand the situation.

@jcouv
Copy link
Member

jcouv commented Dec 16, 2018

Every bug is triaged and prioritized based on impact (how common is it, how bad is the experience), risk of change, and some other factors (regression, compatibility, etc). The bar for fixing issues in VS 2017 is very high at this point, and this bug does not meet that bar.

@Korporal
Copy link
Author

Thanks, that explains things, much appreciated.

@jcouv
Copy link
Member

jcouv commented Jan 16, 2019

Moved back to preview3. Check with Jared and move out again if ok. Thanks

@RikkiGibson
Copy link
Contributor

@jcouv @jaredpar do you want this fix to go in at the same time as unmanaged constructed types, or to extract the specific change that fixes this scenario to its own PR for preview3?

RikkiGibson added a commit to RikkiGibson/roslyn that referenced this issue Jan 16, 2019
RikkiGibson added a commit that referenced this issue Jan 17, 2019
* Add regression test for issue #31439

* Delay constraint checking on field types until after type members are added

* Fix tests

* Fix WithNullability for field type constraint checks

* Use SyntaxTree property instead of TypeSyntax.SyntaxTree

* Fix up more expected diagnostics
@Korporal
Copy link
Author

Korporal commented Mar 31, 2019

@RikkiGibson - Hi, is this fixed in the C# 8.0 beta that's part of VS 2019 16.0.0 preview 4.4? Attempting to compile this generates a compilation error (the crash has gone but it seems there are still issues):

namespace ConsoleApp3
{
    class Program
    {
        static void Main(string[] args)
        {
        }
    }

    public struct X<T> where T : unmanaged
    {

    }

    public struct Z
    {
        private X<Z> field;
    }
}

@jmarolf
Copy link
Contributor

jmarolf commented Mar 31, 2019

@Korporal this appears to work in master

@Korporal
Copy link
Author

Korporal commented Apr 1, 2019

@jmarolf - Hi, I'm not familair with sharplab, looks pretty useful but each time I visit I see a strange error messages at the top of the page:

[connection lost, reconnecting…]

and I have no idea what that means, the left hand side edit window becomes greyed out too.

@RikkiGibson
Copy link
Contributor

It can be finicky occasionally.. I suggest just refreshing, maybe clearing caches and stuff, and report it in https://github.com/ashmind/sharplab if it keeps happening.

@Korporal
Copy link
Author

Korporal commented Apr 5, 2019

@RikkiGibson @jmarolf @jcouv - Hi. There's is considerable confusion here - I cannot get an answer from anyone in the csharplang repo as to whether the code fragment should actually compile (is legal C#) or if it should fail to compile and if it should fail then the error message:

The type 'Z' must be a non-nullable value type, along with all fields at any level of nesting, in order to use it as parameter 'T' in the generic type or method 'X'

is an appropriate error message.

See this issue which is a recent history of the discussion with the csharplang techies.

Is there anyone who knows enough about the language and its formal specification who can give a definitive yes/no answer? My own understanding is that the code should compile when the unmanaged constraint is used just as it does when the struct constraint is used.

Surely this is a simple matter of examining the formal language specification? after all a fix was made by @RikkiGibson (as stated in this issue thread) so that fix must have been to make the compiler align with said specification, yes?

Thanks!

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Apr 12, 2019

Apologies for the delay.

I suspect the hang you're experiencing in SharpLab is due to using the 2.9.0 compiler instead of the master compiler (select 'master' from the dropdown list). Sometimes when the compiler stack overflows on the server side, SharpLab will just hang.

Now, regarding the sample:

public struct X<T> where T : unmanaged
{
}

public struct Z
{
    private X<Z> field; // error in 7.3: feature 'unmanaged constructed types' requires C# 8.0
}

This is not valid in C# 7.3 and earlier. The reason is that Z is considered 'managed' by that language version due to containing a field of a constructed type (namely X<Z>.)

However, this is valid in C# 8. The unmanaged constructed types feature (see dotnet/csharplang#1744) relaxes the rules to allow types such as Z to be considered unmanaged as long as the types of all fields are unmanaged.

@tannergooding
Copy link
Member

It is valid in the current master bits and can be tested by pulling down a nightly build of the compiler. For example: https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEDMACWCCuBjGDADQB4AVAPgwHcALRODMjEDHAO0nYgHM4ATALAAoAN4iAviJHosMXAQwAtEeOEYNGAA4IAlgDcI8YiSVUAZrrgAbfgG4MAekcZECAPYIMu9hgDsAHRorOZwRjgIjADkHFy8Ahh47uzY+PD8GDAAnlpwUFEYkQCOOLqRUBgAwgDEGAAcAQAMkkA===

C# 8 is not RTM and the compiler that shipped in VS2019 only provides C# 8 in a preview form (LangVersion=latest still returns 7.3; and you have to explicit opt into C# 8 using LangVersion=8 or LangVersion=preview).

xoofx pushed a commit to stark-lang/stark-roslyn that referenced this issue Apr 16, 2019
xoofx pushed a commit to stark-lang/stark-roslyn that referenced this issue Apr 16, 2019
…et#32536)

* Add regression test for issue dotnet#31439

* Delay constraint checking on field types until after type members are added

* Fix tests

* Fix WithNullability for field type constraint checks

* Use SyntaxTree property instead of TypeSyntax.SyntaxTree

* Fix up more expected diagnostics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner.
Projects
None yet
Development

No branches or pull requests

9 participants