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] Objective-C exception propagation #80334

Merged

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Jan 8, 2023

Related issue: #77472

This is the final part of the Objective-C Marshal API: the ability to turn a managed exception unwinding across a reverse-pinvoke boundary into a native exception.

The pieces to support this include:

  • If the first pass of exception unwinding hits a reverse-P/Invoke boundary, call a new classlib function to get the native callback.
  • After performing the second pass of unwinding, use a new RhpCallManagedToNativeExceptionCallback assembly helper to restore the context and jump to the native callback. This helper is based off of RhpCallCatchFunclet, with the calling of catch funclet and thread-abort handling removed.

Issues that need to be resolved before merging

  • add AMD64 version of the assembly helper
  • Figure what to do about ThreadAbortException, which will be swallowed if it goes through this code path. Perhaps change RhpPInvokeReturn to check for pending thread aborts? Or maybe thread aborts don't matter since RhpInitiateThreadAbort is not called by anything?
  • Simplify the prolog of the ARM64 assembly helper, since it does not need to compatible with StackFrameIterator::UnwindFuncletInvokeThunk.
  • Debug why AMD64 crashes.
  • Unwind to the correct frame.
  • Fix Windows build
  • Make StackFrameIterator support unwinding into a least one native frame
  • Create a tracking issue for implement the lastMethod parameter and update comments to reference it: [NativeAot] Objective-C Marshal: find method handle when propagating exceptions #80985
  • When an unhandled exception hits a reverse P/Invoke function on Windows, fix backtrace from RaiseFailFastException. Moved to [NativeAOT] correctly initalize CONTEXT before failing fast #81010.
  • Switch back to preemptive mode before jumping to exception propagation function.

Testing done

Built and tested with these commands for ARM64. Similar invocations for testing with X64.

./build.sh -ninja -s clr.aot+libs /p:TestNativeAot=true -c Release
./src/tests/build.sh -nativeaot -ninja Release -tree:Interop/ObjectiveC/ObjectiveCMarshalAPI
export CLRCustomTestLauncher=`pwd`/src/tests/Common/scripts/nativeaottest.sh
export CORE_ROOT=`pwd`/artifacts/tests/coreclr/OSX.arm64.Release/Tests/Core_Root
./artifacts/tests/coreclr/OSX.arm64.Release/Interop/ObjectiveC/ObjectiveCMarshalAPI/ObjectiveCMarshalAPI.sh

The test passes, which means the C++ exception dispatch code is able to unwind the stack after the new assembly helper restored the context. I added some extra asserts to check that the second pass unwind is done and that a value in the frame being restored is not clobbered after execution resumes. Specifically the assert(e == a) assert loads a from the stack in debug and the caller-saved x19 register in release. After jumping from the assembly helper to the native method that throws, the stack trace makes sense in the debugger (looks like CallAndCatch called ThrowInt or ThrowException).

I also tested this program continues to fail fast as expected on Windows:

using System;
using System.Runtime.InteropServices;

static class Program
{
    [UnmanagedCallersOnly]
    static void MyUnmanagedCallersOnlyFunction()
    {
        throw new Exception();
    }

    unsafe static void Main()
    {
        delegate* unmanaged<void> del = &MyUnmanagedCallersOnlyFunction;
        del();
    }
}

Known limitations

The prolog of the AMD64 assembly helper could preserve fewer registers.

