Skip to content

Encode calling convention in mangled function pointer representation #81122

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
trylek merged 3 commits into from
Jan 31, 2023
Merged

Encode calling convention in mangled function pointer representation #81122

Changes from 1 commit
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
Next Next commit
Encode calling convention in mangled function pointer representation 
This fixes the recently introduced issue #81106 tracking Crossgen2
crash due to not being able to distinguish two function pointers
differing only by calling convention (one of the pointers is Static
and the other is Static | UnmanagedCallingConvention).

Thanks

Tomas
  • Loading branch information
@trylek
trylek committed Jan 30, 2023
commit 4e7a10fefbb4a2221c137442de7839e727453eff
4 changes: 2 additions & 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,9 @@ 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?)
// TODO: need to also encode modopts?
var fnPtrType = (FunctionPointerType)type;
mangledName = "__FnPtr" + EnterNameScopeSequence;
mangledName = "__FnPtr_" + ((int)fnPtrType.Signature.Flags).ToString("X2") + EnterNameScopeSequence;
Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 25, 2023

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

@fanyang-mono fanyang-mono Jan 25, 2023

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

@trylek trylek Jan 26, 2023

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

@MichalStrehovsky MichalStrehovsky Jan 26, 2023

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

@fanyang-mono fanyang-mono Jan 26, 2023

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

@trylek trylek Jan 30, 2023

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