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.

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

Encode calling convention in mangled function pointer representation #81122

Merged
merged 3 commits into from
Jan 31, 2023
Merged

Encode calling convention in mangled function pointer representation #81122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e7a10f
Encode calling convention in mangled function pointer representation
trylek Jan 24, 2023
e293180
Delete superfluous comment per Michal's PR feedback
trylek Jan 30, 2023
273ead6
Remove issues.targets exclusion for the function pointer test
trylek Jan 30, 2023
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
3 changes: 1 addition & 2 deletions src/coreclr/tools/Common/Compiler/NativeAotNameMangler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,9 +290,8 @@ private string ComputeMangledTypeName(TypeDesc type)
mangledName = GetMangledTypeName(((PointerType)type).ParameterType) + NestMangledName("Pointer");
break;
case TypeFlags.FunctionPointer:
// TODO: need to also encode calling convention (or all modopts?)
var fnPtrType = (FunctionPointerType)type;
mangledName = "__FnPtr" + EnterNameScopeSequence;
mangledName = "__FnPtr_" + ((int)fnPtrType.Signature.Flags).ToString("X2") + EnterNameScopeSequence;
Copy link
Member

Choose a reason for hiding this comment

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

@steveharter what was the decision about function pointer type identity within the runtime? Do we only consider the value of the legacy "calling convention flags" or do we also include calling conventions from modopts, or do we consider all modopts everywhere in the signature?

(I admit I dropped the ball on following the discussion. I'll need to read up on it to actually implement this in nativeaot but can't spend time on it right now.)

This fix looks good if we only consider the legacy calling convention flags.

Copy link
Member

Choose a reason for hiding this comment

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

Are runtime tests running with NativeAOT? It seems that there is a gap in NativeAOT as well. I don't recall seeing the function pointer tests failing for NativeAOT.

Copy link
Member Author

Choose a reason for hiding this comment

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

@fanyang-mono I defer to Michal to elaborate in more detail, according to the running times and the logs I'm seeing for the NativeAOT legs in the runtime (PR / CI) pipeline I'm guessing that right now just a small subset of Pri0 tests gets run on a regular basis in NativeAOT mode.

Copy link
Member

Choose a reason for hiding this comment

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

This codepath is only exercised in obscure cases in NativeAOT - #71749. Most of the time function pointers are treated as IntPtr right now. It will have to change for .NET 8.

And yes, we don't run regular Pri0/Pri1 CoreCLR test suite runs right now. Another thing getting addressed for 8.0.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha!

Copy link
Member

@jkotas jkotas Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@steveharter what was the decision about function pointer type identity within the runtime?

@steveharter is on vacation.

In current main, the function pointer runtime type identity includes calling convention from signature. Modopts are not included in the function pointer runtime type identity.

After #81006, the function pointer runtime type identity is only going to include managed vs. unmanaged. The full calling convention details and modopts are not going to be part of function pointer runtime type identity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Jan and Michal for your feedback. I have deleted the superfluous comment per Michal's suggestion in the 2nd commit.

mangledName += GetMangledTypeName(fnPtrType.Signature.ReturnType);

mangledName += EnterNameScopeSequence;
Expand Down
3 changes: 0 additions & 3 deletions src/tests/issues.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,9 +874,6 @@
<ExcludeList Include="$(XunitTestBinBase)/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests/*">
<Issue>https://github.com/dotnet/runtime/issues/81103</Issue>
</ExcludeList>
<ExcludeList Include = "$(XunitTestBinBase)/Loader/classloader/Casting/Functionpointer/**">
<Issue>https://github.com/dotnet/runtime/issues/81106</Issue>
</ExcludeList>
</ItemGroup>

<!-- Crossgen2 x86 specific excludes -->
Expand Down