The callback for exception unwinding expects a RuntimeMethodHandle that represents the managed method that was called via reverse P/Invoke. I tried doing this to convert the instruction pointer into a `RuntimeMethodHandle:

MethodBase? method = new StackFrame(ip, needFileInfo: false).GetMethod();
RuntimeMethodInfo? methodInfo = method as RuntimeMethodInfo;
RuntimeMethodHandle lastMethod = methodInfo?.MethodHandle ?? default;

For all the cases in the unit test, the StackFrame is unable to find the method object. I have not looked at this closely, but I believe this is because unmanaged-callers-only function and reverse-p/invoke stubs do not show up in the reflection invoke map. See this part of ILC for some possibly related UCO logic.

Since the code to lookup the RuntimeMethodHandle didn't work and Xamarin does not even use this value, I decided to remove the code and always set the RuntimeMethodHandle to 0.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 8, 2023
@ghost
Copy link

ghost commented Jan 8, 2023

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

Issue Details

Related issue: #77472

This is the final part of the Objective-C Marshal API: the ability to turn a managed exception unwinding across a reverse-pinvoke boundary into a native exception.

The pieces to support this include:

  • If the first pass of exception unwinding hits a reverse-P/Invoke boundary, call a new classlib function to get the native callback.
  • After performing the second pass of unwinding, use a new RhpCallManagedToNativeExceptionCallback assembly helper to restore the context and jump to the native callback. This helper is based off of RhpCallCatchFunclet, with the calling of catch funclet and thread-abort handling removed.

Issues that need to be resolved before merging

  • add AMD64 version of the assembly helper
  • Figure what to do about ThreadAbortException, which will be swallowed if it goes through this code path. Perhaps change RhpPInvokeReturn to check for pending thread aborts? Or maybe thread aborts don't matter since RhpInitiateThreadAbort is not called by anything?
  • Simplify the prolog of the ARM64 assembly helper, since it does not need to compatible with StackFrameIterator::UnwindFuncletInvokeThunk.

Known limitations

The callback for exception unwinding expects a RuntimeMethodHandle that represents the managed method that was called via reverse P/Invoke. I tried doing this to convert the instruction pointer into a `RuntimeMethodHandle:

MethodBase? method = new StackFrame(ip, needFileInfo: false).GetMethod();
RuntimeMethodInfo? methodInfo = method as RuntimeMethodInfo;
RuntimeMethodHandle lastMethod = methodInfo?.MethodHandle ?? default;

For all the cases in the unit test, the StackFrame is unable to find the method object. I have not looked at this closely, but I believe this is because unmanaged-callers-only function and reverse-p/invoke stubs do not show up in the reflection invoke map. See this part of ILC for some possibly related UCO logic.

Since the code to lookup the RuntimeMethodHandle didn't work and Xamarin does not even use this value, I decided to remove the code and always set the RuntimeMethodHandle to 0.

Author: AustinWise
Assignees: -
Labels:

community-contribution, area-NativeAOT-coreclr

Milestone: -

@AustinWise
Copy link
Contributor Author

@jankots
Does this general approach seem reasonable? If so, I'll add an AMD64 version of the assembly helper.

@MichalStrehovsky
Copy link
Member

The callback for exception unwinding expects a RuntimeMethodHandle that represents the managed method that was called via reverse P/Invoke. I tried doing this to convert the instruction pointer into a `RuntimeMethodHandle:

It is not guaranteed that we'll be able to produce a RuntimeMethodHandle for an arbitrary function pointer.

One thing is that they need to go to the mapping table, which currently skips them. That should not be hard to fix (we just need to do the right thing when attempting to reflection-invoke this, which is why it's blocked - the right thing being "whatever CoreCLR does").

The other thing is that the compiler will only generate these mapping table entries for methods that are visible targets of reflection. We don't have the names/signatures of methods that are not visible targets of reflection. If this is only needed for UnmanagedCallersOnly methods, you could make them visible targets of reflection by making UnmanagedCallersOnly on apple platforms take codepath around here, I think:

// Presence of code might trigger the reflectability dependencies.
if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0)

It comes with size costs. It would be preferable if we don't need it.

@jkotas
Copy link
Member

jkotas commented Jan 9, 2023

Does this general approach seem reasonable?

Yes, the approach looks reasonable.

Figure what to do about ThreadAbortException

You do not need to worry about ThreadAbortException. It is not supported.

The callback for exception unwinding expects a RuntimeMethodHandle

Xamarion iOS callback does not look at the RuntimeMethodHandle today: https://github.com/xamarin/xamarin-macios/blob/6e1ba31bae568ec54f53259e9e884f0550cfdae3/src/ObjCRuntime/Runtime.CoreCLR.cs#L144-L149 . It converts exceptions for all reverse PInvokes even the ones that have nothing to do with ObjectiveC interop (it is less than ideal). It should be ok to pass in default(RuntimeMethodHandle) here unless Xamarin iOS implementation changes.

@jkotas
Copy link
Member

jkotas commented Jan 9, 2023

It should be ok to pass in default(RuntimeMethodHandle) here unless Xamarin iOS implementation changes.

We may want to consider introducing declarative scheme for this (mark all methods that want to participate in ObjectiveC interop with an attribute) to make this more pay-for-play and AOT friendly.

@AustinWise AustinWise force-pushed the austin/NativeAotObjectiveCMarshal-Exception branch from 756568f to 5103dc6 Compare January 14, 2023 19:42
@AustinWise

This comment was marked as resolved.

@AustinWise AustinWise force-pushed the austin/NativeAotObjectiveCMarshal-Exception branch from 9c8fac1 to 8758680 Compare January 16, 2023 00:41
@AustinWise
Copy link
Contributor Author

The test now passes on X64 too, after applying the same 8 byte offset to RSP that CoreCLR does:

https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/exceptionhandling.cpp#L4343-L4346

@AustinWise
Copy link
Contributor Author

One thing I'm not sure is right is the back trace looks like this after jumping to ThrowInt in the test:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
  * frame #0: 0x0000000102018a6c libNativeObjCMarshalTests.dylib`(anonymous namespace)::ThrowInt(cxt=-121) at NativeObjCMarshalTests.cpp:104:32
    frame #1: 0x00000001002f98ac ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program__UCO_ThrowIntException + 76
    frame #2: 0x0000000102018b08 libNativeObjCMarshalTests.dylib`::CallAndCatch(fptr=(ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program__UCO_ThrowIntException), a=3423) at NativeObjCMarshalTests.cpp:131:9
    frame #3: 0x00000001002f903a ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_NativeObjCMarshalTests__CallAndCatch + 106
    frame #4: 0x00000001002f9d97 ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program___Validate_ExceptionPropagation + 551
    frame #5: 0x00000001002f9842 ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program__Validate_ReferenceTracking_Scenario + 882
    frame #6: 0x00000001002f9ef5 ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program__Main + 53
    frame #7: 0x00000001003ad1ff ObjectiveCMarshalAPI`ObjectiveCMarshalAPI__Module___MainMethodWrapper + 15
    frame #8: 0x00000001003ad27c ObjectiveCMarshalAPI`ObjectiveCMarshalAPI__Module___StartupCodeMain + 108
    frame #9: 0x00000001000073ef ObjectiveCMarshalAPI`main(argc=1, argv=0x00000003059f3788) at main.cpp:218:12
    frame #10: 0x0000000201902310 dyld`start + 2432

Notice the managed function UCO_ThrowIntException still show up in the stack trace after jumping to the propagation function. Compare to CoreCLR, where the managed function does not show up in the backtrace:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
  * frame #0: 0x000000010043fb94 libNativeObjCMarshalTests.dylib`(anonymous namespace)::ThrowInt(cxt=0x0000000000000d5f) at NativeObjCMarshalTests.cpp:105:9 [opt]
    frame #1: 0x000000010043fc0c libNativeObjCMarshalTests.dylib`::CallAndCatch(fptr=<unavailable>, a=3423) at NativeObjCMarshalTests.cpp:131:9 [opt]
    frame #2: 0x0000000118fbce1c
    frame #3: 0x0000000118fb7814
    frame #4: 0x0000000118fb0d98
    frame #5: 0x0000000102908f34 libcoreclr.dylib`CallDescrWorkerInternal + 132
    frame #6: 0x0000000102785284 libcoreclr.dylib`MethodDescCallSite::CallTargetWorker(unsigned long long const*, unsigned long long*, int) + 856
    frame #7: 0x000000010268bf4c libcoreclr.dylib`RunMain(MethodDesc*, short, int*, PtrArray**) + 656
    frame #8: 0x000000010268c228 libcoreclr.dylib`Assembly::ExecuteMainMethod(PtrArray**, int) + 396
    frame #9: 0x00000001026b3f94 libcoreclr.dylib`CorHost2::ExecuteAssembly(unsigned int, char16_t const*, int, char16_t const**, unsigned int*) + 616
    frame #10: 0x0000000102a3049c libcoreclr.dylib`coreclr_execute_assembly + 192
    frame #11: 0x0000000100005780 corerun`run(config=0x000000016fdff270) at corerun.cpp:429:18 [opt]
    frame #12: 0x0000000100002a88 corerun`main(argc=<unavailable>, argv=<unavailable>) at corerun.cpp:624:21 [opt]
    frame #13: 0x00000001a829be50 dyld`start + 2544

@jkotas
Copy link
Member

jkotas commented Jan 16, 2023

One thing I'm not sure is right is the back trace looks like this after jumping to ThrowInt in the test:

Yes, it does not look right. All managed frames should be gone when the propagation function is called.

@AustinWise
Copy link
Contributor Author

When I tried to step the StackFrameIterator past the reverse P/Invoke frame, I discovered that it skips directly from the reverse P/Invoke frame to the P/Invoke frame. This skips all the unmanaged frames in between:

https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp#L293-L318

https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/Runtime/StackFrameIterator.cpp#L1450-L1458

So I added some hacks to allow the the StackFrameIterator to unwind into unmanaged code. The backtrace from the ThrowInt no longer shows the managed UCO_ThrowIntException function:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
  * frame #0: 0x00000001019679e0 libNativeObjCMarshalTests.dylib`(anonymous namespace)::ThrowInt(cxt=0x0000000000000d5f) at NativeObjCMarshalTests.cpp:105:9
    frame #1: 0x0000000101967a60 libNativeObjCMarshalTests.dylib`::CallAndCatch(fptr=(ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program__UCO_ThrowIntException), a=3423) at NativeObjCMarshalTests.cpp:131:9
    frame #2: 0x00000001002d7d60 ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_NativeObjCMarshalTests__CallAndCatch + 112
    frame #3: 0x00000001002d8bd8 ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program___Validate_ExceptionPropagation + 536
    frame #4: 0x00000001002d860c ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program__Validate_ReferenceTracking_Scenario + 876
    frame #5: 0x00000001002d8d90 ObjectiveCMarshalAPI`ObjectiveCMarshalAPI_ObjectiveCMarshalAPI_Program__Main + 48
    frame #6: 0x00000001003d344c ObjectiveCMarshalAPI`ObjectiveCMarshalAPI__Module___MainMethodWrapper + 12
    frame #7: 0x00000001003d34c0 ObjectiveCMarshalAPI`ObjectiveCMarshalAPI__Module___StartupCodeMain + 96
    frame #8: 0x000000010000cb50 ObjectiveCMarshalAPI`main(argc=1, argv=0x000000016fdff688) at main.cpp:218:12
    frame #9: 0x00000001a829be50 dyld`start + 2544

@jkotas What do you think of the idea enabling the StackFrameIterator to unwind into unmanaged code? It does not look like it was really designed with this in mind, so any ideas on how to more elegantly enable this would be appreciated.

@jkotas
Copy link
Member

jkotas commented Jan 17, 2023

@jkotas What do you think of the idea enabling the StackFrameIterator to unwind into unmanaged code? It does not look like it was really designed with this in mind, so any ideas on how to more elegantly enable this would be appreciated.

  • Add a new StackFrameIterator::Flags that says whether to skips directly from the reverse P/Invoke frame to the P/Invoke frame. This flag should be set for GC stack walks, it should be cleared for EH stack walks. It will avoid need for the hacky IsEH method.
  • Add a new argument to the existing UnwindStackFrame method. I think it would look better than introducing a whole new method.

