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

Generate AotHelper properties at compile time #2927

Open
jamescrosswell opened this issue Nov 30, 2023 · 5 comments
Open

Generate AotHelper properties at compile time #2927

jamescrosswell opened this issue Nov 30, 2023 · 5 comments

Comments

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Nov 30, 2023

A couple of different options

Use built in attributes

The RuntimeFeature.IsDynamicCodeSupported Property looks like it might do what we want in many cases (I think we're using AotHelper.IsNativeAot where we should probably be checking for Trimming compatibility instead in some cases). Need to do a search for uses of AotHelper.IsNativeAot and a bit of a review of these.

Generate these at compile time

For this AotHelper, instead of:

var stackTrace = new StackTrace(false);
IsNativeAot = stackTrace.GetFrame(0)?.GetMethod() is null;

Could we instead write an attribute at compile time? I imagine it's a lot cheaper (worth measuring) than trying to get a stack trace on a static constructor:

Something like this:

<Target Name="WriteSentryAttributes"
Condition="$(Language) == 'VB' or $(Language) == 'C#' or $(Language) == 'F#'"
BeforeTargets="BeforeCompile;CoreCompile"
Inputs="$(MSBuildAllProjects)"
Outputs="$(IntermediateOutputPath)$(SentryAttributesFile)">
<PropertyGroup>
<SentryAttributesFilePath>$(IntermediateOutputPath)$(SentryAttributesFile)</SentryAttributesFilePath>
</PropertyGroup>
<ItemGroup>
<SentryAttributes Include="System.Reflection.AssemblyMetadata">
<_Parameter1>Sentry.ProjectDirectory</_Parameter1>
<_Parameter2>$(ProjectDir)</_Parameter2>
</SentryAttributes>
<!-- Ensure not part of Compile, as a workaround for https://github.com/dotnet/sdk/issues/114 -->
<Compile Remove="$(SentryAttributesFilePath)" />
</ItemGroup>
<WriteCodeFragment AssemblyAttributes="@(SentryAttributes)" Language="$(Language)" OutputFile="$(SentryAttributesFilePath)">
<Output Condition="$(Language) != 'F#'" TaskParameter="OutputFile" ItemName="Compile" />
<Output Condition="$(Language) == 'F#'" TaskParameter="OutputFile" ItemName="CompileBefore" />
<Output TaskParameter="OutputFile" ItemName="FileWrites" />
</WriteCodeFragment>
</Target>

Here's a PR that added writing the attribute at build time as a reference: #1739

Originally posted by @bruno-garcia in #2920 (comment)

References

@vaind
Copy link
Collaborator

vaind commented Dec 1, 2023

For reference: dotnet/runtime#94380

@bitsandfoxes
Copy link
Contributor

I assume this would cover some previously edge cases where the SDK wrongly assumes NativeAOT on unsupported platforms (i.e. UWP)?

@jamescrosswell
Copy link
Collaborator Author

I assume this would cover some previously edge cases where the SDK wrongly assumes NativeAOT on unsupported platforms (i.e. UWP)?

I think initially when we were building AOT support, we identified and tried to avoid (at runtime) code that would throw errors when compiling AOT. It may well be trimming, rather than AOT, that prevents those code blocks from running however and we should probably change our runtime guards to check for trimming rather than AOT.

Currently if NET8_0_OR_GREATER we check to see if trimming is enabled and, if not, we assume IsNativeAot is also enabled. For previous runtimes we always set IsNativeAot to disabled but still check whether trimming is enabled:

#if NET8_0_OR_GREATER
// TODO this probably more closely represents trimming rather than NativeAOT?
internal static bool IsNativeAot { get; }
[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", Justification = AotHelper.SuppressionJustification)]
static AotHelper()
{
var stackTrace = new StackTrace(false);
IsTrimmed = stackTrace.GetFrame(0)?.GetMethod() is null;
IsNativeAot = IsTrimmed;
}
#else
// This is a compile-time const so that the irrelevant code is removed during compilation.
internal const bool IsNativeAot = false;
[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", Justification = AotHelper.SuppressionJustification)]
static AotHelper()
{
var stackTrace = new StackTrace(false);
IsTrimmed = stackTrace.GetFrame(0)?.GetMethod() is null;
}
#endif

I think that's a problem though. It's entirely possible to build net8.0 apps that are trimmed but not compiled AOT.

Then we have code like this:

var frames = (!AotHelper.IsNativeAot && _options.StackTraceMode == StackTraceMode.Enhanced)
? EnhancedStackTrace.GetFrames(stackTrace).Select(p => new RealStackFrame(p))
: stackTrace.GetFrames().Select(p => new RealStackFrame(p));

So it doesn't execute some potentially problematic code if IsNativeAot is set. On net8.0 that kind of works because there is an equivalence between trimming being enabled and native aot being detected. However on UWP that code would be executed even when trimming was enabled - which I think is what causes our problems on UWP.

@bitsandfoxes
Copy link
Contributor

If I get that right then we could get around this issue on UWP by updating the check instead of having to rely on a dedicated package?

@jamescrosswell
Copy link
Collaborator Author

If I get that right then we could get around this issue on UWP by updating the check instead of having to rely on a dedicated package?

Possibly. We still wouldn't be able to hook up the unhandled exception handler automatically as that relies on reflection... but that's not a huge issue (people can copy/paste 6-7 lines of code to hook that up)... and I think there is some device information we can't capture without a dedicated UWP library since gathering that relies on Windows Native libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants