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

NativeAOT crash with ConcurrentBag and interfaces #95574

Closed
vaind opened this issue Dec 4, 2023 · 8 comments · Fixed by #95683
Closed

NativeAOT crash with ConcurrentBag and interfaces #95574

vaind opened this issue Dec 4, 2023 · 8 comments · Fixed by #95683

Comments

@vaind
Copy link

vaind commented Dec 4, 2023

Description

A NativeAOT app crashes on converting a ConcurrentBag to a ReadOnlyCollection and then to array, when interfaces are involved in generic types.

Reproduction Steps

  1. dotnet new ios
  2. Update .csproj to include <PublishAot>true</PublishAot>
  3. Add the following code to FinishedLaunching(), e.g. just before the return true:
    var bag = new System.Collections.Concurrent.ConcurrentBag<MyClass>();
    IReadOnlyCollection<IMyInterface> collection = bag;
    var array = collection.ToArray();
    And define these types, e.g. at the end of the same file:
    public class MyClass : IMyInterface {}
    public interface IMyInterface {}
  4. Run the published app on an iPhone:
    dotnet publish -c Release -f net8.0-ios
    ios-deploy -b bin/Release/net8.0-ios/ios-arm64/publish/*.ipa
    

Expected behavior

Don't crash

Actual behavior

App crashes after startup

Regression?

It works fine without NativeAOT.

Known Workarounds

none

Configuration

  • .NET 8.0.100
  • iPhone X v14.5

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 4, 2023
@ghost
Copy link

ghost commented Dec 4, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

A NativeAOT app crashes on converting a ConcurrentBag to a ReadOnlyCollection and then to array, when interfaces are involved in generic types.

Reproduction Steps

  1. dotnet new ios
  2. Update .csproj to include <PublishAot>true</PublishAot>
  3. Add the following code to FinishedLaunching(), e.g. just before the return true:

var bag = new System.Collections.Concurrent.ConcurrentBag();
IReadOnlyCollection collection = bag;
var array = collection.ToArray();
And define these types, e.g. at the end of the same file:c#
public class MyClass : IMyInterface {}
public interface IMyInterface {}

4. Run the published app on an iPhone: dotnet publish -c Release -f net8.0-ios ; ios-deploy -b bin/Release/net8.0-ios/ios-arm64/publish/*.ipa

### Expected behavior

Don't crash

### Actual behavior

App crashes after startup

### Regression?

It works fine without NativeAOT.

### Known Workarounds

none

### Configuration

* .NET 8.0.100
* iPhone X v14.5

### Other information

_No response_

<table>
<tr>
 <th align="left">Author:</th>
 <td>vaind</td>
</tr>
<tr>
 <th align="left">Assignees:</th>
 <td>-</td>
</tr>
<tr>
 <th align="left">Labels:</th>
 <td>

`area-NativeAOT-coreclr`

</td>
</tr>
<tr>
 <th align="left">Milestone:</th>
 <td>-</td>
</tr>
</table>
</details>

@vitek-karas
Copy link
Member

This reproes on Windows as well - same as above.
It's a "Fail fast" at this callstack:

 	KernelBase.dll!00007ff8f26bc3b2()	Unknown
 	95574.exe!S_P_CoreLib_System_Runtime_CachedInterfaceDispatch__RhpCidResolve_Worker() Line 48	Unknown
 	95574.exe!S_P_CoreLib_System_Runtime_CachedInterfaceDispatch__RhpCidResolve() Line 22	Unknown
 	95574.exe!RhpUniversalTransition_DebugStepTailCall() Line 163	Unknown
 	95574.exe!System_Linq_System_Collections_Generic_LargeArrayBuilder_1<System___Canon>__AddRange() Line 104	Unknown
 	95574.exe!System_Linq_System_Collections_Generic_EnumerableHelpers__ToArray_0<System___Canon>() Line 85	Unknown
>	95574.exe!_95574_Program___Main__() Line 6	Unknown
 	95574.exe!_95574__Module___StartupCodeMain()	Unknown
 	95574.exe!wmain(int argc, wchar_t * * argv) Line 224	C++

The fail fast is here:

EH.FallbackFailFast(RhFailFastReason.InternalError, null);

@am11 am11 added the bug label Dec 4, 2023
@am11
Copy link
Member

am11 commented Dec 4, 2023

Workaround is to add something to the bag:

var bag = new System.Collections.Concurrent.ConcurrentBag<MyClass>();
+ bag.Add(new()); // empty bag will hit the bug https://github.com/dotnet/runtime/issues/95574
IReadOnlyCollection<IMyInterface> collection = bag;
var array = collection.ToArray();

@vaind
Copy link
Author

vaind commented Dec 4, 2023

Thanks for trying @am11 but that is not an acceptable workaround because it's a functional change. But I guess it helps to devise one - checking for an empty bag before doing the conversion.

@am11
Copy link
Member

am11 commented Dec 4, 2023

@vaind, it is definitely a bug (and labeled as such). My comment was mostly from testing perspective that having a non-null item in the bag does not trigger this bug; so the bag has to be empty or with null items. The tests that will go with the fix need to take this into consideration. :)

@lambdageek lambdageek added the os-ios Apple iOS label Dec 4, 2023
@ghost
Copy link

ghost commented Dec 4, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

A NativeAOT app crashes on converting a ConcurrentBag to a ReadOnlyCollection and then to array, when interfaces are involved in generic types.

Reproduction Steps

  1. dotnet new ios
  2. Update .csproj to include <PublishAot>true</PublishAot>
  3. Add the following code to FinishedLaunching(), e.g. just before the return true:
    var bag = new System.Collections.Concurrent.ConcurrentBag<MyClass>();
    IReadOnlyCollection<IMyInterface> collection = bag;
    var array = collection.ToArray();
    And define these types, e.g. at the end of the same file:
    public class MyClass : IMyInterface {}
    public interface IMyInterface {}
  4. Run the published app on an iPhone:
    dotnet publish -c Release -f net8.0-ios
    ios-deploy -b bin/Release/net8.0-ios/ios-arm64/publish/*.ipa
    

Expected behavior

Don't crash

Actual behavior

App crashes after startup

Regression?

It works fine without NativeAOT.

Known Workarounds

none

Configuration

  • .NET 8.0.100
  • iPhone X v14.5

Other information

No response

Author: vaind
Assignees: -
Labels:

bug, untriaged, os-ios, area-NativeAOT-coreclr

Milestone: -

@lambdageek
Copy link
Member

lambdageek commented Dec 4, 2023

/cc FYI @ivanpovazan

Update removing ios label since it repros on Windows, too

@lambdageek lambdageek removed the os-ios Apple iOS label Dec 4, 2023
@MichalStrehovsky MichalStrehovsky added this to the 9.0.0 milestone Dec 5, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Dec 5, 2023
@MichalStrehovsky
Copy link
Member

Nice bug! It has been in the product for a while, probably years (we shipped with this in .NET 7 but nobody hit/reported this).

The necessary condition to hit this is to not have MyClass allocated anywhere in the program. What happens is that MyClass had its interface list optimized away and ConcurrentBag<MyClass> no longer casts to IEnumerable<IMyInterface> at runtime.

@vaind vaind changed the title iOS NativeAOT crash NativeAOT crash with ConcurrentBag and interfaces Dec 5, 2023
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this issue Dec 6, 2023
The compiler can generate two kinds of `MethodTable` structures: constructed and unconstructed. The constructed one has a fully populated vtable and GCInfo and is required whenever the object type could be allocated on the heap. The unconstructed one is generated for all other scenarios as a size optimization.

We were previously also skipping emission of the interface list in the unconstructed case. But interface list might be required for variant casting. We could introduce yet another `MethodTable` kind for this specific scenario, but it doesn't seem to warrant the complexity.

Emitting interface list for all types is a less than 0.1% size regression for the Todos app. It is a 0.5% size regression for Hello World. That part is unfortunate. It's mostly due to the useless numeric interfaces. We can get this size back if we do dotnet#66716 and start trimming interface lists.

Fixes dotnet#95574.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 6, 2023
jkotas pushed a commit that referenced this issue Dec 8, 2023
* Generate interface lists for necessary EETypes

The compiler can generate two kinds of `MethodTable` structures: constructed and unconstructed. The constructed one has a fully populated vtable and GCInfo and is required whenever the object type could be allocated on the heap. The unconstructed one is generated for all other scenarios as a size optimization.

We were previously also skipping emission of the interface list in the unconstructed case. But interface list might be required for variant casting. We could introduce yet another `MethodTable` kind for this specific scenario, but it doesn't seem to warrant the complexity.

Emitting interface list for all types is a less than 0.1% size regression for the Todos app. It is a 0.5% size regression for Hello World. That part is unfortunate. It's mostly due to the useless numeric interfaces. We can get this size back if we do #66716 and start trimming interface lists.

Fixes #95574.

* Update NecessaryCanonicalEETypeNode.cs
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants