Implement `Assembly.GetCallingAssembly` for native AOT #104229

MichalStrehovsky · 2024-07-01T11:30:26Z

The problem with this API has always been the possible non-existence of reflection metadata about the calling method. But I realized that the reflection metadata can be supplemented by stack trace metadata that also knows assemblies of all methods on the stack.

So this API is supportable as long as stack traces are not disabled. It's also conveniently easy to implement with the new DiagnosticMethodInfo API we added in .NET 9.

I'm not sure if we want to go as far as make this API not work on CoreCLR with JIT if StackTraceSupport is false, but we could do that. It might be preferable, but a bit breaking.

Resolves #94200.

Cc @dotnet/ilc-contrib

The problem with this API has always been the possible non-existence of reflection metadata about the calling method. But I realized that the reflection metadata can be supplemented by stack trace metadata that also knows assemblies of all methods on the stack. So this API is supportable as long as stack traces are not disabled. It's also conveniently easy to implement with the new `DiagnosticMethodInfo` API we added in .NET 9. I'm not sure if we want to go as far as make this API not work on CoreCLR with JIT if `StackTraceSupport` is false, but we could do that. It might be preferable, but a bit breaking. Resolves dotnet#94200.

am11 · 2024-07-01T11:50:53Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Assembly.NativeAot.cs

        public static Assembly GetCallingAssembly()
        {
            if (AppContext.TryGetSwitch("Switch.System.Reflection.Assembly.SimulatedCallingAssembly", out bool isSimulated) && isSimulated)
                return GetEntryAssembly();

-            throw new PlatformNotSupportedException();
+            if (!StackTrace.IsSupported)
+                throw new NotSupportedException(SR.NotSupported_StackTraceSupportDisabled);


We can update the relevant section at the same time: https://github.com/dotnet/runtime/blob/f8bcb89796d3c83264fffd913e5196d29d8d730e/src/coreclr/nativeaot/docs/reflection-in-aot-mode.md#simulated-calling-assembly

dotnet-policy-service · 2024-07-01T11:51:47Z

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

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

Co-authored-by: Martin Costello <[email protected]>

jkotas · 2024-07-01T17:25:22Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Assembly.NativeAot.cs

+
+            // We want to be able to handle GetCallingAssembly being called from Main (CoreCLR returns
+            // the assembly of Main), and also GetCallingAssembly being called from things like
+            // delegate invoke thunks. We do this by making the the definition of "calling assembly"


Suggested change

// delegate invoke thunks. We do this by making the the definition of "calling assembly"

// delegate invoke thunks. We do this by making the definition of "calling assembly"

jkotas · 2024-07-01T17:31:05Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Assembly.NativeAot.cs

+            // Technically we want skipFrames: 2 since we're interested in the frame that
+            // called the method that calls GetCallingAssembly, but we might need the first frame
+            // later in this method.
+            var stackTrace = new StackTrace(skipFrames: 1);


This looks expensive. What's the performance of this implementation of GetCallingAssembly compared to regular CoreCLR in typical case (e.g. GetCallingAssembly called somewhere from ASP.NET)?

Is your concern that we obtain entire stack trace as opposed to just a single frame? Both of these end up doing similar work anyway (they call RhGetCurrentStackTrace), just one of them will throw out everything but the one frame.

We could introduce a new specialized stack walking API, but I'm only doing this because it's cheap to implement.

I'm also fine just won't fixing the bug and telling people they can't do this until dotnet/csharplang#4984 is implemented because once this is no longer cheap to implement, it's not worth the effort.

My concern was that this is replacing naot functionality issue with naot-specific performance issue. The performance of this implementation can be much worse compared to CoreCLR one.

introducing specialized stackwalking method that does not walk the whole stack and some perf numbers would address my concern.

I am fine with won’t fixing.

introducing specialized stackwalking method that does not walk the whole stack and some perf numbers would address my concern.

Right, that would double the amount of work and risk/bug potential, tipping this over to the "not worth the effort given the limited benefit" category. Very few people missed this on .NET Native, and so do on native AOT. The csharplang issue is a much better fix. The only reason why I even looked at this is not that some users would benefit from this, but that we produce no warning about potential behavior difference, but this is a rarely used API and I can live with that hole.

jkotas · 2024-07-01T17:42:13Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Assembly.NativeAot.cs

+            // do if GetCallingAssembly is called from e.g. Main.
+            dmi ??= stackTrace.GetFrame(0) is StackFrame sf ? DiagnosticMethodInfo.Create(sf) : null;
+
+            return dmi.DeclaringAssemblyName is string asmName ? Load(asmName) : null;


Can dmi be null here?

Sergio0694 · 2024-07-03T01:36:27Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

+                MethodDesc caller = HandleToObject(callerHnd);
+
+                // Do not tailcall out of the entry point as it results in a confusing debugger experience.
+                if (caller is EcmaMethod em && em.Module.EntryPoint == caller)


Could we return true here and in the last check if StackTraceSupport is disabled? Same question for that CORINFO_FLG_DONT_INLINE_CALLER flag above that's not used on AOT too: could we not skip that too when StackTraceSupport is disabled, so we never miss optimizations there if we don't care about the stack trace anyway?

Could we return true here and in the last check if StackTraceSupport is disabled? Same question for that CORINFO_FLG_DONT_INLINE_CALLER flag above that's not used on AOT too: could we not skip that too when StackTraceSupport is disabled, so we never miss optimizations there if we don't care about the stack trace anyway?

What would be the advantage? Conditioning this on StackTraceSupport would only make sense if we also check this is GetCallingAssembly (and not some other method marked DynamicMethod). But in that case this API is going to throw anyway. Do we need it to throw faster?

MichalStrehovsky · 2024-07-10T12:24:01Z

On a second thought, it's better not to support this to put more pressure on dotnet/csharplang#4984 getting implemented.

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 1, 2024

dotnet-policy-service bot assigned MichalStrehovsky Jul 1, 2024

am11 reviewed Jul 1, 2024

View reviewed changes

am11 added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 1, 2024

Update reflection-in-aot-mode.md

8641fcc

martincostello reviewed Jul 1, 2024

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Jul 1, 2024

[8.0] [wasm] [AOT] HTTP Error 500.30 - ASP.NET Core app failed to start #95160

Open

Update src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

082016d

Co-authored-by: Martin Costello <[email protected]>

jkotas reviewed Jul 1, 2024

View reviewed changes

This was referenced Jul 1, 2024

[browser][MT] System.Net.Http.Functional.Tests.WorkItemExecution timeout #102385

Closed

TimeProviderTests.TestProviderTimer failed in CI #103459

Closed

[browser][MT] test failure: Tests timed out. Killing driver service #103524

Open

Sergio0694 reviewed Jul 3, 2024

View reviewed changes

MichalStrehovsky closed this Jul 10, 2024

MichalStrehovsky deleted the getcallingasm branch July 10, 2024 12:24

github-actions bot locked and limited conversation to collaborators Aug 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `Assembly.GetCallingAssembly` for native AOT #104229

Implement `Assembly.GetCallingAssembly` for native AOT #104229

MichalStrehovsky commented Jul 1, 2024

am11 Jul 1, 2024

dotnet-policy-service bot commented Jul 1, 2024

jkotas Jul 1, 2024

jkotas Jul 1, 2024

MichalStrehovsky Jul 9, 2024

jkotas Jul 10, 2024

MichalStrehovsky Jul 10, 2024

jkotas Jul 1, 2024

Sergio0694 Jul 3, 2024

MichalStrehovsky Jul 9, 2024

MichalStrehovsky commented Jul 10, 2024

	// delegate invoke thunks. We do this by making the the definition of "calling assembly"
	// delegate invoke thunks. We do this by making the definition of "calling assembly"

Implement Assembly.GetCallingAssembly for native AOT #104229

Implement Assembly.GetCallingAssembly for native AOT #104229

Conversation

MichalStrehovsky commented Jul 1, 2024

Choose a reason for hiding this comment

dotnet-policy-service bot commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Jul 10, 2024

Implement `Assembly.GetCallingAssembly` for native AOT #104229

Implement `Assembly.GetCallingAssembly` for native AOT #104229