Skip to content

Do not fold exported UnmanagedCallersOnly symbols #103884

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.

Sign up for GitHub

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

Merged
MichalStrehovsky merged 1 commit into from
Jun 24, 2024
Merged

Do not fold exported UnmanagedCallersOnly symbols #103884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectDataInterner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ private void EnsureMap(NodeFactory factory)
if (body is ISpecialUnboxThunkNode unboxThunk && unboxThunk.IsSpecialUnboxingThunk)
continue;

// Bodies that are visible from outside should not be folded because we don't know
// if they're address taken.
if (factory.GetSymbolAlternateName(body) != null)
continue;

var key = new MethodInternKey(body, factory);
if (methodHash.TryGetValue(key, out MethodInternKey found))
{
Expand Down
7 changes: 7 additions & 0 deletions src/tests/nativeaot/SmokeTests/StackTraceMetadata/BodyFoldingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,16 @@

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

class BodyFoldingTest
{
[UnmanagedCallersOnly(EntryPoint = "FoldableMethod1")]
static void FoldableMethod1() { }

[UnmanagedCallersOnly(EntryPoint = "FoldableMethod2")]
static void FoldableMethod2() { }

class SimpleDelegateTargets
{
public static object Return1DelStatic() => new object();
Expand Down
4 changes: 4 additions & 0 deletions src/tests/nativeaot/SmokeTests/StackTraceMetadata/StackTraceMetadata.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,8 @@
<Compile Include="StackTraceMetadata.cs" />
<Compile Include="BodyFoldingTest.cs" />
</ItemGroup>

<ItemGroup>
<IlcArg Include="--export-unmanaged-entrypoints" />
Copy link
Member

@jkotas jkotas Jun 24, 2024

Choose a reason for hiding this comment

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

Set IlcExportUnmanagedEntrypoints property to true instead of using the command line option directly?

It is what the other tests do:

runtime/src/tests/nativeaot/GenerateUnmanagedEntryPoints/GenerateUnmanagedEntryPoints.csproj

Line 5 in 3f1cfbb

<IlcExportUnmanagedEntrypoints>true</IlcExportUnmanagedEntrypoints>

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Jun 24, 2024

Choose a reason for hiding this comment

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

The reason I did it this way is because we're not really testing the thing is exported, just making sure linker doesn't error out.

If I use the property and we rename or delete the property, the test silently stops testing this. If it stays command line argument, the test build will fail if we ever rename or delete it.

But I can change it if you think it's not a risk worth mitigating.

</ItemGroup>
</Project>
4 changes: 4 additions & 0 deletions src/tests/nativeaot/SmokeTests/StackTraceMetadata/StackTraceMetadata_Stripped.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,8 @@
<Compile Include="StackTraceMetadata.cs" />
<Compile Include="BodyFoldingTest.cs" />
</ItemGroup>

<ItemGroup>
<IlcArg Include="--export-unmanaged-entrypoints" />
</ItemGroup>
</Project>