@AustinWise AustinWise force-pushed the austin/NativeAotObjectiveCMarshal-Exception branch from 1a523c7 to dadbc7d Compare January 21, 2023 23:59
The memory in the CONTEXT structure was not being full initialized.
Switching from using the transition frame to unwinding filled the stack
with 0xDD values. When VS displayed the backtrace after calling
RaiseFailFastException, it was corrupted (showing a selection of unrelated
frames).

This can reproduced on the main branch by changing the memset here to set
0xDD. So I think this a pre-existing problem.
@AustinWise AustinWise marked this pull request as ready for review January 22, 2023 04:30
@AustinWise
Copy link
Contributor Author

This is now ready for review.

I noticed what appears to be a bug for Windows. When calling RaiseFailFastException on an unhanded exception, the CONTEXT parameter contains uninitialized stack memory. Depending on what bytes happen to be on the stack, the call stack displayed in Visual Studio after calling RaiseFailFastException is corrupted. Zero-initializing the CONTEXT appears to fix the problem. Perhaps this fix is worth backporting to .NET 7, since it is small in scope and could aid in diagnosing crashes.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM! @VSadov @janvorli @MichalStrehovsky Any additional feedback?

@MichalStrehovsky
Copy link
Member

LGTM! @VSadov @janvorli @MichalStrehovsky Any additional feedback?

Not my area of expertise, but looks good to me!

@VSadov
Copy link
Member

VSadov commented Jan 25, 2023

asm offsets on windows look incorrect

@AustinWise
Copy link
Contributor Author

asm offsets on windows look incorrect

Fixed; too bad that MSVC does not print the expression in static_assert.

@VSadov
Copy link
Member

VSadov commented Jan 26, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@VSadov
Copy link
Member

VSadov commented Jan 26, 2023

The failures in extra-platforms do not look related to this change.

System.MissingMethodException : Constructor on type 'Microsoft.Extensions.Logging.LoggerFactoryOptions' not found.
   at System.Activator.CreateInstance[T]() + 0x94
   at Microsoft.Extensions.Options.OptionsFactory`1.Create(String) + 0x38
   at Microsoft.Extensions.Options.UnnamedOptionsManager`1.get_Value() + 0xb4
   at Microsoft.Extensions.Logging.LoggerFactory..ctor(IEnumerable`1 providers, IOptionsMonitor`1 filterOption, IOptions`1 options, IExternalScopeProvider scopeProvider) + 0xcc
   at Microsoft.Extensions.Logging!<BaseAddress>+0x78fce8
   at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0x14c
   at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.Invoke(Object, Object[], BinderBundle, Boolean) + 0x60
   at Internal.Reflection.Core.Execution.MethodInvoker.Invoke(Object, Object[], Binder, BindingFlags, CultureInfo) + 0x54
   at System.Reflection.Runtime.MethodInfos.RuntimePlainConstructorInfo`1.Invoke(BindingFlags, Binder, Object[], CultureInfo) + 0xa8
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitRootCache(ServiceCallSite, RuntimeResolverContext) + 0x84
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xe0
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x34
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.CreateServiceAccessor(Type serviceType) + 0xec
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0x9c
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(Type, ServiceProviderEngineScope) + 0x3c
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x64
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider) + 0x30
   at Microsoft.Extensions.Logging.Test.LoggerTest.Log_IgnoresExceptionInIntermediateLoggersAndThrowsAggregateException() + 0x70
   at Microsoft.Extensions.Logging!<BaseAddress>+0x73d730
   at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0xd8

@MichalStrehovsky
Copy link
Member

The failures in extra-platforms do not look related to this change.

System.MissingMethodException : Constructor on type 'Microsoft.Extensions.Logging.LoggerFactoryOptions' not found.
   at System.Activator.CreateInstance[T]() + 0x94
   at Microsoft.Extensions.Options.OptionsFactory`1.Create(String) + 0x38
   at Microsoft.Extensions.Options.UnnamedOptionsManager`1.get_Value() + 0xb4
   at Microsoft.Extensions.Logging.LoggerFactory..ctor(IEnumerable`1 providers, IOptionsMonitor`1 filterOption, IOptions`1 options, IExternalScopeProvider scopeProvider) + 0xcc
   at Microsoft.Extensions.Logging!<BaseAddress>+0x78fce8
   at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0x14c
   at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.Invoke(Object, Object[], BinderBundle, Boolean) + 0x60
   at Internal.Reflection.Core.Execution.MethodInvoker.Invoke(Object, Object[], Binder, BindingFlags, CultureInfo) + 0x54
   at System.Reflection.Runtime.MethodInfos.RuntimePlainConstructorInfo`1.Invoke(BindingFlags, Binder, Object[], CultureInfo) + 0xa8
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitRootCache(ServiceCallSite, RuntimeResolverContext) + 0x84
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xe0
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x34
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.CreateServiceAccessor(Type serviceType) + 0xec
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0x9c
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(Type, ServiceProviderEngineScope) + 0x3c
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x64
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider) + 0x30
   at Microsoft.Extensions.Logging.Test.LoggerTest.Log_IgnoresExceptionInIntermediateLoggersAndThrowsAggregateException() + 0x70
   at Microsoft.Extensions.Logging!<BaseAddress>+0x73d730
   at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0xd8

I think it's from @vitek-karas change. I submitted a revert of his pr and it doesn't show these failures: #81259

@jkotas jkotas merged commit 187508c into dotnet:main Jan 27, 2023
@AustinWise AustinWise deleted the austin/NativeAotObjectiveCMarshal-Exception branch January 28, 2023 05:33
@MichalStrehovsky
Copy link
Member

This change broke something around unhandled exceptions on Windows.

With the Convenience Visual Studio repro project workflow from here: https://github.com/dotnet/runtime/blob/f1bdd5a6182f43f3928b389b03f7bc26f826c8bc/docs/workflow/building/coreclr/nativeaot.md#convenience-visual-studio-repro-project. Place a throw new Exception() in main.

Previously, the experience looked like this in the debugger:

image

Now it looks like this (note "Frame not in module" and a useless stack):

image

This change made it into the Preview 1 snap. Unless we roll back, unhandled exceptions are going to be undiagnosable in Preview 1, at least on Windows.

@AustinWise
Copy link
Contributor Author

This change broke something around unhandled exceptions on Windows.

With the Convenience Visual Studio repro project workflow from here: https://github.com/dotnet/runtime/blob/f1bdd5a6182f43f3928b389b03f7bc26f826c8bc/docs/workflow/building/coreclr/nativeaot.md#convenience-visual-studio-repro-project. Place a throw new Exception() in main.

Previously, the experience looked like this in the debugger:

image

Now it looks like this (note "Frame not in module" and a useless stack):

image

This change made it into the Preview 1 snap. Unless we roll back, unhandled exceptions are going to be undiagnosable in Preview 1, at least on Windows.

This is should be fixed by #81010 .

@AustinWise
Copy link
Contributor Author

@MichalStrehovsky Just to be clear, this problem was reproducible before this PR was merged. This PR incidentally has groomed the stack in a way that makes RaiseFailFastException unhappy when it reads the uninitialized memory.

#81010 should probably be considered for servicing for 7.0 as well.

@MichalStrehovsky
Copy link
Member

My only gripe is that this is the first time I'm observing this failure mode and we just snapped 8.0 Preview 1 a couple hours ago, which means we're going to ship preview 1 like this. It's not the end of the world, but not great either.

Cc @agocke - see #80334 (comment)

@AustinWise
Copy link
Contributor Author

Sorry about that. In retrospect I should have made sure the PR fixing the uninitialized memory read landed before this PR made the call stacks useless. I'll keep that principal in mind going forward.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants